3주차 과제(이진혁) #15
3주차 과제(이진혁) #15
Conversation
| public ResponseEntity<List<Map<String,Object>>> getAdminPage(@RequestBody GetAdminPageRequest request) { | ||
| List<Map<String,Object>> response = adminPageHelper.setResponse(request.requestInfos()); | ||
| return ResponseEntity.ok().body(response); | ||
| } |
There was a problem hiding this comment.
반환할 경우 Object쓰는건 안티패턴입니다. 구체적인 타입 명시나 제네릭을 사용하십쇼! 당신은 타입형 언어를 쓰고 있습니다. 으딜 자스같은 반환형을
There was a problem hiding this comment.
ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ DTO 다 만들기 귀찮아서 Object 사용했는데 죄송합니다...
|
|
||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class GetDesiredTimeUseCase { |
There was a problem hiding this comment.
도메인이 가장 첫 단어로 들어가야 나중에 파일 많아질때 금방 찾습니다.
There was a problem hiding this comment.
항상 관슴적으로 동사를 먼저 적었는데 확실히 도메인 먼저 적으면 찾기 쉽겠네요
| public class GetPathSumUseCase { | ||
| private final Database database; | ||
|
|
||
| public Map<String, Long> execute() { | ||
| return database.findAllPath().stream().collect( | ||
| groupingBy(Path::getSupportPath, Collectors.counting()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
좋긴 한데 저 도메인 조회에 누적 합 구하는 별도의 케이스가 맞나 싶긴 하네요 너무 구체적인건 아닌가 싶기도 하고.
이러면 나중에 차 곱 합 나누기 불라불라 다 만들건 아니잖아요?
| public enum Priority { | ||
| WEB("WEB"), | ||
| APP("APP"), | ||
| AI("AI"), | ||
| GAME("GAME"), | ||
| IoT("IoT"), | ||
| ARVR("AR/VR"); | ||
|
|
||
| private String value; | ||
| Priority(String value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| String getValue() { | ||
| return value; | ||
| } | ||
| } |
There was a problem hiding this comment.
우선순위라고 우리는 대충 알고는 있지만 모르는 사람은 이게 뭔지 잘 모를 수 있기 때문에 주석을 이런 이넘이나 doc 파일같은 경우 주석을 잘 다는 습관을 달아봅시다.
| Map<String,Long> firstPriority = registrations.stream().collect( | ||
| Collectors.groupingBy(Registration::getFirstPriority, Collectors.counting()) | ||
| ); | ||
| Map<String,Long> secondPriority = registrations.stream().collect( | ||
| Collectors.groupingBy(Registration::getSecondPriority, Collectors.counting()) | ||
| ); |
There was a problem hiding this comment.
이렇게 되면 2번 순회를 하는 것이기 때문에 한번에 끝내는 걸로 최적화할 수 있을 것 같습니다.
Map<String, Long>[] result = registrations.stream().collect(
Collectors.teeing(
Collectors.groupingBy(Registration::getFirstPriority, Collectors.counting()),
Collectors.groupingBy(Registration::getSecondPriority, Collectors.counting()),
(first, second) -> new Map[]{first, second}
)
);이런식으로 합칠 수도 있구요. forEach로 찾는 방법도 있구요. 고민해보시면 좋을듯?
| private void checkRequestInfo(String requestInfo) { | ||
| switch (requestInfo) { | ||
| case "Applicant": | ||
| response.put("ApplicantSum", getApplicantSumUseCase.execute()); | ||
| break; | ||
| case "Path": | ||
| response.put("PathSum", getPathSumUseCase.execute()); | ||
| break; | ||
| case "PersonalInfo": | ||
| response.put("PersonalInfoSum", getPersonalInfoUseCase.execute()); | ||
| break; | ||
| case "Priority": | ||
| response.put("PrioritySum", getPrioritySumUseCase.execute()); | ||
| break; | ||
| case "DesiredTime": | ||
| response.put("DesiredTimeSum", getDesiredTimeUseCase.execute()); | ||
| break; | ||
| default: | ||
| throw new IllegalArgumentException("Invalid request info"); | ||
| } | ||
| } |
There was a problem hiding this comment.
코드가 아주 깔끔합니다. 하지만 조금 더 개선해서 전략 패턴이나 이 내부 구현을 한번 더 추상화하면 더 좋을 것 같습니다.
전략패턴을 사용할 수도 있구요.
그리고 코드에 값을 집어넣는 것 같은데check라는 동사가 맞을까 싶습니다. 차라리 set이 맞지 않나 아니면 add라던지
| public List<Map<String,Object>> setResponse(List<String> requestInfos) { | ||
| return requestInfos.stream() | ||
| .map(req -> { | ||
| checkRequestInfo(req); | ||
| return response; | ||
| }) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
이렇게 짤 경우 반환해야할 데이터가 기획적으로 추가해야 할경우 switch문을 추가로 까야할 수도 있습니다. 이런게 확장에 불리한 구조라고 하는데요. 이 부분 꼭 다시 수정해보시길 바랍니다. 주로 aggregate과정에서는 각 데이터를 조회하는 걸 클래스단위로 구분하는게 정석이긴 합니다.
There was a problem hiding this comment.
checkRequestInfo 랑 같이 한번 리팩토링 해보도록 하겠습니다!
| @@ -25,4 +31,10 @@ public ResponseEntity<Registration> getRegistration(String userId) { | |||
| return ResponseEntity.ok().body(database.getRegistration(userId)); | |||
| } | |||
|
|
|||
| @GetMapping("/admin") | |||
| public ResponseEntity<List<Map<String,Object>>> getAdminPage(@RequestBody GetAdminPageRequest request) { | |||
There was a problem hiding this comment.
데이터를 주고 받는 데 있어서 Map은 지양하고 있는 데 응답 클래스로 Map을 사용한 이유가 있을까요??
No description provided.