🚀 2단계 - 요금 조회 #399

kimj2su · 2023-08-14T14:15:04Z

안녕하세요 리뷰어님 ! 미션 수행하는데 너무 오래걸렸습니다..죄송합니다😢
1단계 미션에서 조건이 추가될때마다 분기문이 추가될 수 밖에 없을 것 같아는 말씀을 해주셨는데 이것에 대해서 찾아보고 전략패턴, 스프링의 DI 등 해결방법(?)도 찾아 봤지만 type이 여러개라면 결국엔 어디선가 분기 처리를 해야 될 것 같다는 생각이 들고 적용하려고 하는데 쉽지가 않네요..
OCP 원칙과 DIP원칙을 잘 지켰다는 예제나 참고할만한 공부 자료를 알고 계시면 알려주시면 감사하겠습니다 ㅜㅜ

catsbi

지수님 슬랙 알람이 누락되었는지 안울려서 리뷰가 많이 늦었네요 죄송합니다 😥
2단계 요금조회 미션 잘 구현해주셨어요. 테스트쪽은 주석이나 케이스 모두 꼼꼼하게 잘 작성된것 같아 크게 피드백 드릴 부분이 없는데, 프로덕션 코드에서 아키텍처적으로 좀 고민해보면 좋을 것 같아 피드백 남겨드렸습니다.

OCP나 DIP관련되서는 음 개인적으로 클린아키텍처 책을 추천드려요.
아니면 해당 포스팅을 참고해보셔도 좋을 것 같습니다.
https://catsbi.oopy.io/ded9db4f-6ed6-48c7-9d46-8a0d88b1b6d0

강의로는 김영한님의 스프링 강의중에서 SOLID원칙으로 스프링의 탄생배경에 대해 설명하는 부분도 있는데 이를 참고해보셔도 좋을 것 같네요 ㅎㅎ

피드백 확인 후 다시 리뷰요청 부탁드립니다!

catsbi · 2023-08-16T03:04:59Z

src/main/java/nextstep/subway/domain/Fare.java

+    private final int fare;
+
+    public Fare(int distance) {
+        this.fare = generateFare(distance);


생성자에 초기화 비즈니스로직이 들어가는 것 보다는 말 그대로 생성에 필요한 최소한의 동작(값 할당, 유효성 검증 정도)만 해주시는걸 권장드려요.

개발자 입장에서 생성자 호출시 추가적인 비즈니스 로직이 생성될 것이라고 생각하기보단 단순하게 인스턴스 생성이 될 것 정도만 유추를 할 수 있죠. 그래서 이런 로직들이 추가될 경우를 고려해 정적팩토리 메서드를 사용하거나
별도의 init메서드를 명시적으로 호출하도록 하고 있습니다!

catsbi · 2023-08-16T03:06:47Z

src/main/java/nextstep/subway/domain/Fare.java

+    }
+
+    // 맨처음 55 -> 파라미터로 45 넘어옴
+    private int calculateFare(int distance) {


요금 일급 객체인데 자기자신에 대한 요금 정책을 거리별로 어떻게 가져갈지까지 책임을 지고 있어요. 이게 적절한 책임 일까요? SRP원칙에 위배될 것 같네요! 해당 객체는 요금에 대해서 간단한 연산정도를 책임지는 것이지 거리까지 알고 계산할 책임은 없을 것 같아요!

다른 위치로 메서드가 옮겨졌다고 가정할 경우, 이번에는 매번 요금 정책이 추가될때마다 메서드가 추가되야 할까요? 추상화를 통해 해당 요금계산 로직이 거리가 몇일때 어떤 메서드를 호출해야 할지 몰라도 되도록 할 수 있지 않을까요?

catsbi · 2023-08-16T03:11:28Z

src/main/java/nextstep/subway/constant/FindPathType.java

+        for (FindPathType findPathType : FindPathType.values()) {
+            if (findPathType.equals(type)) {
+                return findPathType;
+            }
+        }
+        return null;


명시적으로 null을 반환하는건 이 메서드를 호출하는 입장에서 항상 NPE 위험에 대해 대응을 해야하기때문에 권장되지 않아요 차라리 NONE타입같은 별도의 타입을 추가하거나 Optional을 반환하는게 낫겠죠

그리고 매개변수가 이미 FindPathType인데 findPathType을 하는 이유가 있을까요? 해당 타입이려면 해당 열거 타입에 있는 인스턴스(DISTANCE, DURATION)중 하나여야 할텐데요. null이 될 수 있을까요?

catsbi · 2023-08-16T03:17:50Z

src/main/java/nextstep/subway/constant/FindPathType.java

    DISTANCE("DISTANCE", "거리"),
    DURATION("DURATION", "시간");


출력을 어떻게 할지에 대해서(시간, 거리)는 출력 계층에서 알아서 책임져도 될 것 같네요!

catsbi · 2023-08-16T03:18:57Z

src/main/java/nextstep/subway/constant/FindPathType.java

+        if (findPathType(type) == DISTANCE) {
+            return new EdgeWeightDistance();
+        }
+        return new EdgeWeightDuration();


차라리 해당 간선정보 객체를 인스턴스 (DISTANCE, DURATION)의 프로퍼티로 잡거나 람다식으로 생성 제공하도록 해주면 if문으로 직접 고르지 않아도 되지 않을까요?

kimj2su · 2023-08-17T14:16:56Z

안녕하세요 리뷰어님!
리뷰 주신 코멘트 위주로 최대한 반영해보았습니다!!
이번 리뷰를 통해 한번 더 생각하게 되고 공부 방향성을 또 하나 잡은 것 같아서 감사드립니다!..

kimj2su added 5 commits August 9, 2023 17:01

1단계 코멘트 반영

329b730

인수 조건 정의 및 문서화 작성

09e7218

요금 도메인 생성 및 단위 테스트 작성

138304e

요금 계산 방법 수정

69c7628

요금 계산 인수 테스트 작성

61d1b68

catsbi requested changes Aug 16, 2023

View reviewed changes

2단계 리뷰 반영

6103db5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚀 2단계 - 요금 조회 #399

🚀 2단계 - 요금 조회 #399

kimj2su commented Aug 14, 2023

catsbi left a comment

catsbi Aug 16, 2023

catsbi Aug 16, 2023

catsbi Aug 16, 2023

catsbi Aug 16, 2023

catsbi Aug 16, 2023

kimj2su commented Aug 17, 2023

		DISTANCE("DISTANCE", "거리"),
		DURATION("DURATION", "시간");

🚀 2단계 - 요금 조회 #399

Are you sure you want to change the base?

🚀 2단계 - 요금 조회 #399

Conversation

kimj2su commented Aug 14, 2023

catsbi left a comment

Choose a reason for hiding this comment

catsbi Aug 16, 2023

Choose a reason for hiding this comment

catsbi Aug 16, 2023

Choose a reason for hiding this comment

catsbi Aug 16, 2023

Choose a reason for hiding this comment

catsbi Aug 16, 2023

Choose a reason for hiding this comment

catsbi Aug 16, 2023

Choose a reason for hiding this comment

kimj2su commented Aug 17, 2023