Week1 step3 #724

Sanghou · 2023-02-06T06:44:09Z

변경내용 요약

Section 기능 추가하기
Section 인수 테스트 추가하기

질문

Service Layer에서 DTO를 반환한다면, 다른 서비스에서 해당 Entity를 같이 사용해야 할 때 어떻게 쓰는게 좋을까요?
DTO.toEntity()는 JPA가 영속성컨텍스트에 보관하는 해당 객체가 아니기 때문에, line.addSection()만으로 바로 저장되지 않아서 결국
lineService에서 해당 변경으로 업데이트 된 line을 저장해주기 위한 lineService.save()를 추가하게 되었습니다.
순환 참조는 어떻게 해결하는게 좋을까요? 순환참조가 발생하지 않는 구조를 만드는게 최선으로 알고 있지만, line과 section이 서로 값을 확인하는 과정에서 영향을 미치는 구조인 것 같아서 @lazy 어노테이션을 이용해서 해결하였습니다. 혹시 튜터님께서는 주로 어떻게 해결하시는지 알 수 있을까요?

catsbi

상호님 어느덧 3단계 미션이네요.
테스트는 잘 작성해주셨어요. 그래서 크게 피드백드릴 부분이 없지만, 테스트 픽스쳐 부분이라거나 애플리케이션 설계 레벨에서 몇 가지 피드백을 남겼으니 확인 후 다시 리뷰요청 해주세요!

질문주신 내용도 코멘트로 바로 남기도록 하겠습니다.

src/main/java/subway/line/Line.java

catsbi · 2023-02-07T04:45:58Z

src/main/java/subway/line/Line.java

