[Spring 지하철 노선도 관리 - 3단계] 다니(이다은) 미션 제출합니다. #141

da-nyee · 2021-05-11T16:42:21Z

안녕하세요, 닉! 다니입니다 🙌

이번 3단계는 저번 1, 2단계처럼 페어 프로그래밍을 했어요!
혼자보다 페어로 하니까 프로그램 설계나 코드에 관해 더 고민하고 의견을 나눌 수 있었습니다 👍

레이어간 의존 관계에 대해 고민하다 질문이 하나 생겼어요 !

서비스에서 여러 DAO를 의존하는 구조 vs 서비스에서 여러 서비스를 의존하는 구조

두 구조 중에 어느 구조가 더 바람직한 건가요??

저는 처음에 전자로 구현하다 중복 코드가 많아지는 것 같아 나중에 후자로 변경했습니다 !
현재 구조는 아래와 같은 모습이에요.

이렇게 의존하니까 중복 코드가 없어지는 장점이 있었지만, 구조가 다소 복잡해지는 단점도 존재했어요.
이런 구조도 괜찮은 건가요?? 닉의 생각이 궁금해요!

코드에서 부족한 부분 전부 지적해주세요~!
피드백 기다리고 있겠습니다 ✨

매번 귀한 시간 내주셔서 감사합니다 🙇‍♀️❣️

- 정렬된 상행역 -> 하행역 목록 반환

wbluke

3단계도 잘 진행해주셨네요 다니! 👍
질문에 대한 답변 포함해서 몇 가지 코멘트 남겼으니 확인 부탁드려요 🙂

wbluke · 2021-05-12T23:49:54Z

src/main/java/wooteco/subway/section/SectionRequest.java

@@ -1,4 +1,4 @@
-package wooteco.subway.line;
+package wooteco.subway.section;

 public class SectionRequest {
    private Long upStationId;


java bean validation의 @NotNull 등을 사용해서 validation을 해보면 어떨까요? :)

wbluke · 2021-05-12T23:52:08Z

src/main/java/wooteco/subway/section/SectionController.java

+
+    @PostMapping("/{lineId}/sections")
+    public ResponseEntity<SectionResponse> createSection(@RequestBody SectionRequest sectionRequest, @PathVariable Long lineId) {
+        lineService.validateId(lineId);


Controller에서는 최소한의 요청, 응답, 그리고 request 값에 대한 유효성 체크만 가져가고,
실제 lineId가 존재하는지에 대한 validation은 서비스 레이어에서 하는게 좋을 것 같아요 :)

wbluke · 2021-05-12T23:54:14Z

src/main/java/wooteco/subway/line/LineService.java

+    public void validateId(Long lineId) {
+        lineDao.findById(lineId)
+                .orElseThrow(LineNonexistenceException::new);
+    }


목적과 실제 행위가 정확하게 일치하지는 않는 것 같아요.
단건 조회를 통한 예외 체크보다는 DAO에서 존재성을 체크하는 기능을 별도로 제공하면 어떨까요? (exists 쿼리 or limit 1 쿼리)

wbluke · 2021-05-12T23:55:57Z

src/main/java/wooteco/subway/section/Section.java

+    public boolean isUpStation(Long id) {
+        return upStationId.equals(id);
+    }
+
+    public boolean isDownStation(Long id) {
+        return downStationId.equals(id);
+    }


파라미터의 id가 어떤 id인지 나타내주면 좋을 것 같아요 :)

wbluke · 2021-05-12T23:57:47Z

src/main/java/wooteco/subway/section/Sections.java

+
+import static java.util.stream.Collectors.toList;
+
+public class Sections {


핵심 비즈니스 로직을 담고 있는 객체인데 단위테스트가 없네요 ㅎㅎ 추가해보면 좋겠어요 :)

wbluke · 2021-05-13T00:02:52Z

src/main/java/wooteco/subway/section/SectionService.java

+        Section section = new Section(
+                sectionRequest.getUpStationId(), sectionRequest.getDownStationId(), sectionRequest.getDistance());
+        Sections sections = new Sections(sectionDao.findByLineId(lineId));
+        sections.validate(section);
+        Optional<Section> overlappedSection = sectionDao.findBySameUpOrDownId(lineId, section);
+        Section newSection = sectionDao.save(lineId, section);
+        overlappedSection.ifPresent(updateIntermediate(newSection));
+        return newSection;


어떤 일들을 하고 있는지 한눈에 파악하기가 어려운데요.
공백 라인도 의미를 가지기 때문에, 맥락에 따라 공백 라인을 넣어 자신의 의도를 표현하는 연습도 같이 해보면 좋을 것 같아요.
그렇게 나누다가 작은 묶음을 메서드로 분리할 수 있다면 더 좋고요!

