Refactor/#857 event query service 리팩터링 #868
Conversation
There was a problem hiding this comment.
@java-saeng
덕분에 코드량도 많이 줄어들고 가독성도 좋아진 것 같아요.
코드를 보면서 크게 의문이 들거나 더 개선할법한 부분은 보이지 않았어서 바로 approve하겠습니다.
수고하셨습니다!
| eq(null), any(), eq("컨퍼"))).thenReturn(eventResponses); | ||
| Mockito.when(eventQueryService.findEvents(any(EventSearchRequest.class), | ||
| any(LocalDate.class), any(LocalDate.class), any(LocalDateTime.class))) | ||
| .thenReturn(eventResponses); |
There was a problem hiding this comment.
통일성 있게 any()로 맞춰주셨네요! 전보다 보기 깔끔해진 것 같습니다~
| @@ -44,7 +45,7 @@ public ResponseEntity<ExceptionResponse> handleValidationException( | |||
| return new ResponseEntity<>(new ExceptionResponse(message), HttpStatus.BAD_REQUEST); | |||
| } | |||
|
|
|||
| @ExceptionHandler(HttpMessageNotReadableException.class) | |||
| @ExceptionHandler({HttpMessageNotReadableException.class, MethodArgumentTypeMismatchException.class}) | |||
There was a problem hiding this comment.
오...enum 타입과 일치하지 않는 입력이 들어오면 이런 예외가 발생하는군요...!
덕분에 알아갑니다!bb
| @RequestParam(required = false) final List<String> tags, | ||
| @RequestParam(required = false) final List<EventStatus> statuses, | ||
| @RequestParam(required = false) final String keyword) { | ||
| final EventSearchRequest eventSearchRequest, |
There was a problem hiding this comment.
GET 요청에서는 이렇게 아무 어노테이션 없이 DTO를 받게 하면 @RequestParam으로 받는군요...! 이쪽은 잘 모르고 있었는데 알아갑니다.
코드도 훨씬 깔끔해져서 좋은 것 같아요.
| .map(EventTag::getTag) | ||
| .map(Tag::getName) | ||
| .collect(toUnmodifiableList()); | ||
| final List<String> tagNames = event.extractTags(); |
There was a problem hiding this comment.
오 이게 중복 코드가 있었군요....굿입니다👍👍
| if (category == null) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
검증로직들을 EventSpecification 클래스로 옮기니 Service 코드의 가독성이 훨씬 좋아졌네요. 좋습니다👍
| return Specification | ||
| .where(EventSpecification.filterByCategory(request.getCategory())) | ||
| .and(EventSpecification.filterByTags(request.getTags())) | ||
| .and(EventSpecification.filterByPeriod(startDate, endDate)) |
There was a problem hiding this comment.
이런식으로 체이닝을 거는 방식으로 구현하니 훨씬 깔끔하네요.
| ) { | ||
| return events.stream() | ||
| .filter(isEventContainsStatus(eventStatuses, nowDateTime)) | ||
| .sorted(comparing(event -> event.getEventPeriod().getStartDate())) |
There was a problem hiding this comment.
지금 남기는건 기능적인 부분이 아닌 기획적인 부분에 대한 내용입니다.
이전에 회의할 때 행사를 시작일순으로 정렬하는 것이 아닌 마감임박일 순으로 정렬하는 것이 어떻겠냐는 의견이 있었어요.
저희 어플을 쓰는 경우에 이미 신청한 행사라면 안 볼 것 같기도 하고,
유저가 원하는건 마감임박일일 것 같아서요.
There was a problem hiding this comment.
시작일 순으로 정렬될 경우, 1달 이상하는 행사가 계속 최상단을 먹어서 새로운 행사를 보려면 내려야 하더라고요.
이건 추후 기획회의 때 한 번 이야기해보면 좋을 것 같아요.
| ); | ||
| } | ||
|
|
||
| private Predicate<Event> isEventContainsStatus( |
There was a problem hiding this comment.
이 isEventContainsStatus는 boolean값들을 반환하고 있군요.
반환값을 Preidicate 보단 boolean으로 바꾸는건 어떤가요?
There was a problem hiding this comment.
혹시 왜 그러는지 알 수 있을까요??
어떤 메서드가 functional interface 를 반환하면 좋지 않은건지 궁금합니다
There was a problem hiding this comment.
사실 이게 바꿔도 영향은 없긴 한데, boolean을 반환하는게 더 일반적이라고 생각했어요.
굳이 바꾸지 않으셔도 될 것 같습니다.
#️⃣ 연관된 이슈
📝 작업 내용
�EventQueryService 리팩터링
예상 소요 시간 및 실제 소요 시간 (일 / 시간 / 분)
예상 소요 시간 : 2주
실제 소요 시간 : 1주
💬 리뷰어 요구사항 (선택)