🚀 3단계 - 지하철 구간 관리 by janeljs · Pull Request #970 · next-step/atdd-subway-map

janeljs · 2023-07-29T12:55:13Z

송용주 리뷰어님 안녕하세요~
리뷰이 임지선입니다.

3단계 - 지하철 구간 관리 리뷰 요청드립니다.

한 동안 바빠서 아예 미션을 못하다가 늦게나마 조금씩 해보려고 PR 보냅니다 😭
저번에 답변주신 코멘트들 도움이 많이 되었습니다.
이번 단계도 리뷰 잘 부탁드리겠습니다~! 😄

testrace

안녕하세요 😃
3단계 깔끔하게 잘 구현해 주셨네요 👍
몇 가지 코멘트 남겼는데 확인해 주시고 다시 리뷰 요청해 주세요

testrace · 2023-07-29T23:46:30Z

    testImplementation 'io.rest-assured:rest-assured:4.5.1'
    testImplementation 'com.navercorp.fixturemonkey:fixture-monkey-starter:0.5.8'
    testImplementation 'com.google.guava:guava:16+'
+    testImplementation 'org.mockito:mockito-core'


사용하지 않는 의존성이 추가되었네요 👀

삭제하였습니다~! 348513e

testrace · 2023-07-29T23:47:24Z