wbluke · 2021-05-13T00:07:02Z

src/main/java/wooteco/subway/section/SectionService.java

+    private final StationService stationService;
+    private final SectionDao sectionDao;


레이어간 의존 관계에 대해 고민하다 질문이 하나 생겼어요 !

서비스에서 여러 DAO를 의존하는 구조 vs 서비스에서 여러 서비스를 의존하는 구조

두 구조 중에 어느 구조가 더 바람직한 건가요??

정답이 없는 문제이긴 한데요 ㅎㅎ
저는 기본적으로 서비스에서 여러 DAO를 의존하는 구조 를 지향하고,
가끔 너무 공통적인 타 도메인 서비스의 로직이 반복되거나 하는 경우는 다른 서비스를 가져와서 쓰기도 합니다 :)

DAO(혹은 Repository)는 최소 단위의 DB Access 로직만 제공하기 때문에 서비스 레이어보다 각 API의 기능이 명확하고요.
같은 레이어끼리 참조하다보면 오히려 도메인별로 레이어를 나눈게 무색할 정도로 스파게티 코드가 될 위험이 있다고 생각하기 때문이에요 ㅎㅎ

wbluke · 2021-05-13T00:13:58Z

src/test/java/wooteco/subway/section/SectionAcceptanceTest.java

+        ExtractableResponse<Response> response = RestAssured.given().log().all()
+                .body(sectionRequest)
+                .contentType(MediaType.APPLICATION_JSON_VALUE)
+                .when()
+                .post("/lines/1/sections")
+                .then().log().all()
+                .extract();


이 부분 공통으로 추출해볼 수 있을 것 같아요 :)

wbluke · 2021-05-13T00:19:02Z

src/test/java/wooteco/subway/section/SectionAcceptanceTest.java

+import static org.assertj.core.api.Assertions.assertThat;
+
+@DisplayName("지하철 구간 관련 기능")
+@Sql("/data.sql")


저는 data.sql 을 사용한 테스트를 지양하는데요 ㅎㅎ
이 파일을 의존해 테스트가 작성된다면, 나중에는 해당 파일에 대한 강한 의존성으로 여러 테스트를 수정하기도, 추가하기도 어렵습니다.
지금의 Section 인수테스트를 보면서도 test fixture가 보이지 않아서, 어떤 내용을 테스트하고자 하는지 파악하기가 어려워보여요 ㅎㅎ
test fixture는 웬만하면 각 메서드에, 그게 어렵다면 각 클래스에 생성해주는 것이 좋습니다 :)

적용했습니다!

기존 data.sql 대신 @BeforeEach로 매 테스트 실행 전에 필요한 데이터를 세팅하도록 했습니다 !
이런 방식으로 test fixture를 생성하면 되나요??

wbluke · 2021-05-13T00:20:13Z

src/test/java/wooteco/subway/section/SectionAcceptanceTest.java

+
+    private final SectionRequest sectionRequest = new SectionRequest(2L, 3L, 10);
+
+    @DisplayName("구간을 생성한다.")


기존 구간의 상행역을 포함한 추가

기존 구간의 하행역을 포함한 추가

이와 같이 더 상세한 케이스로 나눠볼 수 있을 것 같아요 :)

da-nyee · 2021-05-18T17:42:19Z

안녕하세요, 닉! 다니입니다 🙌

먼저 코드 리뷰 감사합니다!! 덕분에 많이 학습할 수 있었습니다 !
이번 리팩토링은 경로 조회 / 로그인 미션과 겹쳐 생각보다 더 늦어졌습니다 💧

저번에 레이어간 의존 관계에 대해 질문을 드렸는데요!
코드를 수정하다보니 자연스레 서비스에서 여러 DAO를 의존하는 구조로 변경하게 됐습니다.

현재 구조는 아래 그림과 같아요 !

이렇게 구조를 변경하니까 제가 생각했던 것과 달리 훨씬 깔끔하게 코드를 구성할 수 있었어요. 중복 코드도 따로 없었어요 !
말씀하신 것처럼 같은 레이어별로 의존하면 오히려 스파게티 코드가 될 가능성이 생길 거 같아요. 답변 감사합니다!

리팩토링 하면서 궁금했던 점은 👀 이모지와 함께 코멘트로 남겨뒀어요! 확인해주시면 감사하겠습니다!

이번에도 부족한 부분 마구마구 지적해주세요! 피드백 기다리고 있겠습니다 ~!

항상 귀한 시간 내주셔서 감사합니다 🙇‍♀️❣️

da-nyee · 2021-05-19T11:14:36Z

저번 단계 코멘트도 함께 반영했습니다!

da-nyee · 2021-05-19T12:58:46Z

📚 다니의 학습 로그

[Spring] Bean Validation - 3