+    if (sections.size() != 0 && !Objects.equals(downStation.getId(), add.getUpStation().getId())) {
+      throw new InvalidUpstationAppendInSection();
+    }
+    if (sections.stream().anyMatch(s -> Objects.equals(add.getDownStation().getId(), s.getUpStation().getId()))) {


하나의 분기문에서 수행하기에는 꽤 긴 작업으로 보이는데 메서드로 분리하는게 어떨까요?

add라는 변수명이 구간정보를 표현할 수 있을까요?

객체에서 연산에 필요한 값을 메서드체이닝으로 꺼내고 있네요 디미터법칙에 어긋나지 않을까요?

메서드 체이닝으로인한 디미터의 법칙을 지키지 못하는 문제는
Station에 public class로 equals를 정의해서 equals의 대상으로 id만 쓰고 있다는 정보도 숨겼는데 이런 접근 방식을 시도해보는 것을 말씀해주신 것이 맞을까요?
혹시 다른 방법들이 추가로 있을까요?

Objects.equals(add.getDownStation().getId(), s.getUpStation().getId()) => s.getUpStation().equals(add.getDownStation())

src/main/java/subway/line/Line.java

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

src/main/java/subway/section/SectionResponse.java

catsbi · 2023-02-07T05:05:34Z

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

+import subway.station.StationService;
+
+@Service
+public class SectionService {


엄밀히 말해 구간 서비스는 필요 없는 서비스 계층입니다. 왜일까요?

구간정보는 결국 노선정보에 종속되며 같은 라이프사이클을 가져가는 엔티티라는게 힌트입니다.

아하! 애초에 순환 참조가 발생한 이유도 같이 동작해야하는 로직이었기 때문이겠네요!

이렇게 된다면 LineService에서 Section 저장을 위해서 SectionRepository를 참조하게 될텐데
괜찮은 방법일까요?
구간이라는 개념이 노선에 종속적이기 때문에 구간 조회에 대한 책임도 노선 서비스가 담당하는게 맞을까요?

catsbi · 2023-02-07T05:06:47Z

src/test/java/subway/line/LineTestDTO.java

@@ -0,0 +1,42 @@
+package subway.line;
+
+public class LineTestDTO {


그냥 DTO나 객체를 테스트 목적으로 만들면, 나중에 유지보수 관점에서 프로덕션 코드가 크게 바뀌거나할 때 놓치게 되면 쓸 수 없는 테스트 객체가 되기 쉽습니다.

상속, 위임, 구현 등의 개념을 사용해보는건 어떨까요?

혹시 이 부분에 대해서 조금 더 자세히 알려주실 수 있으실까요?

테스트 픽스쳐를 관리하기위한 방법에 대해서 말씀해주시는 것으로 이해했습니다.
프로덕션에서 쓰이는 dto와 달라서 따로 프로덕션에 정의된 클래스의 상속이나 구현이 어렵다고 판단이 되었는데
(기존 DTO가 쓰던 Station id가 아닌 테스트DTO는 name을 기반으로 받음)
혹시 유연하고 관리하기 쉬우려면 어떻게 사용하는 것이 좋을지 잘 떠오르지 않습니다 😢

이미 이해하셨겠지만 다시 한 번 추가하자면 위 그림과 같이 LineTestDTO와 LineRequest는 내용물은 같지만 완전히 별개의 객체입니다. 그렇기에 유지보수로 프로덕션의 LineRequest가 변경될 때 매번 해당 TestDTO도 신경써주지 않으면 문제가 발생할 수 있죠. 그래서 최대한 LineRequest를 이용해 테스트를 하되, 해당 객체의 내부 값 혹은 로직이 추상화되어 조작할 수 없을 때 해당 객체를 상속 혹은 구현해서 값 자체보다는 행위 를 재정의해서 테스트 가능하게 만드는데 그 목적이 있습니다.

그렇기 때문에 DTO가 station id로 받건 name으로 받는건, 주 관심사가 아닙니다 😅

catsbi · 2023-02-07T05:14:57Z

Service Layer에서 DTO를 반환한다면, 다른 서비스에서 해당 Entity를 같이 사용해야 할 때 어떻게 쓰는게 좋을까요?DTO.toEntity()는 JPA가 영속성컨텍스트에 보관하는 해당 객체가 아니기 때문에, line.addSection()만으로 바로 저장되지 않아서 결국lineService에서 해당 변경으로 업데이트 된 line을 저장해주기 위한 lineService.save()를 추가하게 되었습니다.
1. service layer에서 dto를 반환하지 않아도 된다는 말을 먼저 드리고 싶습니다. 오히려 서비스 계층에서 DTO를 의존하지 않는다면 변화의 양을 더 줄일 수 있겠죠. 더하여 코멘트 남겻지만 addSection이 어디에 위치하는게 좋을지에 대해서도 고민해보면 좋습니다.

순환 참조는 어떻게 해결하는게 좋을까요? 순환참조가 발생하지 않는 구조를 만드는게 최선으로 알고 있지만, line과 section이 서로 값을 확인하는 과정에서 영향을 미치는 구조인 것 같아서 [@lazy](https://github.com/lazy) 어노테이션을 이용해서 해결하였습니다. 혹시 튜터님께서는 주로 어떻게 해결하시는지 알 수 있을까요
1. 보통 대다수의 순환참조 문제는 설계의 문제입니다. 지금 구간과 노선에서 발생하는 문제 역시 설계상의 문제로 순환참조가 발생하는것이기에 코멘트로 남겨드렸구요.

catsbi

상호님 3단계 미션 잘 구현해주셨어요 👍
테스트를 꼼꼼하게 작성해주셔서 코드의 퀄리티가 높아진게 보이네요 ㅎㅎ

몇 가지 피드백 코멘트로 남겼으니 확인 해주시고 질문주신 내용에 대해서도 코멘트로 남겨드리겠습니다. 확인 후 리뷰요청 해주세요!

catsbi · 2023-03-02T12:55:08Z

src/main/java/subway/SubwayExceptionHandler.java

+  @ExceptionHandler(value = IllegalArgumentException.class)
+  public ResponseEntity<Void> handleIllegalStateException(IllegalArgumentException error) {
+    return ResponseEntity.badRequest().build();
+  }
+
+  @ExceptionHandler(value = IllegalStateException.class)
+  public ResponseEntity<Void> handleIllegalStateException(IllegalStateException error) {
+    return ResponseEntity.badRequest().build();
+  }


예외 처리 핸들러를 만드셨네요 👍
그런데 IllegalArgumentException과 IllegalStateException이 같은 동작을 하고 있네요.
두 예외의 차이점이 뭘까요?

catsbi · 2023-03-02T12:55:36Z

src/main/java/subway/line/Line.java

-
-  private Long distance;
+  @Embedded
+  private Sections sections;


이런 컬렉션(with 일급 컬렉션)자료는 필드 초기화를 해주면 NPE에서 좀 더 자유로워 질 수 있습니다 ㅎㅎ

catsbi · 2023-03-02T12:56:30Z

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


 @Service
 public class LineService {

  private final LineRepository lineRepository;
-  private final StationRepository stationRepository;
+  private final StationService stationService;
+  private final SectionRepository sectionRepository;


여기서 순환참조가 발생하셨군요 🤔
제 생각에는 해당 리포지토리를 지울 수 있을 것 같아요!

catsbi · 2023-03-02T12:56:56Z

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

+    Station upStation = stationService.findById(request.getUpStationId());
+    Station downStation = stationService.findById(request.getDownStationId());
+
+    Line created = lineRepository.save(new Line(request.getName(), request.getColor()));
+    createSection(created.getId(), new SectionCreateRequest(upStation.getId(), downStation.getId(), request.getDistance()));
+
+    return LineResponse.from(created);


고민하셨던 부분이 여기군요!
이 부분에 대해 코멘트 남겨드렸으니 확인해주세요!

catsbi · 2023-03-02T12:58:48Z

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

+    return id;
+  }
+
+  // 상행역


불필요한 주석은 제거해주세요!

catsbi · 2023-03-02T12:59:29Z

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

+    return ResponseEntity.ok().body(lineService.createSection(id, request));
+  }
+
+  @DeleteMapping("/lines/{lineId}/sections")


같은 Resource(ex: id)인데 어디선 id 어디선 lineId라고 이름을 다르게 지을 필요가 있을까요?!

catsbi · 2023-03-02T13:06:21Z

src/main/java/subway/sections/SectionsValidator.java

+import subway.section.Section;
+import subway.station.Station;
+
+class SectionsValidator {


검증만을 수행하는 클래스인데 접근제어자를 통해 핸들링하려고 하셨네요 ㅎ

유틸리티 클래스로 보이는데, 인스턴스화 방지를 하면 어떨까 싶네요.

그리고 예외 메세지들이 좀 더 디버깅이 편리하게끔 정보를 담는건 어떨까 싶습니다. [link]

catsbi · 2023-03-02T13:18:00Z

src/test/java/subway/line/LineTestDTO.java

@@ -0,0 +1,42 @@
+package subway.line;
+
+public class LineTestDTO {


이미 이해하셨겠지만 다시 한 번 추가하자면 위 그림과 같이 LineTestDTO와 LineRequest는 내용물은 같지만 완전히 별개의 객체입니다. 그렇기에 유지보수로 프로덕션의 LineRequest가 변경될 때 매번 해당 TestDTO도 신경써주지 않으면 문제가 발생할 수 있죠. 그래서 최대한 LineRequest를 이용해 테스트를 하되, 해당 객체의 내부 값 혹은 로직이 추상화되어 조작할 수 없을 때 해당 객체를 상속 혹은 구현해서 값 자체보다는 행위 를 재정의해서 테스트 가능하게 만드는데 그 목적이 있습니다.

그렇기 때문에 DTO가 station id로 받건 name으로 받는건, 주 관심사가 아닙니다 😅

catsbi · 2023-03-02T13:18:16Z

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

+@DisplayName("구간 관리 기능 테스트")
+@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.DEFINED_PORT)
+@DirtiesContext(classMode = ClassMode.AFTER_EACH_TEST_METHOD)
+public class SectionAcceptanceTest {


테스트가 꼼꼼하게 잘 작성되었네요 💯

catsbi · 2023-03-02T13:19:26Z

Service Layer에서 DTO를 반환한다면, 다른 서비스에서 해당 Entity를 같이 사용해야 할 때 어떻게 쓰는게 좋을까요?
DTO.toEntity()는 JPA가 영속성컨텍스트에 보관하는 해당 객체가 아니기 때문에, line.addSection()만으로 바로 저장되지 않아서 결국
lineService에서 해당 변경으로 업데이트 된 line을 저장해주기 위한 lineService.save()를 추가하게 되었습니다.

=> DTO를 반환의 책임을 컨버터나 컨트롤러에서 책임지도록 하는 방법도 있습니다 ㅎ 상황에 따라 맞추시면 될 것 같네요. 그리고 구간정보를 관리하는 일급컬렉션(sections)가 line.addSection만으로 바로 저장되지 않는다고 하셨는데, 이는 영속성 전이 속성을 사용하시면 가능합니다! 포스팅한 글이 있으니 참고해보세요![link]

순환 참조는 어떻게 해결하는게 좋을까요? 순환참조가 발생하지 않는 구조를 만드는게 최선으로 알고 있지만, line과 section이 서로 값을 확인하는 과정에서 영향을 미치는 구조인 것 같아서 @lazy 어노테이션을 이용해서 해결하였습니다. 혹시 튜터님께서는 주로 어떻게 해결하시는지 알 수 있을까요?

=> 크게 다르지 않습니다 ㅎㅎ 저도 일단 순환참조가 발생하지 않도록 설계하려고 항상 고려하는편이고, 이미 순환참조가 되있는 경우라면 Lazy를 이용하거나 아니면 의존성을 역전시키거나 책임이 적절히 분배되지 않고 부적절한 책임오염이 발생했다고 보고 리팩토링을 하는편입니다.

Sanghou added 10 commits February 2, 2023 03:00

Apply Step2 reviews. Use dto

f9d6204

Add acceptance test spec

0048d9c

Add acceptance test frame

f6c7aac

Add acceptance tests

5719092

Add error handling for section add test

ad987a4

Add sectionCreate. (ask lazy injection)

b7147a1

Add remove test

2d2c49f

Clear format

f38468d

Remove unused constructor

54e05c1

Split Exception and domain

a2176c4

catsbi requested changes Feb 7, 2023

View reviewed changes

Sanghou added 8 commits March 1, 2023 17:02

Change Line Response to API Document

4eb3fe1

Fix Section Spec from getting sectionId to getting stationId

1ed21d6

Section return domain to stationDto

59c4c8e

Add sectionValidator

33eeb02

Add Exception Handler for bad response

7a87a68

Remove Section Service

93a2671

Reduce error logic

32d0bde

Split method chaining

155cf3d

catsbi requested changes Mar 2, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Week1 step3 #724

Week1 step3 #724

Sanghou commented Feb 6, 2023 •

edited

Loading

catsbi left a comment

catsbi Feb 7, 2023

Sanghou Mar 1, 2023

catsbi Feb 7, 2023

Sanghou Mar 1, 2023

Sanghou Mar 1, 2023

catsbi Feb 7, 2023

Sanghou Mar 2, 2023

catsbi Mar 2, 2023

catsbi commented Feb 7, 2023

catsbi left a comment

catsbi Mar 2, 2023

catsbi Mar 2, 2023

catsbi Mar 2, 2023

catsbi Mar 2, 2023

catsbi Mar 2, 2023

catsbi Mar 2, 2023

catsbi Mar 2, 2023

catsbi Mar 2, 2023

catsbi Mar 2, 2023

catsbi commented Mar 2, 2023

		@@ -0,0 +1,42 @@
		package subway.line;

		public class LineTestDTO {

Week1 step3 #724

Are you sure you want to change the base?

Week1 step3 #724

Conversation

Sanghou commented Feb 6, 2023 • edited Loading

변경내용 요약

질문

catsbi 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

catsbi commented Feb 7, 2023

catsbi 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

catsbi commented Mar 2, 2023

Sanghou commented Feb 6, 2023 •

edited

Loading