[Step1] 🚀 1단계 - 지하철 구간 추가 기능 개선 #681

oh980225 · 2023-08-03T11:42:17Z

작업

요구사항에 추가된 구간 추가 제약사항 변경에 대한 내용을 적용하였습니다.
변경된 스펙 - 예외 케이스 에 대해 커버하도록 적용하였습니다.

기타

실제 서비스를 만들기보다 ATDD 를 경험하는 것이 우선이니, ControllerAdvice, Custom Exception 등은 편의상 생략하였습니다.
entity를 그대로 값 객체로 사용하다보니, 테스트시 id 관련하여 불편한 부분이 여럿 발생하네요..😂 우선 mock을 최대한 활용하여 테스트를 작성하였습니다.
개인적으로 entity와 별도로 도메인용 객체를 따로 만들고 싶지만... 작업 범위가 너무 넓어질 것 같고, 시간이 모자라서 우선 진행하였습니다..😂
인수테스트로는 커다랗고 간단한 시나리오만 테스트하고, 그에 대한 세부 요구사항은 단위 테스트로 작성해보았습니다.
감사합니다 🙏🏻

구간 추가에 대한 요구사항이 변경 및 추가되었습니다. 그에 따라서 불필요해진 테스트를 우선 주석 처리하고, 새로 작성해야하는 인수 테스트의 인수 조건을 도출하였습니다.

구간 추가시 기존 노선의 구간의 길이와 비교하는 요구사항이 추가되었습니다. 이를 검증하기 위해, 테스트를 먼저 작성하고 구현을 완성하였습니다. 기존에 작성해놓은 모든 테스트가 통과하는 것을 확인했습니다.

구간 추가에 대해 새로운 요구사항이 추가되었습니다. 그에 대한 새로운 테스트 케이스를 먼저 정의하여 추가하였습니다.

구간 추가시, 추가하려는 구간의 상행역, 하행역이 이미 모두 노선 내 구간에 등록되어 있는지 확인하는 로직과, 모두 노선 내 없는지 확인하는 로직을 구현하고 모두 테스트를 통과하는지 확인하였습니다.

역을 조회시, 이제 중간에도 구간을 삽입할 수 있기에 로직 수정이 필요했습니다. 첫번째 역id, 마지막 역id 를 저장하여 순차적으로 역을 가져올 수 있도록 수정하였습니다. 테스트가 통과하는 것도 확인하였습니다.

구간 추가시 firstStationId, lastStationId를 수정해야하는 케이스가 존재합니다. 이를 테스트하기 위해 테스트 코드를 작성하였습니다.

구간 추가시에 대한 요구사항이 수정되었습니다. 단순한 추가 시나리오에 대해서는 인수테스트를 작성하였고, 좀 더 세부적인 요구사항에 대한 케이스는 단위테스트로 작성하였습니다. 모든 테스트가 통과하는 것을 확인했습니다.

pch8388

안녕하세요
1단계 전체적으로 잘 수행해주셨는데, 수정하고 넘어갔으면 하는 부분이 있습니다 😄
코멘트 확인 후 수정하셔서 리뷰요청 해주세요 🔥

pch8388 · 2023-08-04T10:09:05Z

src/main/java/nextstep/subway/section/repository/Sections.java

@@ -1,34 +1,53 @@
 package nextstep.subway.section.repository;


의도는 이해가 가지만 현재 상황에서 repository 라는 패키지 분류는 오해를 부르기 쉬울 것 같아요 🤔
현재 사용하신 것은 domain 이나 entity 와 같은 패키지 이름이 더 어울릴것 같습니다

넵넵, entity 패키지에 별도로 넣어두겠습니다!

pch8388 · 2023-08-04T10:11:01Z

src/main/java/nextstep/subway/line/service/LineService.java

