-
Notifications
You must be signed in to change notification settings - Fork 204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
3단계 - 요금 정책 추가 #218
base: artium59
Are you sure you want to change the base?
3단계 - 요금 정책 추가 #218
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 😃
3단계 미션 잘 수행해 주셨네요 👍
몇 가지 코멘트로 남겨두었으니 확인해 주시고 리뷰 요청 부탁드립니다.
궁금한 내용이 있으시면 편하게 DM 주세요!
private int calculateByAge(int age, int price) { | ||
if (age < 13 && age >= 6) { | ||
return (price - 350) / 10 * 5; | ||
} else if (age < 19 && age >= 13) { | ||
return (price - 350) / 10 * 8; | ||
} | ||
return price; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석 없이는 쉽게 이해하기 힘든 코드라고 생각드네요 😢
(객체지향 생활 체조 규칙 2: else 예약어를 쓰지 않는다.
)
할인 정책이 추가되거나 변경되면 조건문이 무한히 늘어날 수 을 것 같아요
정책을 효과적으로 관리할 수 있는 방법은 어떤게 있을까요?
this.lineService = lineService; | ||
this.stationService = stationService; | ||
this.pathResponseBuilder = pathResponseBuilder; | ||
} | ||
|
||
public PathResponse findPath(Long source, Long target, SearchType searchType) { | ||
public PathResponse findPath(String email, Long source, Long target, SearchType searchType) { | ||
int age = memberService.findMember(email).getAge(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
회원가입 되어 있는 회원이 로그인 상태가 아니라면 이 서비스는 이용을 못할 것 같네요
요구사항에 있듯이 로그인 사용자의 경우 연령별 요금 계산을 적용하고
비회원(로그인하지 않은 상태)에서는 연령별 할인 정책이 적용되지 않은 상태에서 경로를 조회할 수 있으면 좋을 것 같아요
import nextstep.subway.constant.SearchType; | ||
import org.springframework.http.MediaType; | ||
|
||
public class PathSteps extends AcceptanceTestSteps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PathSteps를 분리해 주셨네요 👍
@ParameterizedTest(name = "{index}: {3}: {0}km 이용 = {2}원") | ||
@MethodSource("이용_거리_0km") | ||
void 이용_거리가_0이면_요금도_0원이다(int distance, int age, int price, String message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message 파라미터를 테스트의 이름으로 활용하시네요 👍
private static int 연령별_요금(int age, int price) { | ||
if (age < 13 && age >= 6) { | ||
return (price - 350) / 10 * 5; | ||
} else if (age < 19 && age >= 13) { | ||
return (price - 350) / 10 * 8; | ||
} | ||
return price; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
프로덕션 코드와 동일한 코드 혹은 테스트 코드내에서 비즈니스 로직 코드가 있다면
관리포인트도 늘어나고 유지보수면에서 쉽지 않을 일이 될 것 같은데요
이 메서드가 테스트 코드 내에서 꼭 필요할까요?
Arguments.of(51, 6, 연령별_요금(6, 이용거리_51km_요금), "어린이"), | ||
Arguments.of(51, 16, 연령별_요금(16, 이용거리_51km_요금), "청소년"), | ||
Arguments.of(51, 26, 연령별_요금(26, 이용거리_51km_요금), "성인"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반복되는 매직넘버, 매직 리터럴은 상수화 해도 좋을 것 같아요 😃
삼호선 = 지하철_노선_생성_요청("3호선", "orange", 교대역, 남부터미널역, 2, 10); | ||
이호선 = 지하철_노선_생성_요청("2호선", "green", 교대역, 강남역, 10, 4, 200); | ||
신분당선 = 지하철_노선_생성_요청("신분당선", "red", 강남역, 양재역, 10, 4, 1000); | ||
삼호선 = 지하철_노선_생성_요청("3호선", "orange", 교대역, 남부터미널역, 2, 10, 300); | ||
|
||
지하철_노선에_지하철_구간_생성_요청(관리자, 삼호선, createSectionCreateParams(남부터미널역, 양재역, 3, 1)); | ||
} | ||
|
||
@DisplayName("두 역의 최단 거리 경로를 조회한다.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
테스트 내용이 변경됨에 따라 DisplayName도 변경하면 어떨까요?
경로와 시간, 요금까지 모두 조회할 수 있다는 표현이 포함되면 좋을 것 같아요
Summary
추가된 요금 정책
Comment
@ParameterizedTest
가 있는 것이 더 깔끔해져서 쓰게 되었습니다.