+import java.util.List;
+import java.util.stream.Stream;
+
+public class Sections {


일급 컬렉션 활용 👍

testrace · 2023-07-29T23:51:33Z

+    public void validateUpStationId(Line line) {
+        if (!Objects.equals(upStationId, line.getTerminalStationId())) {
+            throw new InvalidSectionRequestException("해당 노선의 하행종점역이 아닌 역이 상행역으로 설정되었습니다.",
+                    Map.of(
+                            "lineId", line.getId().toString(),
+                            "upStationId", upStationId.toString(),
+                            "downStationId", downStationId.toString()
+                    ));
+        }
+    }


도메인 로직이 DTO에 포함된 것 같아요
CreateSectionRequest를 활용하지 않고 구간을 추가한다면 구간 추가 제약을 무시할 수 있게 되는데요
구간을 추가할 때 Sectios 내부에서 검증이 이뤄져야하지 않을까요?

헛 맞네요ㅠㅠ 리뷰 반영했습니다~! 덕분에 코드가 훨씬 깔끔해졌네요 😄 63c094a

testrace · 2023-07-29T23:56:35Z

+        if (!line.getTerminalStationId().equals(stationId)) {
+            throw new InvalidSectionRequestException("마지막 구간만 삭제할 수 있습니다.");
+        }


getter를 활용해 비교하기 보다 line에게 메시지를 보내면 어떨까요?

리뷰 반영했습니다ㅎㅎ 63c094a

testrace · 2023-07-29T23:57:17Z

-public class StationResponse {
-    private final Long id;
-    private final String name;
+public record StationResponse(Long id, String name) {


다른 DTO와 다르게 record를 사용하신 이유가 궁금합니다 🤔

이거 인텔리제이가 추천해주길래 호기심에 한 번 써봤습니다 😆

testrace · 2023-07-29T23:58:35Z

+    List<Section> findAllByLine_Id(Long lineId);
+
+    Optional<Section> findByLine_IdAndDownStation_Id(Long lineId, Long downStationId);


메서드명 컨벤션이 맞지 않은 것 같아요 😢

camelCase가 아니기는 한데 자동완성 추천 이름에 _가 있고, 가독성이 조금 좋아지는 측면도 있는 것 같아서 그대로 사용했습니다.
제가 실무에서는 주로 마이바티스를 사용하여 잘 모르는데 보통은 camelCase를 사용하나요?
(querydsl 메서드나 다른 메서드와 통일성을 생각하면 그게 맞는 거 같기도 하네요ㅎㅎ)

Optional<Section> findByLineIdAndDownStationId(Long lineId, Long downStationId);처럼 고치니까 Unable to locate Attribute with the the given name [downStationId] on this ManagedType [subway.common.BaseEntity] 에러가 발생하는 문제도 있네요. _가 없으면 Station downStation 내부의 id로 인식을 못하는 것 같아요. 😢

testrace · 2023-07-30T00:03:02Z

+        Section section = Section.builder()
+                .line(line)
+                .upStation(upStation)
+                .downStation(downStation)
+                .distance(createSectionRequest.getDistance())
+                .build();
+
+        Section savedSection = sectionRepository.save(section);


Line#addSection 를 활용하면 어떨까요?

리뷰 반영했습니다~ 63c094a

testrace · 2023-07-30T00:06:28Z

+            assertThat(getResponse.jsonPath().getList("upStationId", Long.class)).containsAnyOf(신분당선_하행종점역_ID);
+            assertThat(getResponse.jsonPath().getList("downStationId", Long.class)).containsAnyOf(신분당선_신규역_ID);


신규 하행 종점역을 추가한 경우 신규역이 구간에 포함되어 있다는 것보다 하행 종점역임을 검증하면 어떨까요?
containsExactly 를 활용해서 역 목록 순서를 검증하면 좋을 것 같은데 어떻게 생각하시나요?

assertThat(getResponse.jsonPath().getList("upStationId", Long.class)).containsExactly(신분당선_상행종점역_ID, 신분당선_하행종점역_ID); assertThat(getResponse.jsonPath().getList("downStationId", Long.class)).containsExactly(신분당선_하행종점역_ID, 신분당선_신규역_ID); assertThat(getResponse.jsonPath().getList("distance")).containsExactly(10, 5);

말씀하신대로 위처럼 작성해보았는데 이해하려면 3초 정도 버퍼링이 걸리는 것 같기도 해서 아예 새로 생성된 구간만 isEqualTo()로 검증하도록 수정해보았습니다! eb8b58c

assertThat(sectionResponse.getUpStationId()).isEqualTo(신분당선_하행종점역_ID); assertThat(sectionResponse.getDownStationId()).isEqualTo(신분당선_신규역_ID); assertThat(sectionResponse.getDistance()).isEqualTo(5);

testrace · 2023-07-30T00:07:10Z

+        @Nested
+        @DisplayName("새로운 구간 등록 실패")
+        class CreateSectionWithInvalidRequest {


예외 테스트 👍

- null 체크 추가 - color 업데이트 추가

testrace

안녕하세요 지선님 😃
피드백 반영 잘해주셨네요 👍
지난 코멘트에서 설명이 부족한 부분이 있는 것 같아 추가 코멘트 남겼습니다
확인해 주시고 다시 리뷰 요청 부탁드려요

testrace · 2023-07-30T10:38:55Z

+    public void add(Section section) {
+        if (!sections.isEmpty()) {
+            validateUpStationId(section);
+            validateDownStationId(section);
+            validateDistance(section);
+        }
+
+        sections.add(section);


구간 등록 기능처럼 구간 제거 메서드도 추가하면 어떨까요?

리뷰 반영했습니다!ㅎㅎ eb1ecaf

testrace · 2023-07-30T10:41:26Z

+    }
+
+    public List<SectionResponse> findSections(Long lineId) {
+        List<Section> sections = sectionRepository.findAllByLine_Id(lineId);


노선에 포함된 구간들을 line 객체를 활용해서 가져올 수가 있는데요
sectionRepository 대신 line 객체를 활용해서 구간 목록을 조회하면 어떨까요?

Suggested change

List<Section> sections = sectionRepository.findAllByLine_Id(lineId);

Line line = findLineById(lineId);

List<Section> sections = line.getSections();

testrace · 2023-07-30T10:43:16Z

+    public void deleteSection(Long lineId, Long stationId) {
+        Line line = findLineById(lineId);
+
+        if (line.hasLessThanTwoSections()) {
+            throw new InvalidSectionRequestException("구간이 2개 이상일 때만 삭제할 수 있습니다.");
+        }
+
+        if (!line.isTerminalStationId(stationId)) {
+            throw new InvalidSectionRequestException("마지막 구간만 삭제할 수 있습니다.");
+        }
+
+        Section section = sectionRepository.findByLine_IdAndDownStation_Id(lineId, stationId)
+                .orElseThrow(() -> new NotFoundException(
+                        Map.of(
+                                "lineId", lineId.toString(),
+                                "stationId", stationId.toString()
+                        )));
+
+        sectionRepository.delete(section);
+    }


addSection 메서드처럼 line 을 조회한 후에 메시지를 보내서 구간을 삭제하면 어떨까요?
삭제 기능이니 @Transactional도 추가되면 좋을 것 같아요
line.deleteSection(stationId);

리뷰 반영했습니다!ㅎㅎ eb1ecaf

testrace · 2023-07-30T10:58:06Z

+            assertThat(sectionResponse.getUpStationId()).isEqualTo(신분당선_하행종점역_ID);
+            assertThat(sectionResponse.getDownStationId()).isEqualTo(신분당선_신규역_ID);
+            assertThat(sectionResponse.getDistance()).isEqualTo(5);


사실 변경 전과 다르지 않다고 생각들어요 😅
인수 테스트는 블랙박스 테스트이니 하나 하나의 구간이 어떻게 구성되었는지 관심 있기 보다
구간이 추가되었을 때 노선이 가지는 전체 역 목록이 원하는대로(순서대로) 구성되었는지가 더 관심이 크다고 생각해서 피드백 드렸었는데요
노선에 A-B-C 역이 있을 때 하행역(C-D 구간)을 추가하면 노선의 전체 역 목록을 A-B-C-D가 되었음을 검증하면
인수 테스트를 읽는 입장에서 구간 추가 기능을 빠르게 파악할 수 있을 것 같아요

testrace · 2023-07-30T11:01:36Z

    @Column(length = 20, nullable = false)
    private String name;

+    @Setter


setter를 제공하기 보다 update or modify 메서드를 제공하고
메서드 내부에서 값을 수정하도록 하면 어떨까요?

testrace · 2023-07-30T11:03:19Z

+        setIfNotNull(modifyLineRequest.getName(), line::setName);
+        setIfNotNull(modifyLineRequest.getColor(), line::setColor);


setIfNotNull 메서드를 거치지 않고 line 의 값을 변경하면 도메인의 제약사항이 지켜지지 않을 것 같아요
null 체크를 line 내부에서 검증하면 어떨까요?

janeljs added 14 commits July 29, 2023 21:38

📝 step 3 요구사항 업데이트

f51f4ec

♻️ BaseEntity 클래스 추가

2a286c1

📦 NotFoundException 패키지 이동

2823721

🎨 코드 정렬

03bf561

➕ mockito-core 의존성 추가

5d020a4

♻️ StationResponse를 record 클래스로 변경, StationMapper 메서드 이름 수정

191e611

♻️ distance를 int type으로 변경

8774fd2

✨ SectionAcceptanceTest 추가

a91f8a0

✨ SectionController 추가, DTO 및 메서드 이름 형식 통일

72d5fbc

♻️ Station, Line 관련 메서드 및 DTO 이름 통일

a47ed9a

✨ 구간 등록, 제거 기능 구현

11e8be0

⬆️ Java 16으로 버전업

84c0ebd

📦 테스트 패키지 이동

9b5259e

✏️ 오타 수정

69d31c6

testrace requested changes Jul 30, 2023

View reviewed changes

janeljs added 3 commits July 30, 2023 16:20

♻️ Section 생성 요청 시 검증 로직을 Sections 내부로 이동 및 리팩토링

63c094a

➖ 사용하지 않는 의존성 삭제

348513e

♻️ 구간 생성 테스트코드 리팩토링

eb8b58c

janeljs requested a review from testrace July 30, 2023 08:00

♻️ SectionService, Controller에 있던 로직을 LineService, Controller로 이동

cfb49a7

- null 체크 추가 - color 업데이트 추가

testrace requested changes Jul 30, 2023

View reviewed changes

♻️ Line, Sections에 구간 제거 메서드 추가

eb1ecaf

janeljs closed this Mar 2, 2026

		List<Section> findAllByLine_Id(Long lineId);

		Optional<Section> findByLine_IdAndDownStation_Id(Long lineId, Long downStationId);

	List<Section> sections = sectionRepository.findAllByLine_Id(lineId);
	Line line = findLineById(lineId);
	List<Section> sections = line.getSections();

		setIfNotNull(modifyLineRequest.getName(), line::setName);
		setIfNotNull(modifyLineRequest.getColor(), line::setColor);

Conversation

janeljs commented Jul 29, 2023

Uh oh!

testrace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janeljs Jul 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

testrace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

janeljs Jul 30, 2023 •

edited

Loading