Step4 리뷰 부탁드립니다 #903
Step4 리뷰 부탁드립니다 #903
Conversation
seokhongkim
left a comment
seokhongkim
left a comment
There was a problem hiding this comment.
동길님, 4단계 미션 잘 구현해주셨습니다. 👍
코드를 보고 고민해보시면 좋을 것 같은 내용을 코멘트로 남겼습니다.
코멘트 확인 부탁드립니다. 🙏
| import org.springframework.web.bind.annotation.RestControllerAdvice; | ||
|
|
||
| @RestControllerAdvice | ||
| public class GlobalExceptionAdvisor { |
|
|
||
| import java.util.Arrays; | ||
|
|
||
| public enum DiscountPolicy { |
There was a problem hiding this comment.
나이에 따른 할인 요금 계산 로직 잘 구현해주셨습니다. 👍
ageStart, ageEnd도 충분히 무슨 역할을 하는지 알 수 있는 이름인데, 비교에 사용되니까 이상/초과, 이하/미만 구분을 위해 ageMinInclusive, ageMaxExclusive`와 같이 수정해보셔도 좋겠습니다.
|
|
||
| public int getDiscount(int fare) { | ||
| if (fare < 0) { | ||
| throw new IllegalArgumentException(); |
There was a problem hiding this comment.
예외를 발생시킬 때 예외 내용, 디버깅에 필요한 값을 메시지로 함께 전달하시면, 문제 상황에서 원인 추적이 수월해서 좋습니다. :)
| if (distance > this.distanceEnd) { | ||
| distance = this.distanceEnd; | ||
| } | ||
| return ((distance - this.distanceStart) / this.unitDistanceInKiloMeter) * this.fare; |
There was a problem hiding this comment.
16km를 가는 경우 추가 요금은 ((16 - 10) / 5) * 100 = 100원이 될 것 같아요!
참고 링크에 있는 값과 결과가 다른 것 같습니다.
미션 페이지에 있는 식을 사용해서 추가 요금을 계산해보시면 좋겠습니다. :)
|
|
||
| import java.util.Objects; | ||
|
|
||
| public class Fare { |
There was a problem hiding this comment.
값 객체 사용 좋습니다. 👍
고민해보시면 좋을 것 같은 부분은, Fare 클래스를 사용하는 사람이 plus, discount 순서를 잘 알고 호출해야 요금 계산이 정상적으로 될 것 같아요. 그리고, 할인 정책은 생성하는 시점에 받지만, 실제로는 추가 요금 계산이 끝난 후 사용이 되어야 하네요.
요금 계산은 FareCalculator가 담당하는 것으로 보이니, DiscountPolicy를 요금 계산 메서드에서 관리해보셔도 좋겠습니다. :)
| private void checkSourceTargetHasPath(Long sourceStationId, Long targetStationId) { | ||
| pathService.findShortestPath(sourceStationId, targetStationId); | ||
| private void checkSourceTargetHasPath(Long memberId, Long sourceStationId, Long targetStationId) { | ||
| pathService.findShortestPath(memberId, sourceStationId, targetStationId); |
There was a problem hiding this comment.
최단 경로를 조회하는 메서드가 요금 계산도 하기 때문에 memberId가 필요하게 된 걸까요?
경로 조회와 요금 계산을 분리해봐도 좋을 것 같네요! :)
| return new PathFinder(allSections, algorithm); | ||
| } | ||
|
|
||
| public List<Station> find(Station departStation, Station destStation) { |
There was a problem hiding this comment.
사용처가 없어진 것 같으니, 이 메서드를 포함해 PathFindAlgorithm의 findShortestPathStations를 삭제하시면 좋겠습니다. :)
| } | ||
|
|
||
| public PathResponse findShortestPath(Long source, Long target) { | ||
| public PathResponse findShortestPath(Long loginId, Long source, Long target) { |
There was a problem hiding this comment.
경로 계산하는 부분과 요금 계산하는 로직을 잘 분리할 수 있게 구현하신 것으로 보여요.
실제로 담당하는 클래스를 나눠보면 어떨까요? FavoriteService 에서는 경로가 존재하는지만 확인하면 되니 재사용도 좀 더 효율적으로 할 수 있을 것 같네요!
| @RequestParam("source") Long source, | ||
| @RequestParam("target") Long target) { | ||
| PathResponse response = service.findShortestPath(source, target); | ||
| @AuthenticationPrincipal LoginMember loginMember, |
There was a problem hiding this comment.
로그인하지 않은 경우에도 경로를 조회할 수 있어야 할 것 같아요.
이를 위해 AuthenticationPrincipal에 인증이 필요한지 여부 필드를 추가하고, 이를 통해 인증을 하지 않은 회원도 사용할 수 있는 기능(경로 조회 등)과 인증을 해야만 사용할 수 있는 기능(회원 정보 변경 등)에서 모두 LoginMember를 이용할 수 있도록 관리해보시면 좋겠습니다.
https://edu.nextstep.camp/s/urD8knTu/ls/4r6iyUcO
"/paths 요청 시 LoginMember 객체 처리" 부분을 참고 부탁드립니다.
2 participants
inside-out 방식으로 진행했습니다 :) 걱정했던 것 보다는 수월하게 구현한 느낌인데
뭔가 잘못 하진 않았을까 걱정이 마구 올라오네요..ㅎㅎ
인수 테스트와 서비스 테스트가 있으니 확실히 안정감이 있게 개발할 수 있음을 느낍니다 :)
마지막 리뷰...!! 부탁드립니다 :)