3단계 - 지하철 구간 제거 리팩터링 #323

zbqmgldjfh · 2022-07-22T02:17:03Z

안녕하세요 리뷰어님!!

이번 리뷰도 잘 부탁드립니다!! 덕분에 많은 배움을 얻을 수 있어 행복한 2주차를 보내고 있습니다!!
감사하다는 말 전합니다 ㅎㅎ!

질문1

이번에 CustomException을 좀더 세분화 하기위해, 기존의 SectionException을 SectionsAdd, SectionsDelete 별로 나눠 CustomException을 구현하게 되었습니다.

그보다 더 세부적인 내용들은 예외 안에서 메시지로 구별하였습니다!

우선 모든 예외가 공통적으로 상속받을 BusinessException을 구현하였습니다.
해당 class 안에는 HttpStatus를 저장 할 수 있습니다.

public class BusinessException extends RuntimeException {

    private final HttpStatus httpStatus;

    public BusinessException(String message, HttpStatus httpStatus) {
        super(message);
        this.httpStatus = httpStatus;
    }

    public HttpStatus getStatus() {
        return httpStatus;
    }
}

이후 이를 상속하는 세부적인 예외들은 다음과 같이 사용하였습니다.

public class SectionsAddException extends BusinessException {

    private static final String ALREADY_BOTH_STATION_REGISTER_EXCEPTION = "상행역과 하행역이 이미 노선에 모두 등록되어 있습니다";
    private static final String NOT_FOUND_BOTH_STATION_EXCEPTION = "상행역과 하행역 모두 찾을 수 없습니다";
    private static final String SECTION_DISTANCE_EXCEPTION = "기존의 구간 길이보다 긴 신규구간을 중간에 추가할 수 없습니다";

    private SectionsAddException(String message, HttpStatus httpStatus) {
        super(message, httpStatus);
    }

    public static SectionsAddException ALREADY_BOTH_STATION_REGISTER_EXCEPTION() {
        return new SectionsAddException(ALREADY_BOTH_STATION_REGISTER_EXCEPTION, HttpStatus.BAD_REQUEST);
    }

    public static SectionsAddException NOT_FOUND_BOTH_STATION_EXCEPTION() {
        return new SectionsAddException(NOT_FOUND_BOTH_STATION_EXCEPTION, HttpStatus.NOT_FOUND);
    }

    public static SectionsAddException SECTION_DISTANCE_EXCEPTION() {
        return new SectionsAddException(SECTION_DISTANCE_EXCEPTION, HttpStatus.BAD_REQUEST);
    }
}

이후 사용할 때는 다음과 같이 사용하였습니다.

    public void deleteSection(Station station) {
        if (isSizeLessThanOne()) {
            throw SectionsDeleteException.CANT_DELETE_LAST_ONE_SECTION_EXCEPTION();
        }

        // 생략...
    }

기존에 말씀해주신 것 처럼 ErrorCode한곳에서 메시지를 다 관리하기는 힘들것 같아, 해당 Exception 내부에서 관리하도록 변경하였습니다.
또한 factory method를 만들어 예외를 좀더 쉽게 사용하려 하였습니다!!

이번 변경된 저의 예외 사용방식에 대한 리뷰어 님의 의견이 궁금합니다!

…제를 해결

zbqmgldjfh · 2022-07-22T06:43:34Z

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

        }

+        if (deleteFirstOrLastStation(station)) return;
+        deleteMiddleStation(station);
+    }


44 행이 의식적으로 메서드로 추출하려 하다보니 위와 같이 되었는데,
사실 if문안에 조건을 검증하는 메서드가 아닌, 커맨드성 메서드 자체가 들어가는 것이 적합한지? 궁금합니다!

코멘트 남겼습니다 😄

zbqmgldjfh · 2022-07-22T11:42:32Z

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

+        sections.add(newSection);
+
+        // upStation -(10)- downStation -(5)- newStation -(9)- newSection2
+        Station newStation2 = new Station(4L, "신규역2");


370번 행과 같은 주석 부분은 추후 삭제하도록 하겠습니다!!

pch8388

미션 잘 수행해주셨습니다 👏
코멘트 확인해주시고 다음 단계 진행해주세요 🚀

pch8388 · 2022-07-22T11:16:19Z

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

+            sections.remove(sameUpStationSection);
+            sections.remove(sameDownStationSection);
+            sections.add(newSection);
+            return;


Suggested change

return;

pch8388 · 2022-07-22T11:20:46Z

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

        }

+        if (deleteFirstOrLastStation(station)) return;


클린코드를 인용하겠습니다

명령과 조회를 분리하라!
함수는 뭔가를 수행하거나 뭔가에 답하거나 둘 중 하나만 해야 한다. 둘 다 하면 안된다. 객체 상태를 변경하거나 아니면 객체 정보를 반환하거나 둘 중 하나다.

조금 코드가 더 길어지기는 했는데, CQRS를 지키도록 변경하였습니다 ㅎㅎ

pch8388 · 2022-07-22T11:26:16Z

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

+        validateSection(section);
+        addSection(section);


리팩토링 💯

pch8388 · 2022-07-22T11:29:30Z

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

        sections.add(newSection2);

        // then
        assertThat(sections.getStations()).containsExactly(newStation, upStation, newStation2, downStation);
    }

+    @Test
+    public void add_section_middle_3() {


@DisplayName 을 추가하지 않으신 이유가 있을까요? 🤔

DisplayName으로 위 테스트를 어떻게 표현해야 할지 잘 모르겠었습니다 ㅠ,ㅠ
상행역 앞에 한번, 상행역 바로 뒤에 2번 역을 추가하는데 이걸 다 그냥 적어주면 될지...

pch8388 · 2022-07-22T11:32:58Z

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

+        Section newSection2 = new Section(3L, line, upStation, newStation2, 9);
+        sections.add(newSection2);
+
+        // newStation -(5)- upStation -(9)- newStation2 -(3)- newStation3 -(??)- downStation


이 주석이 코드를 이해하는 데 도움을 줄까요? 😢

pch8388 · 2022-07-22T11:53:37Z

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

@@ -34,7 +34,7 @@ public void addSection(Section section) {
    }

    public void deleteLastSection(Station station) {


아직도 이 이름이 올바른가요? 🤔

pch8388 · 2022-07-22T11:54:47Z

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

-        // then
-        assertThat(line.isEmptySections()).isTrue();
-    }
-
    @DisplayName("Line에 section이 없는데 section을 제거하려 한 경우 예외를 발생한다")
    @Test
    void remove_section_when_no_section() {


정상적인 동작에 대한 검증은 필요없을까요? 😅

음 사실 Line의 로직은 다음과 같이 Sections에게 위임을 할 뿐이라

public void deleteLastSection(Station station) { sections.deleteSection(station); }

LineTest에서 성공적인 제거 검증을 추가로 할 필요는 없을것 같습니다?
이미 SectionsTest에서 검증이 되었다고 생각됩니다!

pch8388 · 2022-07-22T12:04:12Z

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

+    private void deleteMiddleStation(Station station) {
+        Section sameUpStationSection = findSameUpStationSection(station);
+        Section sameDownStationSection = findSameDownStationSection(station);
+        if (sameUpStationSection != null && sameDownStationSection != null) {


early return 을 고려해보셔도 좋을 것 같아요 😄
참고

pch8388 · 2022-07-22T12:11:45Z

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

+        return sections.stream()
+                .filter(s -> s.hasSameUpStation(section.getUpStation()) || s.hasSameDownStation(section.getDownStation()))
+                .findFirst()
+                .orElseThrow(() -> SectionsDeleteException.NOT_FOUND_LAST_SECTION_EXCEPTION());


아래와 같은 스타일은 어떻게 생각하세요? 😄

Suggested change

.orElseThrow(() -> SectionsDeleteException.NOT_FOUND_LAST_SECTION_EXCEPTION());

.orElseThrow(SectionsDeleteException::NOT_FOUND_LAST_SECTION_EXCEPTION);

pch8388 · 2022-07-22T12:19:43Z

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

-    private int getLastIndex() {
-        return sections.size() - 1;
+    private Section getLastSection() {
+        Station lastDownStation = getLastDownStation();


재귀함수는 항상 위험성을 동반하기 때문에 조심히 써야 한다고 생각합니다.
타인의 관점에서 보았을 때, 재귀함수의 위험 가능성을 충분히 인지하고 사용할 수 있을 지 고민해봐야 할 것 같습니다. 😅

pch8388 · 2022-07-22T12:56:22Z

질문 주신 예외처리에 대해 제 개인적인 의견은 여기서 추가로 말씀드리겠습니다
예외를 나타내는 클래스가 http status를 상태로 갖는 게 괜찮을까요? 🤔
사용하신 HttpStatus 클래스는 스프링프레임워크에 포함된 클래스인데 의존성을 만드는 것에 대해 고민해보아야 할 것 같습니다.
또한, 비즈니스 로직에 대한 예외처리인데, http 라는 프로토콜에 의존하게 된다고 생각합니다.
가장 중요한 도메인에 관련한 클래스들은 외부 기술에 종속되지 않도록 구현하는 것을 추천드립니다. 😄

예외 클래스를 계층적으로 구현하신 것은 매우 기발하다고 생각합니다 👍 👍
다만 클래스가 매우 커질 우려가 있기 때문에 어느 부분에서 분리를 할지 고민이 필요하다고 생각합니다.
개인적으로는 개별 클래스로 만들고 패키지 레벨로 관리하는 방법도 좋다고 생각합니다 😄
사용하신 예외처리 방법도 매우 흥미롭게 보았고 공부가 되었습니다 👏

zbqmgldjfh · 2022-07-24T08:09:06Z

예외 처리에 대한 자세한 의견을 남겨주셔서 감사합니다!!

일단 4단계 기능을 구현한 후, 예외처리를 다시 리팩토링 해보겠습니다!

zbqmgldjfh added 24 commits July 21, 2022 08:39

docs(README.md): Step2 PR 추가 수정 TODO List 작성

b380cb4

refactor(Sections.add): add메서드 리팩토링 하기

cf8bf52

fix(Sections.findTargetSection): 메서드 내부에서 인자로 받은 section을 사용하고있지 않던 문…

f9bd74d

…제를 해결

test(SectionsTest): 구간 앞에 연속적으로 추가할때 발생하던 StackOverFlow문제 해결

148687a

test(SectionsTest): 중간에 삽입할 떄 십규 구간의 거리가 기존 구간보다 길다면 예외를 던지는 테스트 작성

c74085b

refactor: 기존의 예외를 좀더 세분화 하도록 변경

de9b780

refactor(LineSectionAcceptanceTest): 인수테스트에서 주석으로된 인수 조건 삭제하기

475fc7d

docs(README.md): Step3 TODO list 작성

9991d6f

docs(README.md): 인수 조건 도출 완료

8784d7b

test: 상행 종점역 삭제 인수테스트 작성

d1ec2bb

feat(Sections.deleteSection): 상행역을 삭제하는 기능 구현

21f8d1c

refactor(Sections.deleteSection): 상행구간 삭제 부분 리팩터링하기

9597ec6

test: 하행 종점역 삭제 인수테스트 작성

3d731b1

feat(Sections.deleteSection): 하행역을 삭제하는 기능 구현

0a65be3

refactor(Sections.deleteSection): 하행구간 삭제 부분 리팩터링하기

81412c4

test: 중간역 삭제 인수테스트 작성

6876fc4

feat(Sections): 중간역 삭제 기능 구현

4f4ce48

refactor(Sections): 중간역 삭제 기능 리팩터링하기

0485e03

test: 구간이 하나 남은경우 삭제할 수 없는 인수테스트 작성

80bf03b

feat(Sections): 구간이 하나 남은경우 삭제시 예외를 던지도록 구현

67415b2

test: 등록되지 않은 역은 삭제할 수 없는 인수테스트 작성

7e0df90

feat(Sections): 등록되지 않은 역은 삭제할 수 없도록 구현

08a57d5

refactor(Sections): 등록되지 않은 역은 삭제부분 리팩토링

abfef86

refactor(Section): 두 Section을 합치는 메서드를 Section 내부로 이동

846c21c

zbqmgldjfh commented Jul 22, 2022

View reviewed changes

pch8388 approved these changes Jul 22, 2022

View reviewed changes

pch8388 merged commit 6d6f4c0 into next-step:zbqmgldjfh Jul 22, 2022

zbqmgldjfh mentioned this pull request Jul 25, 2022

4단계 - 경로 조회 기능 #357

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3단계 - 지하철 구간 제거 리팩터링 #323

3단계 - 지하철 구간 제거 리팩터링 #323

zbqmgldjfh commented Jul 22, 2022 •

edited

zbqmgldjfh Jul 22, 2022

pch8388 Jul 22, 2022

zbqmgldjfh Jul 22, 2022 •

edited

pch8388 left a comment

pch8388 Jul 22, 2022

pch8388 Jul 22, 2022

zbqmgldjfh Jul 24, 2022

pch8388 Jul 22, 2022

pch8388 Jul 22, 2022

zbqmgldjfh Jul 24, 2022

pch8388 Jul 22, 2022

pch8388 Jul 22, 2022

pch8388 Jul 22, 2022

zbqmgldjfh Jul 24, 2022

pch8388 Jul 22, 2022

pch8388 Jul 22, 2022

pch8388 Jul 22, 2022

pch8388 commented Jul 22, 2022

zbqmgldjfh commented Jul 24, 2022

		@@ -34,7 +34,7 @@ public void addSection(Section section) {
		}

		public void deleteLastSection(Station station) {

	.orElseThrow(() -> SectionsDeleteException.NOT_FOUND_LAST_SECTION_EXCEPTION());
	.orElseThrow(SectionsDeleteException::NOT_FOUND_LAST_SECTION_EXCEPTION);

3단계 - 지하철 구간 제거 리팩터링 #323

3단계 - 지하철 구간 제거 리팩터링 #323

Conversation

zbqmgldjfh commented Jul 22, 2022 • edited

안녕하세요 리뷰어님!!

질문1

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbqmgldjfh Jul 22, 2022 • edited

Choose a reason for hiding this comment

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

pch8388 commented Jul 22, 2022

zbqmgldjfh commented Jul 24, 2022

zbqmgldjfh commented Jul 22, 2022 •

edited

zbqmgldjfh Jul 22, 2022 •

edited