미니 프로젝트 Step03 #18
미니 프로젝트 Step03 #18
Conversation
Yeong-Huns
requested review from
SungbinYang,
crispindeity,
jo-kim,
leeedohyun and
Darimi2222
| import org.example.yeonghuns.config.Error.exception.BadRequestException; | ||
|
|
||
| public class RemainAnnualLeavesException extends BadRequestException { | ||
| public RemainAnnualLeavesException() { super(ErrorCode.NOT_REMAIN_ANNUAL_LEAVE); } |
There was a problem hiding this comment.
super 부분을 줄바꿈한 예외 클래스도 있고, 안 한 것도 있는데 이유가 있으신가요?
There was a problem hiding this comment.
지금 확인해보니 이 부분은 인텔리제이 기능 때문에 인지하지 못한거 같습니다 .
자동으로 한줄로 보이게 하는 기능이 있더라구요 😂
| ChronoUnit.DAYS.between(LocalDate.now(), request.date()) < member.getTeam().getDayBeforeAnnual(); | ||
| } | ||
| private long remainAnnualLeaves(Member member){ // 남은 연차 계산 & 연차 조회시 반환 | ||
| long maxAnnualLeave = ChronoUnit.YEARS.between(member.getCreatedAt(), LocalDateTime.now()) >= 1 ? 15L : 11L; |
There was a problem hiding this comment.
매직 넘버를 상수로 처리하면 가독성이 더 좋아질 것 같은데, 어떻게 생각하시나요?
There was a problem hiding this comment.
오 마침 방금 enum 처리하고 커밋했습니다 😊😊
| public boolean isAlreadyUsingAnnualLeaves(Member member, LocalDate date){ | ||
| return annualLeaveRepository.existsByMemberIdAndAnnualDateLeaveEquals(member.getId(), date); | ||
| } |
There was a problem hiding this comment.
AnnualLeaveService에서만 쓰이는 것 같은데, public으로 열어둔 이유가 있을까요?
There was a problem hiding this comment.
이부분은 CommuteService 에서 연차사용일에 출근하려하는 경우 검증을 위해 사용하고 있습니다..!
| //해당범위에 통근기록 존재 X? -> 통근기록없음 예외처리 | ||
| List<GetCommuteDetail> commuteDetailList = commuteList.stream() | ||
| .map(GetCommuteDetail::from) | ||
| .collect(Collectors.toList()); //통근기록을 CommuteDetail으로 변환 |
There was a problem hiding this comment.
Collectors.toList()와 Stream.toList()의 차이를 아시나요? Collectors.toList()를 사용한 이유가 궁금합니다!!
There was a problem hiding this comment.
이 부분은 .toList()로 처리하고 싶었지만, 바로 아래 mergeAndSort 를 위해 Collectors.toList()를 사용했습니다 ㅠ
.toList()는 수정이 불가능한 List를 반환하기 때문에 .addAll을 사용해도 적용이 되지 않았습니다.
mergeAndSort 로직을 .addAll() 을 사용하지 않고 전달받은 리스트를 전부 스트림으로 변환해서 .Concat() 으로
처리할까 고민도 해보았지만, 가독성이 너무 떨어지는 느낌이라 .addAll()을 사용하였습니다 🙇
There was a problem hiding this comment.
피드백 감사합니다.>! 😊
변환 가능한 리스트를 반환하는게 저도 조금 신경쓰였는데,
이 부분은 mergeAndSort 종료 후,
Merge가 완료된 CommuteList를 Collections.unmodifiableList()를 통해 불변 List로 감싸서 반환하도록 하겠습니다 🙇
이렇게 처리하면 .addAll()도 사용할 수 있고, 반환되는 List도 수정할 수 없게 만들 수 있을듯 합니다!
There was a problem hiding this comment.
private List<GetCommuteDetail> findCommuteListByMemberIdAndStartOfWork(GetCommuteRecordRequest request) {
List<Commute> commuteList = commuteRepository
.findCommuteListByMemberIdAndStartOfWork(request.id(), request.getYear(), request.getMonth());
if (commuteList.isEmpty()) throw new CommuteNotFoundException();
//해당범위에 통근기록 존재 X? -> 통근기록없음 예외처리
List<GetCommuteDetail> commuteDetailList = commuteList.stream()
.map(GetCommuteDetail::from)
.collect(Collectors.toList()); //통근기록을 CommuteDetail으로 변환
List<AnnualLeave> annualLeaveLeavesList = annualLeaveService // 연차기록찾기 (오늘보다 미래의 연차기록은 가져오지않음)
.findAnnualLeavesByMemberIdAndYearMonth(request.id(), request.yearMonth());
mergeAndSort(commuteDetailList, annualLeaveLeavesList); // .addAll()을 통한 merge
return Collections.unmodifiableList(commuteDetailList); // 불변리스트로 변환 후 반환
}피드백 적용 후 코드입니다!
| import org.springframework.boot.SpringApplication; | ||
| import org.springframework.boot.autoconfigure.SpringBootApplication; | ||
|
|
||
| import java.time.Duration; | ||
| import java.time.LocalDate; | ||
| @Slf4j |
There was a problem hiding this comment.
혹시 메인 메서드에 @Slf4j 어노테이션을 붙이신 이유가 따로 있으실까요?
There was a problem hiding this comment.
@Slf4j 테스트 해보려고 log 한번찍어보고 지우는걸 깜빡한거 같습니다 😭
바로 지우겠습니다 ! 감사합니다 🙇
| @@ -30,6 +31,12 @@ protected ResponseEntity<ErrorResponse> handle(MethodArgumentNotValidException e | |||
| return createErrorResponse(ErrorCode.INVALID_INPUT_VALUE); | |||
| } | |||
|
|
|||
| @ExceptionHandler(HttpMessageNotReadableException.class) | |||
| protected ResponseEntity<ErrorResponse> handle(HttpMessageNotReadableException e){ | |||
| e.getStackTrace(); | |||
There was a problem hiding this comment.
아래 로그로 한번 찍는데 stackTrace부분을 하는 이유를 알 수 있을까요? 제가 생각하기에는 아래와 동일할 것 같은데 궁금해서 한번 여쭤봅니다.
There was a problem hiding this comment.
피드백 감사합니다 🙇
@Slf4j를 사용하기 전에 찍어뒀던건데,
지우는걸 깜빡하고 복붙까지 해버렸네요 🤣
바로 삭제하겠습니다!
| JoinDate(long maxAnnualLeaves) { | ||
| this.maxAnnualLeaves = maxAnnualLeaves; | ||
| } |
There was a problem hiding this comment.
롬복을 사용하시는 것 같은데 이 부분을 제외하고 @RequiredargsConstructor로 변경해보시는 것은 어떠신가요?
There was a problem hiding this comment.
아주 좋습니다!😊 바로 반영하겠습니다!!! 🙇
🚀 작업 내용
Version02💬 리뷰 중점사항
📌 PR 진행 시 이러한 점들을 참고해 주세요