3주차 과제(황대선) #20
3주차 과제(황대선) #20
Conversation
| public List<Registration> getAllRegistration(){ | ||
| return registration.keySet().stream().map( | ||
| key -> registration.get(key) | ||
| ).toList(); | ||
| } | ||
|
|
||
| public List<PersonalInformation> getAllPersonalInformation(){ | ||
| return personalInformation.keySet().stream().map( | ||
| key -> personalInformation.get(key) | ||
| ).toList(); | ||
| } | ||
|
|
||
| public List<PersonalInformation> getPersonalInformation(List<String> keys){ | ||
| return keys.stream().map(key -> personalInformation.get(key)).toList(); | ||
| } | ||
|
|
||
| public List<Path> getAllPath(){ | ||
| return path.keySet().stream().map( | ||
| key -> path.get(key) | ||
| ).toList(); | ||
| } | ||
|
|
||
| public List<DesiredTime> getAllDesiredTime(){ | ||
| return desiredTime.keySet().stream().map( | ||
| key -> desiredTime.get(key) | ||
| ).toList(); | ||
| } | ||
|
|
There was a problem hiding this comment.
map의 values 함수를 사용하면 stream을 사용하지 않고 모든 Registration, Path, DesiredTime 등을 불러올 수 있을 것 같네요
There was a problem hiding this comment.
헉 그러네요!! 감사합니다
| zerozero(0,0),zeroone(0,1),zerotwo(0,2),zerothree(0,3),zerofour(0,4),zerofive(0,5),zerosix(0,6),zeroseven(0,7),zeroeight(0,8),zeronine(0,9),zeroten(0,10), | ||
| onezero(1,0),oneone(1,1),onetwo(1,2),onethree(1,3),onefour(1,4),onefive(1,5),onesix(1,6),oneseven(1,7),oneeight(1,8),onenine(1,9),oneten(1,10), | ||
| twozero(2,0),twoone(2,1),twotwo(2,2),twothree(2,3),twofour(2,4),twofive(2,5),twosix(2,6),twoseven(2,7),twoeight(2,8),twonine(2,9),twoten(10,10), | ||
| ; | ||
|
|
||
| private Integer row; | ||
| private Integer column; | ||
|
|
||
|
|
||
| Table(Integer row, Integer column) { | ||
| this.row = row; | ||
| this.column = column; | ||
| } | ||
|
|
||
| public int[] toArray(){ | ||
| return new int[]{this.getRow(), this.getColumn()}; | ||
| } | ||
| } |
There was a problem hiding this comment.
DesiredTime을 int[]로도 적용할 수 있는데 Enum을 채택하신 이유가 있을까요?? Enum으로 했을 때 어떤 장점이 있을까요?
There was a problem hiding this comment.
뭔가 싱글톤 개념으로 이점이 있을 것 같았는데 아래 코드들을 더 읽어보니 toArray를 호출하면서 새로운 int[]배열을 만들어 주고 있네요
There was a problem hiding this comment.
FlyWeight 패턴을 적용해서 메모리를 절약하는 게 목표였습니다.
코드를 좀 수정해서 아래처럼 작성해야할 것 같네요
@Getter
public enum Table {
zerozero(new int[]{1,1})
;
private int[] time;
Table(int[] time) {
this.time = time;
}
}| @GetMapping("/admin/{applicants}/{majors}/{programmers}/{path}/{desiredTime}") | ||
| public ResponseEntity<AdminQueryResponse> getAdmin( | ||
| @PathVariable(required = false) String applicants, | ||
| @PathVariable(required = false) String majors, | ||
| @PathVariable(required = false) String programmers, | ||
| @PathVariable(required = false) String path, | ||
| @PathVariable(required = false) String desiredTime) throws ExecutionException, InterruptedException { |
There was a problem hiding this comment.
값을 여러개 받아올 때는 PathVariable보다 RequestParam이 더 낫지않나요?
There was a problem hiding this comment.
url이 너무 어지러워요~~ 특히나 앞에 리소스 명시도 없이 계속 데이터를 받으면 그게 무슨 데이터인지 알 수가 없습니다.
최소한 /admin/applicants/{applicants}/majors/{majors}/programmers/{programmers}/path ... 이런식으로 하셔야합니다.
There was a problem hiding this comment.
그러네요. 프론트 입장에서 어떤 데이터인지 알기 힘들 것 같네요. RequestParam을 사용하는 게 더 나은 선택지 같습니다.
| @GetMapping("/admin/{applicants}/{majors}/{programmers}/{path}/{desiredTime}") | ||
| public ResponseEntity<AdminQueryResponse> getAdmin( | ||
| @PathVariable(required = false) String applicants, | ||
| @PathVariable(required = false) String majors, | ||
| @PathVariable(required = false) String programmers, | ||
| @PathVariable(required = false) String path, | ||
| @PathVariable(required = false) String desiredTime) throws ExecutionException, InterruptedException { |
There was a problem hiding this comment.
url이 너무 어지러워요~~ 특히나 앞에 리소스 명시도 없이 계속 데이터를 받으면 그게 무슨 데이터인지 알 수가 없습니다.
최소한 /admin/applicants/{applicants}/majors/{majors}/programmers/{programmers}/path ... 이런식으로 하셔야합니다.
| CompletableFuture<List<ApplicantDTO>> applicantFuture = applicants.isEmpty() | ||
| ? null | ||
| : CompletableFuture.supplyAsync(() -> applicantQueryService.execute()); | ||
|
|
||
| CompletableFuture<List<MajorDTO>> majorFuture = majors.isEmpty() | ||
| ? null | ||
| : CompletableFuture.supplyAsync(() -> majorQueryService.execute()); | ||
|
|
||
| CompletableFuture<List<ProgrammerFieldDTO>> programmerFuture = programmers.isEmpty() | ||
| ? null | ||
| : CompletableFuture.supplyAsync(() -> hopeFieldQueryService.execute()); | ||
|
|
||
| CompletableFuture<List<PathDTO>> pathFuture = path.isEmpty() | ||
| ? null | ||
| : CompletableFuture.supplyAsync(() -> supportPathQueryService.execute()); | ||
|
|
||
| CompletableFuture<List<DesiredTimeDTO>> desiredTimeFuture = desiredTime.isEmpty() | ||
| ? null | ||
| : CompletableFuture.supplyAsync(() -> desiredTimeQueryService.execute()); | ||
|
|
||
| CompletableFuture<AdminQueryResponse> completableFuture = CompletableFuture.allOf(applicantFuture, majorFuture, programmerFuture, pathFuture, desiredTimeFuture) | ||
| .thenApply(Void -> AdminQueryResponse.builder() | ||
| .applicants(applicantFuture.join()) | ||
| .majors(majorFuture.join()) | ||
| .programmers(programmerFuture.join()) | ||
| .path(pathFuture.join()) | ||
| .desiredTime(desiredTimeFuture.join()) | ||
| .build()); |
There was a problem hiding this comment.
컨트롤러에서 집계를 하지 마세요 집계 클래스를 따로 만드시는게 좋겠습니다.
Future를 이용한 집계를 도입한 부분이 인상적입니다.
future null join이 가능한가요? NPE가 발생하지 않나요?
최대한 nul을 대놓고 바인딩하는 것은 정말 위험합니다.
이런 경우 코드가 좀 길어질 수는 있겠지만 .handler 로 null 을 만날 경우 별도의 처리를 해주는 것도 방법이겠죠.
이렇게 코드를 짠 궁극적인 원인은 이미 response 클래스가 고정적으로 변수를 선언됐기 때문이겠죠. 거기부터 다시 생각해봅시다.
There was a problem hiding this comment.
집계 관련 유틸 클래스를 만드는 게 좋을 것 같네요 감사합니다!
allOf 메서드에서 null을 만나면 NPE가 발생합니다. 말씀하신 것처럼 null일 경우 allOf 메서드에 해당 객체가 가지 않도록 코드를 수정해야겠네요.
response 클래스를 Map 을 사용하지 않은 이유는 아래와 같습니다.
위의 이유들로 인해 Map을 사용하지 않고 DTO 응답 클래스를 생성하였습니다.
| List<DesiredTimeDTO> desiredTimeDTOS = tableListMap.keySet().stream() | ||
| .map(table -> | ||
| DesiredTimeDTO.of( | ||
| table, | ||
| tableListMap.get(table).size(), | ||
| database.getPersonalInformation( | ||
| tableListMap.get(table).stream() | ||
| .map(timeTable -> timeTable.getPersonalInfoId()).toList())) | ||
| ).toList(); |
There was a problem hiding this comment.
stream stream 인덴트 장풍이다!
size 같은 경우 클래스 내에 size를 호출하면 될 것이고,
stream 내에서 다른 클래스 함수 호출하면 side-effect 가 얼마나 클지 다시 고민해봅시다. 최대한 다른 클래스를 다 호출하고 stream 내에서는 집계만 하는게 어떨까요. 여러번 호출해야할 경우 In 쿼리 형태로 한번에 처리하도록 하면 좋겠죠. (그런식의 코드 스타일을 짜라는 말입니다)
| Map<Major, List<PersonalInformation>> majorListMap = allPersonalInformation.stream().collect(Collectors.groupingBy(PersonalInformation::getMajor)); | ||
| Map<Major, List<PersonalInformation>> doubleMajorListMap = allPersonalInformation.stream().collect(Collectors.groupingBy(PersonalInformation::getDoubleMajor)); | ||
| return majorListMap.keySet().stream() | ||
| .map(major -> MajorDTO.of(major.name(), majorListMap.get(major).size() + doubleMajorListMap.get(major).size())).toList(); |
There was a problem hiding this comment.
주전공 복수전공의 합을 합치는 경우 new 클래스 내에서 + 를 선언하기 보다는 합치는 메소드를 구현하거나 더 명시적인 방법이 좋지 않을까요?
There was a problem hiding this comment.
이것 또한 명시적으로 계산 하는 유틸 클래스를 만드는 것이 좋아보이네요. 감사합니다!
| public enum Major { | ||
| 산업공학과,소프트웨어공학과,컴퓨터정보통신공학과,Iot인공지능융합전공,디자인학과 | ||
| } |
There was a problem hiding this comment.
전공무관인 만큼 다양한 전공이 있을 수 있는데 이것을 enum으로 관리하는건 적절하지 않다고 봐요
미처 enum으로 정의하지 않은 학과에서 동아리 지원을 한다면...?
There was a problem hiding this comment.
enum으로 전공을 관리하였을 때 제가 생각하는 장단점은 아래와 같습니다.
장점
- Enum을 이용하여 Validation을 할 수 있다. 즉, 존재하지 않는 학과를 걸러낼 수 있습니다.
- 싱글톤이라 메모리를 절약할 수 있다.
만약 정의 되어 있지 않거나 잘못된 학과(산업공학과가 아닌 산ㅋ공ㅋ업과) 이런 식으로 온다면, 잘못된 값이 저장되는 것도 문제고 이를 집계하는 것 또한 어렵다고 판단해서, Enum으로 명시적으로 관리하는 게 좋다고 생각했습니다.
단점
- Enum으로 정의할 학과들이 너무 많다.
- 너무 많아서 정의하지 못한 학과들이 있을 수 있다.
- 확장에 취약하다.
| public enum SupportPath { | ||
| 홍보_포스터,학교_공지사항,지인소개,인스타그램,에브리타임 | ||
| } |
There was a problem hiding this comment.
경로에 제 기억으로는 기타가 있었던 걸로 기억합니다.. 기타에 유튜브 보고 왔음 라고 했을 때 어떻게 하실거죠?
There was a problem hiding this comment.
�음.. 문제에서 기타라는 항목은 찾지 못 했습니다.
다만, 확장에 취약하네요.
만약, 기타를 선택할 수 있고, 기타를 문자열로 무언가를 적을 수 있을 경우는 지원 경로는 enum으로 관리하고 기타 유튜브 보고 왔음은 따로 저장해야할 것 같네요.
PR이 merge되서 다시 PR 날립니다!