3주차 과제 (윤석호) #17
3주차 과제 (윤석호) #17
Conversation
기준이 바뀔때마다 클래스 하나씩 추가하는건 좋지 않다고 생각돼서 삭제함
| "major", personalInformationService.getApplicantNumberEachMajor(), | ||
| "path", pathService.getApplicantNumberEachPath(), | ||
| "desired_time", desiredTimeService.getApplicantNumberEachTime() | ||
| )); |
There was a problem hiding this comment.
컨트롤러에서 집계 호출 때리지 마세요. 컨트롤러는 위 서비스의 기술 스펙이다 생각하셔야지 구현체가 아닙니다.
There was a problem hiding this comment.
아 그렇군요 어떻게 할지 고민해보겠습니다
| try { | ||
| Thread.sleep(1000); | ||
| } catch (InterruptedException e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
?? 굳이?? 스스로 제한사항을 걸어보신거죠?
There was a problem hiding this comment.
비동기로 동작하는지 확인해보려고 넣었습니다!
| db.getAllPersonalInformation().forEach(info -> { | ||
| //주전공 | ||
| majorCounts.merge(info.getMajor(), 1, Integer::sum); | ||
| //복수전공 | ||
| info.getDoubleMajor().ifPresent(doubleMajor -> majorCounts.merge(doubleMajor, 1, Integer::sum)); | ||
| }); |
There was a problem hiding this comment.
합계에 필요한 데이터를 지금 순차적으로 한개씩 만들어서 map에 한개씩 추가하는 코드입니다.
이 코드보다는 동시에 다 호출해서 호출이 끝나는 대로 집계를 하는게 좋지 않을까요? .merge.merge.merge 이렇게 짜면 데이터 하나 늘어날때마다 merge하는건 너무 코드가 길어지니까요.
|
|
||
| db.getAllRegistrations() | ||
| .forEach(regi->{ | ||
| jobPriorityCounts.get(regi.getFirstPriority())[0]++; |
There was a problem hiding this comment.
명시적인 값을 집어넣는게 아니라 1개씩 추가하는 명시적이지 않습니다.
조회 집계하는 코드를 메소드 분리를 더 해보셔야 할 것 같습니다. SRP를 준수하고 있는지 고민해보세요
| .forEach(regi->{ | ||
| jobPriorityCounts.get(regi.getFirstPriority())[0]++; | ||
| regi.getSecondPriority() | ||
| .ifPresent(secondPriority -> jobPriorityCounts.get(secondPriority)[1]++); |
There was a problem hiding this comment.
값에서 index로 1 로 이렇게 위치로 조회하는 것은 가독성을 떨어뜨립니다.
| return jobPriorityCounts.entrySet().stream() | ||
| .map(entry-> | ||
| new ApplicantNumberInField( | ||
| entry.getKey().getFieldName(), | ||
| entry.getValue()[0], | ||
| entry.getValue()[1] | ||
| ) | ||
| ) | ||
| .sorted(reverseOrder()) | ||
| .toList(); |
There was a problem hiding this comment.
너무 엔트리에 깊게 의존된 느낌이군요. 이렇게 생성자를 자주 호출된다면 별도의 커스텀 생성자를 만드셔서 파라미터로 entry를 집어넣어서 파라미터가 1개인 생성자를 쓰는게 깔끔할 것 같습니다. Collect(Dto::new) 이런식으로 하면 7줄이 아니라 1줄로 끝나게 되고 명시적이고 좋은 것처럼요.
There was a problem hiding this comment.
생각 못한 방법인거 같습니다 감사합니다.
| public Integer upsertApplication(Integer userId, Application application){ | ||
| Integer applicationId = applications.computeIfAbsent(userId, k -> application.getApplicationId()); | ||
| upsertRegistration(applicationId, application.getRegistration()); | ||
| upsertPath(applicationId, application.getPath()); | ||
| upsertDesiredTime(applicationId, application.getDesiredTime()); | ||
| upsertPersonalInformation(applicationId, application.getPersonalInformation()); | ||
| return applicationId; | ||
| } | ||
| public void upsertRegistration(Integer applicationId, Registration registration){ | ||
| registrations.put(applicationId, registration); | ||
| } | ||
| public void upsertPath(Integer applicationId, Path path){ | ||
| paths.put(applicationId, path); | ||
| } | ||
| public void upsertPersonalInformation(Integer applicationId, PersonalInformation personalInformation){ | ||
| this.personalInformation.put(applicationId, personalInformation); | ||
| } | ||
| public void upsertDesiredTime(Integer applicationId, DesiredTime desiredTime){ | ||
| desiredTimes.put(applicationId, desiredTime); |
There was a problem hiding this comment.
upsert라는 개념을 put 한줄로 표현한 부분 아주 깔끔합니다. 아주 잘하셨습니다.
차 후, Map이 아니라, 일반 클래스에서도 적용하는 부분도 한번 짜보세요!
There was a problem hiding this comment.
hashMap에 key가 중복인 데이터를 넣을 때 value가 덮어씌워진다는 것을 이용하여 upsert로 표현한게 멋지네요
No description provided.