[1단계 - 지하철 정보 관리 기능] 조이(김성연) 미션 제출합니다. #54

yeonkkk · 2023-05-11T08:45:48Z

안녕하세요 또링! 조이입니다.

페어(도치)와 함께 최대한 요구사항에 맞게 구현해보았습니다.
하지만 시간이 부족하여 미흡한 부분을 많이 남겨 둔 채로 리뷰를 요청드리게 되었습니다(request 검증, 메서드 분리, 테스트, 예외 처리 등)😓.

부족함이 많지만 이번 미션 리뷰 잘 부탁드리겠습니다!
좋은 하루 되세요!

jnsorn

안녕하세요. 조이! 미션 잘 구현해주셨네요 🚇
크게 변경해야할 부분은 없는 것 같아 바로 다음단계를 진행할까 하다가
1단계에서 학습했던 부분들을 적용해보면 코드가 더 깔끔해질것같아 코멘트 남겼습니다.
확인해보시고 다시 리뷰요청 주세요 😃

jnsorn · 2023-05-13T04:59:49Z

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

+import java.util.stream.Collectors;
+
+public class Sections {
+    private final long lineId;


사용하고 있지 않은 필드로 보이는데요, Sections가 lineId를 갖게 된 이유가 있을까요?

처음 구현할 당시에는 Line과 Sections의 관계를 표현하기 위한 2가지 방법을 고민했었습니다.

Line이 Sections를 가지게 하는 것

Sections가 lineId를 가지게 하는 것

페어와 오랜 고민 끝에 2번을 선택했는데요, 그 이유는 Sections이 line id를 가지는 것만으로도 대부분의 로직을 수행할 수 있다고 생각했습니다. 그리고 Line이 Sections를 가지는 경우, 두 객체간의 로직이 거의 없는 상황에서 Line을 생성할 때마다 Sections를 만드는 것이 비효율적이라고 생각하였습니다.
Line을 생성하기 위해서 불필요하게 Sections를 생성하기 위한 데이터를 db에서 가져와야 한다고 생각했기 때문입니다.

위와 같은 이유로 Sections가 lineId를 가지게 되었고, 리팩터링한 현재의 코드에서는 삭제하였습니다!

jnsorn · 2023-05-13T05:01:17Z

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

+
+    public Section(final Long id, final Station upStation, final Station downStation, final int distance,
+                   final Long nextSectionId) {
+        // TODO: 2023/05/10 distance 1이상 검증


distance에도 비즈니스 규칙이 적용되어야 하는군요~
레벨1에서 학습한대로 값객체를 만들어보는건 어떨까요?
distance외에도 name등 여러 곳에 적용해볼수 있을 것 같네요 :)

또링이 말씀해주신 대로 리팩토링 과정에서 Distance 값 객체를 생성하였습니다!

jnsorn · 2023-05-13T05:02:46Z

src/main/java/subway/application/SectionService.java

+    public long addSection(final long lineId, SectionRequest request) {
+        validateLineId(lineId);
+        Station requestUpStation = findVerifiedStationByName(request.getUpStationName());
+        Station requestDownStation = findVerifiedStationByName(request.getDownStationName());
+
+        Sections sections = sectionDao.findSectionsByLineId(lineId);
+
+        Section requestedSection = new Section(requestUpStation, requestDownStation, request.getDistance());
+
+        if (sections.isInitialSave()) {
+            return sectionDao.save(requestedSection, lineId);
+        }
+        if (sections.isDownEndAppend(requestedSection)) {
+            long savedSectionId = sectionDao.save(requestedSection, lineId);
+            Section downEndSection = sections.getDownEndSection();
+            sectionDao.updateNextSection(savedSectionId, downEndSection.getId());
+            return savedSectionId;
+        }
+        if (sections.isUpEndAppend(requestedSection)) {
+            Section upEndSection = sections.getUpEndSection();
+            Section newSection = new Section(requestUpStation, requestDownStation, request.getDistance(),
+                    upEndSection.getNextSectionId());
+            return sectionDao.save(newSection, lineId);
+        }
+
+        Section includeSection = sections.getIncludeSection(requestedSection);
+        Direction direction = includeSection.checkDirection(requestedSection);
+        if (direction == Direction.INNER_LEFT) {
+            Section innerRight = new Section(requestedSection.getDownStation(), includeSection.getDownStation(),
+                    includeSection.getDistance() - requestedSection.getDistance(),
+                    includeSection.getNextSectionId());
+
+            long savedId = sectionDao.save(innerRight, lineId);
+
+            Section innerLeft = new Section(includeSection.getId(), requestedSection.getUpStation(),
+                    requestedSection.getDownStation(), requestedSection.getDistance(), savedId);
+
+            sectionDao.update(innerLeft);
+            return savedId;
+        }
+


메서드를 분리해볼까요?

기존에 SectionService의 로직 분리가 어렵다고 느껴 전체 구조를 변경해보았습니다!
수정하면서 메서드 분리도 함께 진행하였습니다.
감사합니다!

jnsorn · 2023-05-13T05:03:19Z

src/main/java/subway/application/SectionService.java

+            sectionDao.deleteByLineId(lineId);
+        }
+        // 상행 종점일 때
+        if (sections.getUpEndSection().isSameUpStationId(stationId)) {


get하지 않고 바로 물어볼 수 있을 것 같아요~

수정하였습니다!

jnsorn · 2023-05-13T05:03:28Z

src/main/java/subway/application/SectionService.java

+        Section downEndSection = sections.getDownEndSection();
+        if (downEndSection.isSameDownStationId(stationId)) {
+            Section section = sections.findSectionByNextSection(downEndSection);


여기도 동일하게 변경해보면 좋겠네요

수정하였습니다!

jnsorn · 2023-05-13T05:08:15Z

src/main/java/subway/ui/SectionController.java

+import subway.dto.StationsResponse;
+
+@RestController
+public class SectionController {


조이가 작성한 코드는 아니지만 LineController와 StationController도 다른 컨트롤러와 일관성을 갖도록 수정해보면 어떨까요?

@RequiredArgsConstructor 에 대한 말씀으로 이해하여 해당 내용을 수정하였습니다.
만약 다른 의미로 말씀 주신 것이라면 알려주시면 감사하겠습니다!

jnsorn · 2023-05-13T05:10:34Z

src/main/java/subway/dto/SectionSaveRequest.java

@@ -0,0 +1,25 @@
+package subway.dto;
+
+public class SectionSaveRequest {


사용하지 않는 객체는 제거해주세요~!

리팩터링을 진행하면서 삭제하였습니다. 감사합니다!

jnsorn · 2023-05-13T06:06:12Z

src/main/java/subway/application/SectionService.java

+        }
+    }
+
+    private Station findVerifiedStationByName(String name) {


어떤 의도의 네이밍일까요?
해당 메서드가 두가지일을 하고 있는 것 같아요~

Station Name을 통해 Station을 찾고 없으면 생성하여 반환해주고자 하였습니다.
또링의 말씀대로 두 가지일을 하고 있는 것 같습니다.
해당 메서드는 리팩터링 과정에서 제거하였습니다!

jnsorn · 2023-05-13T06:07:17Z

src/main/java/subway/ui/SectionController.java

+import subway.dto.StationsResponse;
+
+@RestController
+public class SectionController {


SectionController도 테스트를 작성해볼까요?

리팩터링을 진행하면서 SectionIntegrationTest 를 추가로 작성했습니다!

yeonkkk

안녕하세요 또링!

전체적인 구조와 코드를 변경하다보니, 빨리 리뷰를 해주셨음에도 리뷰 요청이 늦어졌습니다..😓

이번 리팩터링 과정에서 생긴 주요한 변화는 아래와 같습니다.

Entity와 Repository 생성
Section의 id와 nextId 제거
StationController, StationService 제거 - SectionController, SectionService, SectionRepository로 통합

코드를 작성하면서 생긴 질문은 코드와 함께 아래에 작성하였습니다!
이번에도 리뷰 잘 부탁드리겠습니다!

감사합니다!

yeonkkk · 2023-05-19T14:02:50Z

src/main/java/subway/ui/SectionController.java

+import subway.dto.StationsResponse;
+
+@RestController
+public class SectionController {


리팩터링을 진행하면서 SectionIntegrationTest 를 추가로 작성했습니다!

yeonkkk · 2023-05-19T14:04:22Z

src/main/java/subway/ui/SectionController.java

+import subway.dto.StationsResponse;
+
+@RestController
+public class SectionController {


@RequiredArgsConstructor 에 대한 말씀으로 이해하여 해당 내용을 수정하였습니다.
만약 다른 의미로 말씀 주신 것이라면 알려주시면 감사하겠습니다!

yeonkkk · 2023-05-19T14:22:48Z

src/main/java/subway/domain/Station.java

+    public boolean equalsName(Station station) {
+        return this.name.equals(station.name);
    }


식별자 역할로 Station 도메인이 id를 가집니다.
그리고 Equals와 hashCode를 구현하였습니다.
그런데 새로운 역을 추가할 때 입력되는 기존에 존재하던 역을 도메인 내부에서 비교할 때, id가 있는 객체와 없는 객체를 비교하게 됩니다. 이 경우 동일한 것으로 판단해야하기에 고민하다가 해당 메서드를 구현하였습니다.
Equals와 hashCode를 재정의할 때 name만을 사용할 수 있었으나, id 값이 객체 식별의 목적을 가지고 있으므로 이를 재정의하기 보다는 메서드를 구현하는 것이 낫다고 판단하였습니다.
그런데 어떤 방법이 더 좋은지 잘 모르겠습니다. 그래서 또링의 의견을 여쭤보고자 합니다!

현재 equalsName() 은 결국 station이 같은 객체인지 판단하는데 사용되고 있네요. (ex. isNextSection())
station의 동등성을 판단하는 기준이 name이라면, station의 id는 어떤 역할일까요?
정말 station의 name이 같으면 같은 객체가 맞을까요?

station의 name이 유니크한 값이기 때문에 id를 제거하고 name으로 객체 비교를 해도 되지 않을까?라는 생각을 했습니다.
하지만 조금 더 생각해본다면 1) 요구사항 변경으로 유니크 제약 조건이 사라지거나 2) 기존 역의 이름이 변경된 상태에서 어떠한 요청이 들어왔을 때 식별의 어려움이 발생할 수 있을 것 같습니다.

다른 방법을 고민해보겠습니다!

yeonkkk · 2023-05-19T14:25:33Z

src/main/java/subway/dto/LineRequest.java

+@Getter
+@AllArgsConstructor
+@NoArgsConstructor
 public class LineRequest {
+    @NotBlank(message = "호선명은 빈 문자열일 수 없습니다.")
    private String name;
-    private String color;


전체적으로 Lombok을 사용하였고, 일부 필요한 경우에 Lombok을 사용하지 않고, 직접 구현했습니다.
이렇게 사용하기 보다는 일관성을 맞추기 위해서 아예 사용하지 않거나, 모두 사용하는 것이 좋을까요?
또링의 의견이 궁금합니다!

개인적으로 일관성을 맞추는 것을 선호하지만, 현재는 미션중이기 때문에 학습을 위해 Lombok 없이 구현해보는 것을 권장드리고 있습니다. 다만 기존 프로젝트에 lombok 의존성이 추가되어있는 상태이기 때문에, 이 부분은 조이가 선택해서 적용해주세요.

참고하여 진행하겠습니다. 감사합니다!

yeonkkk · 2023-05-19T14:29:55Z

src/main/java/subway/entity/vo/SectionVo.java

+@Getter
+@RequiredArgsConstructor(access = AccessLevel.PRIVATE)
+public class SectionVo {
+    private final StationEntity upStationEntity;
+    private final StationEntity downStationEntity;
+    private final Integer distance;


리팩터링 과정에서 dm으로 문의드렸던 것 처럼 해당 객체에 대한 정체성을 고민하다가 Vo로 작성하였습니다.
식별자가 존재하지 않고 불변 값이기 때문에 Vo의 조건을 충족한다고 생각하였습니다.
또한 dto의 경우 로직이 포함하지 않고, 데이터 전달의 역할을 한다고 알고 있습니다.
현재 이 객체에서는 로직이 들어가 있지는 않지만, 추후에 추가될수도 있기 때문에 dto 보다는 vo에 가깝지 않을까 생각하였습니다.
이에 대한 또링의 의견도 궁금합니다!

yeonkkk · 2023-05-19T14:34:52Z

src/main/java/subway/repository/LineRepository.java

+@Transactional
+@Repository
+public interface LineRepository {
+    Line insert(final Line line);


현재 @Repository 애너테이션을 Dao와 Repository에 모두 사용하고 있습니다.
repository는 네이밍 그대로 repository의 역할을 하고, dao는 db에 접근하여 필요한 데이터를 가져오는 역할을 합니다.
그래서 이 경우에 양쪽 모두 붙이는 것인지, db에 직접적으로 접근하는 dao에만 붙이는 건지 궁금합니다..!

@Repository 어노테이션을 사용하는 이유는 무엇일까요? 또, @Transactional을 해당 인터페이스에 선언해줌으로써 어떤 역할을 할까요?

@Repository
@Component를 사용하지 않고 @Repository를 사용하는 이유는 데이터베이스와 데이터를 송수신하는 과정에서 발생할 수 있는 unchecked exception들을 스프링의 DataAccessException으로 처리할 수 있기 때문입니다. 그리고 명시적인 이유도 있을 것 같습니다.
이 관점에서 보았을 때, repository에 해당 애너테이션을 붙이기보다는 db와 직접적으로 통신하는 dao에 해당 애너테이션을 사용하는 것이 좋을 것 같습니다.

@Transactional
@Transacional을 사용하여 문제가 발생할 경우 rollback할 수 있도록 하고싶었습니다. 그런데 이미 service에서 @Transactional 을 사용했기 때문에 내부에서 호출되는 repository의 메소드들에도 적용이 될 것 같습니다.

그러니 위 의 경우 두 애너테이션 모두 불필요할 것 같습니다! 수정하겠습니다.

yeonkkk · 2023-05-19T14:35:58Z

src/main/resources/schema.sql

+create table if not exists SECTIONS
+(
+    up_id bigint not null,
+    down_id bigint not null,
+    line_id bigint not null,
+    distance int not null
+);


리팩터링 과정에서 기존에 Section이 가지고 있던 SectionId와 nextSectionId를 제거하였습니다!

jnsorn

안녕하세요 조이! 리뷰 잘 반영해주셨어요!
1단계 미션은 여기서 마무리하고, 2단계 미션 진행해볼까요?

jnsorn · 2023-05-20T16:32:45Z

src/main/java/subway/domain/Station.java

+    public boolean equalsName(Station station) {
+        return this.name.equals(station.name);
    }


현재 equalsName() 은 결국 station이 같은 객체인지 판단하는데 사용되고 있네요. (ex. isNextSection())
station의 동등성을 판단하는 기준이 name이라면, station의 id는 어떤 역할일까요?
정말 station의 name이 같으면 같은 객체가 맞을까요?

jnsorn · 2023-05-20T16:40:54Z

src/main/java/subway/dto/LineRequest.java

+@Getter
+@AllArgsConstructor
+@NoArgsConstructor
 public class LineRequest {
+    @NotBlank(message = "호선명은 빈 문자열일 수 없습니다.")
    private String name;
-    private String color;


개인적으로 일관성을 맞추는 것을 선호하지만, 현재는 미션중이기 때문에 학습을 위해 Lombok 없이 구현해보는 것을 권장드리고 있습니다. 다만 기존 프로젝트에 lombok 의존성이 추가되어있는 상태이기 때문에, 이 부분은 조이가 선택해서 적용해주세요.

jnsorn · 2023-05-20T16:43:08Z

src/main/java/subway/repository/LineRepository.java

+@Transactional
+@Repository
+public interface LineRepository {
+    Line insert(final Line line);


@Repository 어노테이션을 사용하는 이유는 무엇일까요? 또, @Transactional을 해당 인터페이스에 선언해줌으로써 어떤 역할을 할까요?

jnsorn · 2023-05-20T16:51:01Z

src/main/java/subway/dto/LineRequest.java

+@Getter
+@AllArgsConstructor
+@NoArgsConstructor
 public class LineRequest {


(참고) jackson 2.13부터 기본생성자 없이도 바인딩이 가능해졌습니다. "No Constructor" Deserializer module

참고하여 수정하겠습니다.
감사합니다!

jnsorn · 2023-05-20T16:57:03Z

src/main/java/subway/domain/Status.java

+public enum Status {
+    INIT,
+    INCLUDED,
+    NOT_INCLUDED
+}


status를 따로 분리한 목적은 무엇일까요?

새로운 station을 추가하거나 삭제할 때 특정 노선에 최초로 등록하는 것인지, 기존에 등록되어 있는 역인지, 새롭게 등록할 역인지를 구분하기 위해 분리하였습니다.

yeonkkk added 10 commits May 9, 2023 14:01

docs: 기능 요구사항 작성

171ff4a

feat: Section(구간) 및 SectionDao 구현

a9bf07f

feat: Sections 구현

0adff23

feat: SectionService 구현

917ca85

feat: 역을 제거하는 기능 구현

c27d3df

test: 역을 추가하는 테스트

a3a691b

refactor: 테스트 fixture 분리, 전체 역 조회 메서드 로직 수정

245839a

docs: 기능 요구 사항 수정

1528b1b

refactor: 검증 추가, 메서드 정렬, station 검증 수정

c27a38a

fix: 잘못된 sql 수정

ba249ed

jnsorn self-requested a review May 12, 2023 08:42

jnsorn requested changes May 13, 2023

View reviewed changes

yeonkkk added 10 commits May 15, 2023 15:56

refactor: Lombok 애너테이션 적용

7b15531

refactor: 기존 도메인들 새 파일로 변경

1be28e6

chore: schema.sql 파일 수정

d691518

refactor: entity, dao 수정

b238c01

feat: sectionRepository 생성

f9b5713

refactor: contoller, service 로직 수정

26f518d

feat: ExceptionAdvice 생성

9cfd39e

docs: 기능 목록 업데이트

4b4e86e

refactor: 인터페이스 분리, 예외 처리

4afa981

refactor: 일관성을 위해 SectionController의 애너테이션 수정

f0db060

yeonkkk commented May 19, 2023

View reviewed changes

refactor: 애너테이션 인터페이스로 변경

5f267fe

yeonkkk commented May 19, 2023

View reviewed changes

jnsorn approved these changes May 20, 2023

View reviewed changes

jnsorn merged commit fc653ac into woowacourse:yeonkkk May 20, 2023

		@@ -0,0 +1,25 @@
		package subway.dto;

		public class SectionSaveRequest {

[1단계 - 지하철 정보 관리 기능] 조이(김성연) 미션 제출합니다. #54

[1단계 - 지하철 정보 관리 기능] 조이(김성연) 미션 제출합니다. #54

Conversation

yeonkkk commented May 11, 2023

jnsorn 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

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

yeonkkk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeonkkk May 19, 2023 • edited Loading

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

jnsorn 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

yeonkkk left a comment •

edited

Loading

yeonkkk May 19, 2023 •

edited

Loading