@@ -21,6 +22,7 @@
 class LineService implements LineFindable, LineSavable, LineUpdatable, LineDeletable {


어떤 장점이 있어서 인터페이스를 implements 하신걸까요? 🤔

사용하는 클라이언트 입장에서, 필요한 부분만 인터페이스로 사용할 수 있도록하는 것이 의도였습니다.
또한 필요한 기능만 인터페이스로 의존할 수 있기에 테스트시 fake 객체를 만들거나 할 때에도 테스트 코드를 작성할 때 더 용이하지 않을까 싶은 생각으로 이렇게 구현하였습니다.

pch8388 · 2023-08-04T10:17:47Z

src/main/java/nextstep/subway/section/policy/add/IntegrationAddSectionPolicy.java

+import java.util.List;
+
+@RequiredArgsConstructor
+public class IntegrationAddSectionPolicy implements AddSectionPolicy {


흥미로운 구조 👍

다만, IntegrationAddSectionPolicy 가 꼭 AddSectionPolicy 를 구현해야만 했나? 에 대해서는 의문이 듭니다.
오히려 구조가 너무 복잡해진 것 같아요 😢
동료가 유지보수 할 때, 직관적일지 고민해보셨을까요? 🤔

저는 오히려 AddSectionPolicy 인터페이스를 구현함으로써 언제든 그 정책을 바꿀 수 있고, 테스트가 더 용이해졌다고 생각하였습니다.

또한 유지보수 측면에서도, 정책이 많아지고 복잡해져도, 각 정책이 섞일 일 없이 각자의 정책 객체로써 존재하게 될 것이고, 각 객체별로 테스트되고 있을 것이기에 괜찮지 않을까라고 생각하였습니다..! 만약 정책이 지금은 3개뿐이지만, 10개가 된다면? 그 이상이 된다면? 하나의 클래스가 그 정책을 다 담고 있는 것이 과연 단일 책임을 지킨 것인가? 그 코드는 유지보수하기 쉽고 이해하기 편한가? 오히려 모든 로직을 다 봐야 그제서야 이해할 수 있지 않을까? 라고 생각했던 것 같습니다.

물론 구조가 인터페이스를 두고 각 구현체들이 생기고 주입되는 과정이 생겨 더 복잡하고 오버엔지니어링이라고 볼수도 있을 것 같긴 합니다..! 😓 다만 정말 정책이 많았다면? 이와 반대로 그냥 모든 정책이 도메인 클래스 또는 하나의 클래스에 몰려있었다면? 과연 유지보수하기 쉬웠을까, 그 클래스의 테스트 코드는 읽기 쉬웠을까, 그 클래스를 주입받는 다른 클래스는 테스트하기 쉬웠을까를 생각했을 때는 잘 모르겠네요.. 물론 지금은 정책이 몇개 없긴합니다.. 🤣🤣

pch8388 · 2023-08-04T10:19:05Z

src/main/java/nextstep/subway/section/policy/add/CheckRegisterPolicy.java

+        Optional<Station> upStation = sections.getAllStation().stream()
+                .filter(station -> Objects.equals(station, section.getUpStation()))
+                .findFirst();


중복되는 부분은 메서드 추출하는 게 어떨까요? 😄

pch8388 · 2023-08-04T10:30:15Z

src/main/java/nextstep/subway/section/policy/add/CheckDistancePolicy.java

+        if ((sectionEqualUpStation.isPresent() && (sectionEqualUpStation.get().getDistance() <= section.getDistance()))
+                || (sectionEqualDownStation.isPresent() && (sectionEqualDownStation.get().getDistance() <= section.getDistance()))) {


조금 더 가독성 있게 바꿔볼 수 있을 것 같아요 😅

pch8388 · 2023-08-04T10:52:24Z

src/main/java/nextstep/subway/section/repository/Sections.java

+                .findFirst();
+
+        if (upStation.isPresent() && upStation.get().getId().equals(lastStationId)) {
+            lastStationId = section.getDownStation().getId();


디미터법칙을 보고 개선할 방법을 찾아보시면 좋을 것 같아요 😄

pch8388 · 2023-08-04T10:57:59Z

src/main/java/nextstep/subway/section/repository/Sections.java

+            for (Section section : sections) {
+                if (Objects.equals(section.getUpStation().getId(), targetStationId)) {


객체지향생활체조

규칙 1: 한 메서드에 오직 한 단계의 들여쓰기(indent)만 한다.

pch8388 · 2023-08-04T10:59:08Z

src/main/java/nextstep/subway/section/repository/Sections.java

    }

    public Sections(List<Section> sections) {
        if (sections.isEmpty()) {
            throw new RuntimeException("sections: at least one section is required");
        }
        this.sections = sections;
+        this.firstStationId = sections.get(0).getUpStation().getId();


생성자 파라미터를 List 로 받지 않아도 될 것 같아요 😢

pch8388 · 2023-08-04T10:59:35Z

src/main/java/nextstep/subway/section/repository/Sections.java

+            }
+        }
+
+        return Collections.unmodifiableList(result);
    }

    public Sections(List<Section> sections) {


일반적으로 생성자는 메서드들중 클래스의 가장 상단에 위치할 것을 기대합니다

pch8388 · 2023-08-04T11:01:18Z

src/test/java/nextstep/subway/unit/LineServiceTest.java

+            // nothing
+        }
+    };
+    private final LineService lineService = new LineService(lineRepository, stationFindable, addSectionPolicy);


Suggested change

private final LineService lineService = new LineService(lineRepository, stationFindable, addSectionPolicy);

private final LineService lineService = new LineService(lineRepository, stationFindable, (sections, section) -> {});

AddSectionPolicy 요소를 따로 검증했기 때문에 검증이 필요없다고 생각하신 것 같은데, 위와 같이 간단하게 할수도 있습니다 😅

oh980225 added 7 commits July 30, 2023 22:09

test: 구간 추가에 대해 추가된 요구사항의 테스트 작성(실패 상태)

9e57e9f

구간 추가에 대해 새로운 요구사항이 추가되었습니다. 그에 대한 새로운 테스트 케이스를 먼저 정의하여 추가하였습니다.

test: 구간추가 관련 테스트 추가(실패 상태)

15425d0

구간 추가시 firstStationId, lastStationId를 수정해야하는 케이스가 존재합니다. 이를 테스트하기 위해 테스트 코드를 작성하였습니다.

pch8388 requested changes Aug 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Step1] 🚀 1단계 - 지하철 구간 추가 기능 개선 #681

[Step1] 🚀 1단계 - 지하철 구간 추가 기능 개선 #681

oh980225 commented Aug 3, 2023

pch8388 left a comment

pch8388 Aug 4, 2023

oh980225 Aug 7, 2023

pch8388 Aug 4, 2023

oh980225 Aug 7, 2023

pch8388 Aug 4, 2023

oh980225 Aug 7, 2023

pch8388 Aug 4, 2023

pch8388 Aug 4, 2023

pch8388 Aug 4, 2023

pch8388 Aug 4, 2023

pch8388 Aug 4, 2023

pch8388 Aug 4, 2023

pch8388 Aug 4, 2023

		@@ -1,34 +1,53 @@
		package nextstep.subway.section.repository;

		@@ -21,6 +22,7 @@
		class LineService implements LineFindable, LineSavable, LineUpdatable, LineDeletable {

		if ((sectionEqualUpStation.isPresent() && (sectionEqualUpStation.get().getDistance() <= section.getDistance()))
		\|\| (sectionEqualDownStation.isPresent() && (sectionEqualDownStation.get().getDistance() <= section.getDistance()))) {

		for (Section section : sections) {
		if (Objects.equals(section.getUpStation().getId(), targetStationId)) {

	private final LineService lineService = new LineService(lineRepository, stationFindable, addSectionPolicy);
	private final LineService lineService = new LineService(lineRepository, stationFindable, (sections, section) -> {});

[Step1] 🚀 1단계 - 지하철 구간 추가 기능 개선 #681

Are you sure you want to change the base?

[Step1] 🚀 1단계 - 지하철 구간 추가 기능 개선 #681

Conversation

oh980225 commented Aug 3, 2023

작업

기타

pch8388 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment