1단계 - 단위 테스트 작성 #290

zbqmgldjfh · 2022-07-16T03:36:10Z

안녕하세요 리뷰어님! 김지우 라고 합니다!

우선 저의 리뷰를 담당해주셔서 감사하다는 말 먼저 전합니다!!

질문 1가지

1. Section의 생성을 어디서?

예를 들어 다음과 같이 section을 추가하는 메서드가 있다고 해봅시다!

LineService 내부

    @Transactional
    public void addSection(Long lineId, SectionRequest sectionRequest) {
        Station upStation = stationService.findById(sectionRequest.getUpStationId());
        Station downStation = stationService.findById(sectionRequest.getDownStationId());
        Line line = lineRepository.findById(lineId).orElseThrow(IllegalArgumentException::new);
        line.addSection(new Section(line, upStation, downStation, sectionRequest.getDistance()));
    }

이때 addSection의 인자부분에서 Section을 생성하여 전달하는 방식을 사용하고 있습니다.
즉, LineService에서 Section을 생성하고 있습니다.

이와는 달리

line.addSection(line, upStation, downStation, sectionRequest.getDistance());

위와 같이 값만 전달 한 후, line 내부에서 section을 생성하는 방법도 가능할 것 입니다.

저는 개인적으로 전자의 방식이 적합하다 생각하는데... Section의 생성은 어디가 적합할까요?
Service일까요? 아니면 Line과 같은 class 내부 일까요?

- 기존에는 line의 sections를 getter로 가져와서 add시킴 - 변경 후에는 line에게 메시지를 전달하도록 변경

…메서드 리팩토링 완료

pch8388 · 2022-07-16T15:21:13Z

Section의 생성을 어디서?

두 방법 모두 많이 사용하기도 하고, 의견이 분분할 수 있다는 점 미리 말씀드리면서, 제 의견을 말씀드리겠습니다.
개인적으로는 Line 내에서 생성하는 것을 추천드립니다.
Section 이 과연 외부로 노출되어야 할 객체인가를 고민해봐야 한다고 생각합니다.
노선 을 사용할 때, 클라이언트가 노선은 A 구간(X역과 Y역으로 이루어져 있고 거리는 3), B 구간(�Y역과 Z역으로 이루어져 있고 거리는 5) 로 이루어져 있다 라는 세부적인 정보를 알아야 할까요? 아니면 정제된 노선은 X ~(거리3) Y ~(거리5) Z 역으로 이루어져있다 라는 정보만 알아도 될까요? 당연히 정답은 없습니다 😅
다만, 대부분의 경우에는 현재와 같이 Line<Section> 이 외부로 드러나 있으면 부작용이 있을 수 있습니다. (컬렉션을 마음대로 조작)
하지만 Section 이라는 객체를 아예 숨기는 것은 선택적인 문제라고 생각합니다.

다만, 객체 생성의 책임을 지는 곳이 명확하면, 그만큼 변경에 대한 전파가 줄어들고, 리팩토링도 더 쉬울 수 있습니다.
예를 들어 Section 의 생성 로직이 복잡해져서, Factory 클래스를 만들고 싶다거나 하는 경우에도, 해당 Factory 에 의존성을 갖는 클래스가 적은게 더 좋겠죠

개인적인 결론을 말씀드리면 라이프 사이클을 같이 하는 객체는 대표하는 엔티티의 속성 이라고 볼 수도 있기 때문에 내부로 감추는 것이 더 좋다고 생각합니다. 😄

pch8388

지우님 안녕하세요 🙇
이번 미션을 같이 진행하게 된 권승철입니다. 😄
코멘트 참고하셔서 수정 후 리뷰 요청 부탁드립니다 🔥

pch8388 · 2022-07-16T14:17:08Z

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

    public List<Section> getSections() {
        return sections;
    }
+
+    public void addSection(Section section) {


비즈니스 로직에 관련된 메서드가 getter 보다 상위에 위치하는 것이 클래스를 이해하는 데 도움이 된다고 생각합니다 😄

pch8388 · 2022-07-16T14:32:04Z

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

+        Exception exception = assertThrows(IndexOutOfBoundsException.class, () -> lineService.deleteSection(line.getId(), downStation.getId()));
+
+        // then
+        then(exception).isInstanceOf(IndexOutOfBoundsException.class);


이 예외가 적절할까요? 🤔

애당초 IndexOutOfBoundsException는 적절하지 않았던 예외라 생각됩니다 ㅎㅎ
검증 부분을 추가하여 IllegalStateException을 던지도록 수정하였습니다.

추가로 커스텀 예외를 만들까 하다가? 일단 IllegalStateException 을 반환하도록 변경하였습니다.
다음 step2에서 커스텀 예외의 적용을 고민해보도록 하겠습니다!

pch8388 · 2022-07-16T14:37:05Z

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

+        for (Section section : sections) {
+            stations.add(section.getDownStation());
+        }
+        return new ArrayList<>(stations);


이렇게 하신 이유가 있을까요? 🤔

Suggested change

return new ArrayList<>(stations);

return stations;

이게 변명을 하자면?
지난번 과제에서는 일급컬렉션을 구현하여 사용했는데, 이때 일급 컬렉션 내부의 �section들을 가져오기 위해 불변 객체로 만들어 가져왔었습니다.

문제는 이번 과제를 만드는 도중 일급컬렉션이 없음에도 불구하고... 일급컬랙션이 있다고 착각하여 생긴 문제인것 같습니다 ㅎㅎ
바로 stations list를 반환하도록 수정하였습니다!

pch8388 · 2022-07-16T14:39:07Z

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

+
+    public List<Station> getStations() {
+        final List<Station> stations = new ArrayList<>();
+        stations.add(getFirstSection().getUpStation());


첫번째 상행역을 가져오는 메서드를 만드는 건 어떨까요? 💭
추가로 현재 구현으로는 Section이 하나도 없는 경우도 가능할 것 같은 데, 그런 경우에 이 메서드가 호출되면 어떻게 될까요? 🤔

원래 service 계층에서 Sections이 없는 경우 empty list를 반환하도록 했었는데, 이를 Sections 내부로 로직을 이동시켰습니다!

pch8388 · 2022-07-16T14:55:44Z

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

+    }
+
+    public void deleteLastSection(Station station) {
+        Section lastSection = sections.get(getLastIndex());


마지막 구간 을 가져오는 메서드를 만들면 어떨까요? 😃

pch8388 · 2022-07-16T14:56:42Z

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

+    }
+
+    private Section getFirstSection() {
+        return sections.get(0);


클린 코드 G25: 매직 숫자는 명명된 상수로 교체하라

pch8388 · 2022-07-16T14:58:11Z

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

+        given(stationService.findById(downStationId)).willReturn(new Station("건대입구역"));
+        given(lineRepository.findById(anyLong())).willReturn(Optional.of(new Line("2호선", "green")));
+
+        SectionRequest request = new SectionRequest(upStationId, downStationId, 10);

        // when
        // lineService.addSection 호출


불필요해진 주석은 삭제하는 것이 좋은 것 같아요 😅
클린코드의 한 구절입니다.

C3 : 중복된 주석
코드만으로 충분한데 구구절절 설명하는 주석이 중복된 주석이다. ...(중략)... 주석은 코드만으로 다하지 못하는 설명을 부언한다.

pch8388 · 2022-07-16T15:05:12Z

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

+        return new ArrayList<>(stations);
+    }
+
+    public void removeLastSection() {


이 메서드가 있어야 하나요? 🤔

pch8388 · 2022-07-16T15:06:21Z

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

+    }
+
+    private int getLastIndex() {
+        return sections.size() - 1;


sections 를 일급 컬렉션으로 만들어보면 어떨까요?
일급컬렉션

앗... 이전 미션에서 일급컬렉션을 사용하여 Sections를 만들었는데, 여기에도 적용되있다고 잘못 생각한것 같습니다 ㅎㅎ

pch8388 · 2022-07-16T15:15:25Z

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

-    public void setColor(String color) {
-        this.color = color;
-    }
-
    public List<Section> getSections() {


Line<Section> 이 외부로 드러나는 것이 괜찮을까요? 🤔

일급 컬렉션으로 변경 하였으니, 외부에서는 메시지를 전달하여 필요한 정보를 얻도록 수정하였습니다!

- 사실 불변객체로 바꿔줄 필요가 없다. 어짜피 메서드 안에서 생성한 List이기 때문에 그대로 반환해도 된다

pch8388

전체적으로 잘 리팩토링 해주셨습니다 💯
참고하실만한 코멘트를 남겨드렸습니다. 😄
이번 단계는 여기서 마무리하겠습니다. 다음 단계로 가시죠 ~ 🚀

pch8388 · 2022-07-17T02:36:16Z

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

+
+    public boolean dontHasDownStation(Station station) {


개인적으로는 hasNot 이라고 그냥 쓰기도 합니다.
일반적으로 is 나 has prefix 로 시작할 때 boolean 을 반환한다고 예측하기가 쉽기 때문입니다. 😅

pch8388 · 2022-07-17T02:47:41Z

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

+        return sections.getStations();
+    }
+
+    public int getSectionsSize() {


이 메서드는 테스트를 위해 공개된 것 같은 데, 테스트를 위해서 메서드를 열어 두는 것에 대해 고민해보셨을까요? 🤔

pch8388 · 2022-07-17T02:53:50Z

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

+        if (sections.isEmpty()) {
+            return Collections.emptyList();
+        }
+        final List<Station> stations = new ArrayList<>();


Sections 가 비어있다면 emptyList를 반환하고, 아니라면 반환할 list를 만든다라는 부분을 빈 행으로 분리해서 조금 더 가독성을 높일 수 있다고 생각합니다.
클린코드를 인용하겠습니다. 😸

개념은 빈 행으로 분리하라
거의 모든 코드는 왼쪽에서 오른쪽으로 그리고 위에서 아래로 읽힌다. 각 행은 수식이나 절을 나타내고, 일련의 행 묶음은 완결된 생각 하나를 표현한다. 생각 사이는 빈 행을 넣어 분리해야 마땅하다.

확실히 빈 행을 통해, 코드와 코드간에 한숨 쉬어간다? 와 같은 생각이 들어서
가독성이 높아진것 같습니다! 감사합니다

pch8388 · 2022-07-17T02:57:19Z

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

+        for (Section section : sections) {
+            stations.add(section.getDownStation());
+        }
+        return stations;


pch8388 · 2022-07-17T02:59:19Z

src/main/java/nextstep/subway/applicaion/LineService.java

@@ -29,10 +28,10 @@ public LineService(LineRepository lineRepository, StationService stationService)
    @Transactional
    public LineResponse saveLine(LineRequest request) {
        Line line = lineRepository.save(new Line(request.getName(), request.getColor()));
-        if (request.getUpStationId() != null && request.getDownStationId() != null && request.getDistance() != 0) {
+        if (isValidRequest(request)) {


pch8388 · 2022-07-17T03:03:57Z

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

+    }
+
+    public List<Station> getStations() {
+        if (sections.isEmpty()) {


👀 중복이 발생한 것 같아요. 하지만 해당 메서드 이름을 쓰면 getStations 에서는 조금 어색하다고 생각될 수 있겠네요

Suggested change

if (sections.isEmpty()) {

if (isInValidSize()) {

pch8388 · 2022-07-17T03:05:01Z

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

+import java.util.List;
+
+@Embeddable
+public class Sections {


Sections 와 Section 에 대한 단위 테스트를 작성하시지 않으셨네요 😢
다음 단계에서 단위 테스트를 작성해주시기 바랍니다. 🔥

pch8388 · 2022-07-17T03:08:14Z

추가로 깜빡하고 코멘트를 안남긴 부분이 있네요 😢
ControllerExceptionHandler 를 이용하여 예외를 클라이언트에게 전달할 때 어떻게 보여줄 지에 대해서도 고민해주세요 🙇

zbqmgldjfh added 14 commits July 15, 2022 20:47

docs(README.md): 구현 TODO list 작성

67a55ff

test(LineTest): LineTest 작성 완료

ceab35b

test(LineServiceMockTest): Stub을 활용한 LineService 테스트 작성 완료

bddccb2

test(LineServiceTest): Stub없이 LineService 테스트 작성 완료

4f8be71

refactor(LineService): Section을 추가하는 addSection 메서드 리팩토링

49ff33c

- 기존에는 line의 sections를 getter로 가져와서 add시킴 - 변경 후에는 line에게 메시지를 전달하도록 변경

test(LineServiceMockTest): Section을 삭제하는 테스트 추가

284ea4f

refactor(LineService): LineService의 deleteSection로직을 line 도메인 내부로 이동완료

b16269a

refactor(saveLine): saveLine 메서드 리팩토링 완료

59da14a

test(LineServiceMockTest): LineService의 update 테스트 작성

8a9c8d1

refactor(LineService.updateLine): updateLine 메서드 리팩토링 완료

1a3fac3

refactor(LineService.createStationResponses): createStationResponses …

edb4ca2

…메서드 리팩토링 완료

test(LineServiceMockTest): 존재하지 않는 line에 구간삭제 요청하는 테스트 작성

391898b

test(LineServiceMockTest): 하행구간역이 동일하지 않는 경우에 삭제를 요청하는 테스트 작성

39b867c

refactor(LineService): Private 메서드 들을 하단으로 이동

e8533ed

pch8388 requested changes Jul 16, 2022

View reviewed changes

zbqmgldjfh added 11 commits July 17, 2022 08:49

docs(README.md): Step1 PR 수정할 TODO list 작성

5c6f020

feat(Sections): 일급 컬렉션 구현하기 완료

2e016b0

refactor(Line): Getter를 비지니스 메서드보다 아래로 이동

973b966

refactor(Sections.getStations): 불변 객체를 반환하던 부분 수정

51b8e81

- 사실 불변객체로 바꿔줄 필요가 없다. 어짜피 메서드 안에서 생성한 List이기 때문에 그대로 반환해도 된다

refactor(Sections.deleteLastSection): 부정 조건 부분 피하기

fa5a841

refactor(Sections.getFirstSection): 매직넘버 0 대신 상수를 사용하도록 수정

1441d94

refactor(LineServiceMockTest): 구현 완료된 부분의 주석 제거완료

9e86743

refactor(Sections): Sections이 비어있을때 빈 리스트를 반환하는 로직을 Sections 내부로 이동

0170114

refactor(Sections.deleteLastSection): 마지막 section을 가져오는 메서드 구현

7fe7edf

refactor(Line): 불필요 한 isEmpty 메서드 제거

5817106

refactor(Sections): 예외 메시지 추가

63a4e87

pch8388 approved these changes Jul 17, 2022

View reviewed changes

pch8388 merged commit d114eb8 into next-step:zbqmgldjfh Jul 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1단계 - 단위 테스트 작성 #290

1단계 - 단위 테스트 작성 #290

zbqmgldjfh commented Jul 16, 2022

pch8388 commented Jul 16, 2022

pch8388 left a comment

pch8388 Jul 16, 2022

pch8388 Jul 16, 2022

zbqmgldjfh Jul 17, 2022 •

edited

pch8388 Jul 16, 2022

zbqmgldjfh Jul 17, 2022

pch8388 Jul 16, 2022

zbqmgldjfh Jul 17, 2022

pch8388 Jul 16, 2022

pch8388 Jul 16, 2022

pch8388 Jul 16, 2022

pch8388 Jul 16, 2022

pch8388 Jul 16, 2022

zbqmgldjfh Jul 17, 2022

pch8388 Jul 16, 2022

zbqmgldjfh Jul 17, 2022

pch8388 left a comment

pch8388 Jul 17, 2022

pch8388 Jul 17, 2022

pch8388 Jul 17, 2022

zbqmgldjfh Jul 17, 2022

pch8388 Jul 17, 2022

pch8388 Jul 17, 2022

pch8388 Jul 17, 2022

pch8388 Jul 17, 2022

pch8388 commented Jul 17, 2022

1단계 - 단위 테스트 작성 #290

1단계 - 단위 테스트 작성 #290

Conversation

zbqmgldjfh commented Jul 16, 2022

안녕하세요 리뷰어님! 김지우 라고 합니다!

질문 1가지

1. Section의 생성을 어디서?

pch8388 commented Jul 16, 2022

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

zbqmgldjfh Jul 17, 2022 • edited

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 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

pch8388 commented Jul 17, 2022

zbqmgldjfh Jul 17, 2022 •

edited