내용

[Spring] Bean Validation

음 이런 도메인에는 java bean validation을 사용하지 않는 것이 좋습니다.
도메인 레이어는 프로젝트 전반적으로 쓰이기 때문에 순수한 자바 객체로 유지하는 것이 좋은데요, 지금 같은 경우는 validation 라이브러리에 의존하는 구조가 되어버립니다.
말씀드린 validation은 request DTO에서 최초 검증하는 용도로만 사용해 주세요!

wbluke · 2021-05-20T00:22:00Z

src/test/java/wooteco/subway/section/SectionTestFixture.java

+public class SectionTestFixture {
+    public static Section 강남_역삼 = new Section(1L, 2L, 10);
+    public static Section 역삼_선릉 = new Section(2L, 3L, 6);
+    public static Section 강남_선릉 = new Section(1L, 3L, 16);
+    public static Section 선릉_삼성 = new Section(3L, 4L, 8);
+    public static Section 교대_강남 = new Section(5L, 1L, 10);
+    public static Section 교대_서초_불가능1 = new Section(5L, 6L, 10);
+    public static Section 교대_서초_불가능2 = new Section(5L, 6L, 14);
+    public static Section 합정_당산 = new Section(10L, 11L, 12);


fixture를 잘 만들어 주셨군요 👍
추가적으로, 저는 fixture가 메서드마다, 그게 어렵다면 클래스마다 있어야 한다고 생각하는데요.
이렇게 별도의 파일로 존재할 경우 테스트를 작성할 때 항상 이 내용을 모두 인지하고 있어야만 하고,
테스트를 읽는 사람도 해당 파일까지 번갈아 확인하면서 내용을 파악해야 하기 때문에 허들이 생깁니다.
(data.sql과 크게 다르지 않을 수 있어요 ㅎㅎ)

그리고 static으로 fixture를 만들면 공통으로 해당 인스턴스를 사용하는 다수의 테스트에서 어떤 동시성 이슈가 있을지 예측할 수 없습니다.
한쪽에서 fixture를 수정하는 테스트를 했는데 다른 쪽에서 영향을 받을 수 있겠죠.
고로 로컬 변수나 클래스 필드로 만들어주는 것이 좋습니다 :)

테스트를 작성할 때 제가 가장 중시하는 원칙 중 하나는
테스트는 애플리케이션을 설명하는 문서 라는 말입니다.
프로젝트를 처음 보는 사람이 이 테스트 문서를 읽었을 때 애플리케이션이 어떻게 동작하겠구나를 대략적으로라도 예측할 수 있도록
좋은 테스트 케이스와 명확한 명세로 정리하는 것이 중요하다고 생각합니다 :)

da-nyee · 2021-05-21T11:16:05Z

안녕하세요, 닉! 다니입니다 🙌

추가 피드백 반영해서 다시 제출합니다 ~!
이번에도 잘 부탁드립니다 ✨

귀한 시간 내주셔서 감사합니다 🙇‍♀️❣️

wbluke

추가 피드백 반영까지 긴 시간 고생 많으셨어요 👏
남은 미션도 화이팅입니다 🙂

da-nyee and others added 30 commits May 9, 2021 01:19

fix(dao): 노선 이름으로 데이터를 찾는 쿼리 버그 수정

e80a453

test(dao): 노선 이름으로 데이터를 찾는 쿼리 테스트 작성

2c64ceb

docs: README.md 3단계 기능 요구사항 추가

e449cd8

docs: 객체 구조 설계 작성

a782255

feat: 구간을 추가하는 기능 구현

523c8c4

feat: 노선을 추가하는 기능 구현

be7971f

feat: 노선을 추가하는 기능 수정 및 구간 초기화

493bffc

feat: 구간 추가 해당 노선 연결 여부 검증 로직 구현

65051f5

feat: 해당 노선에 이미 등록된 구간인지 검증하는 로직 구현

c07eacb

feat: 등록 가능한 구간 거리인지 검증하는 로직 구현

ec6e37b

chore: docs 디렉토리 생성

5af65b7

docs: TODO 작성

37850f1

docs: README.md 기능 요구사항 수정

4f85d7f

fix: 구간 존재 검증 로직 수정

329bf0e

feat: 구간 추가 기능 구현

62cd38a

feat: 구간 제거 기능 구현

ee1d0e5

refactor: Optional 반환으로 변경

133cda1

feat: 구간 삭제 검증 로직 구현

c9a2618

feat: 노선을 조회하는 기능 구현 - 추가 기능

dd68134

- 정렬된 상행역 -> 하행역 목록 반환

refactor: 불필요한 import 삭제

d7b588b

fix: 노선 조회 추가 기능 버그 수정

3ac4d01

feat: 구간 Controller 작성

2c09441

refactor: getter 대신 DTO 사용

8e8588d

refactor: Dao 인터페이스 분리

d658475

refactor: 불필요한 생성자 제거

2322642

refactor: 노선 유효성 검증 로직 수정

5c1c250

refactor: DAO 참조를 서비스 참조로 변경

2434d8a

test: 인수 테스트 버그 수정

fc75427

refactor: 불필요한 import 삭제

4a7a1b7

refactor: 메소드 참조로 변경

dba2a8d

da-nyee added 2 commits May 12, 2021 00:11

refactor(subway): 생성자 체이닝 적용

965c4c7

refactor(subway): 불필요한 import 삭제

bd54bdd

wbluke suggested changes May 13, 2021

View reviewed changes

da-nyee added 16 commits May 17, 2021 16:28

test(sections): 단위 테스트 작성

daf00ad

refactor(service): @transactional 추가

c4f721f

refactor(service): private -> public 생성자로 변경

120e1ec

test(acceptance): 중복 로직 메소드 추출

0a2ac89

refactor(dto): Bean Validation으로 유효성 검증

bcd7943

fix(dto): Bean Validation으로 유효성 검증 버그 해결

98e83b8

refactor(service): 공백 라인 추가 및 메소드 분리

280c69a

refactor(domain): Bean Validation으로 유효성 검증

8e8d018

refactor(section): 파라미터명 수정

1bdad48

refactor(controller): lineId 유효성 검증을 Service에서 하도록 변경

7aa5a39

refactor(service): Service에서 여러 DAO를 의존하는 구조로 변경

41a5aca

refactor(dao): EXISTS, LIMIT 쿼리로 데이터 존재성 체크

47f1123

refactor(dao): rowMapper() 분리

9bf7477

test(section): data.sql 대신 @beforeeach로 test fixtures 생성

4e0caf0

refactor(section): 클래스명 변경

0d90e79

test(section): 구간 생성 및 삭제를 더 상세한 케이스로 나눠 테스트 작성

ce8c234

refactor(subway): DTO가 도메인 객체 생성 책임을 가지도록 변경

d538621

wbluke suggested changes May 20, 2021

View reviewed changes

da-nyee added 3 commits May 21, 2021 19:50

refactor(domain): Bean Validation으로 유효성 검증 삭제

71407d7

test(section): 별도 클래스 대신 @beforeeach로 test fixture 생성

eae6cdb

test(subway): test fixtures 수정

7db3e4b

wbluke approved these changes May 21, 2021

View reviewed changes

wbluke merged commit 047645b into woowacourse:da-nyee May 21, 2021

+              public class Section {
+                  @NotNull
+                  @Positive
                   private Long id;
+                  @NotNull
+                  @Positive
                   private Long upStationId;
+                  @NotNull
+                  @Positive
                   private Long downStationId;
+                  @NotNull
+                  @Positive
                   private int distance;

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Spring 지하철 노선도 관리 - 3단계] 다니(이다은) 미션 제출합니다. #141

[Spring 지하철 노선도 관리 - 3단계] 다니(이다은) 미션 제출합니다. #141

da-nyee commented May 11, 2021

wbluke left a comment

wbluke May 12, 2021

wbluke May 12, 2021

wbluke May 12, 2021

wbluke May 12, 2021

wbluke May 12, 2021

wbluke May 13, 2021

wbluke May 13, 2021

wbluke May 13, 2021

wbluke May 13, 2021

da-nyee May 18, 2021

da-nyee May 18, 2021

wbluke May 13, 2021

da-nyee commented May 18, 2021

da-nyee commented May 19, 2021

da-nyee commented May 19, 2021

wbluke left a comment

wbluke May 20, 2021

wbluke May 20, 2021

wbluke May 20, 2021

wbluke May 20, 2021

da-nyee commented May 21, 2021

wbluke left a comment


		import static java.util.stream.Collectors.toList;

		public class Sections {

		private final StationService stationService;
		private final SectionDao sectionDao;


		private final SectionRequest sectionRequest = new SectionRequest(2L, 3L, 10);

		@DisplayName("구간을 생성한다.")

		@Service
		@Transactional(readOnly = true)

[Spring 지하철 노선도 관리 - 3단계] 다니(이다은) 미션 제출합니다. #141

[Spring 지하철 노선도 관리 - 3단계] 다니(이다은) 미션 제출합니다. #141

Conversation

da-nyee commented May 11, 2021

wbluke 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

da-nyee commented May 18, 2021

da-nyee commented May 19, 2021

da-nyee commented May 19, 2021

📚 다니의 학습 로그

[Spring] Bean Validation - 3

내용

태그

wbluke 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

da-nyee commented May 21, 2021

wbluke left a comment

Choose a reason for hiding this comment