Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[1단계 - 지하철 정보 관리 기능] 히이로(문제웅) 미션 제출합니다. #93

Merged
merged 26 commits into from
May 20, 2023

Conversation

MoonJeWoong
Copy link

@MoonJeWoong MoonJeWoong commented May 11, 2023

안녕하세요 현구막, 이번 지하철 미션 리뷰이로 함께하게 된 히이로입니다!

이번 페어 미션을 진행하면서 최우선순위로 진행했던 부분은 레벨1과 같은 도메인의 설계였습니다. spring 프레임워크의 기능이 개입되지 않은 순수한 도메인을 설계하고 그 외적인 부분들이 도메인에 맞추도록 하는 방식으로 진행하고 싶었습니다. 이러한 목적을 가지고 도메인 객체를 설계하고 구현하는 과정은 TDD를 통해 어느정도 잘 진행할 수 있었던 것 같습니다.

문제는 어느 정도 도메인을 완성하고 계층형 구조 설계를 고려하기 시작했을 때부터 발생했습니다. 일단 처음에 설계했던 API의 URI들이 도메인과 전혀 맞지 않았던 것 같습니다. 처음 API를 설계할 때는 노선을 나타내는 line을 최상위 URI 계층으로 생각하고 진행했었으나 실제로 도메인 로직을 수정하면서 설계하다보니 많이 변경되어야 했습니다. 그 이유가 뭔지에 대해 생각해보니 도메인 로직 내부에서 line이 아니라 section을 주된 객체 단위로 사용하다 보니 처음의 설계가 많이 바뀌었던 것 같습니다.

이 외에도 웹 API 제공을 위한 도메인 외적인 코드들을 작성하면서 겪은 어려움은 다음과 같습니다.

  • 클라이언트 입장에서는 '역(station)'을 주된 객체 단위로 사용하는데 저희의 도메인 설계에서는 '구간(section)'이라는 객체 단위가 주된 단위로 사용되기 시작하면서 어려움이 발생했습니다.
  • 이렇게 '구간'이 도메인에서 주된 객체 단위로 사용되기 시작하면서 지하철 노선에 역이 추가될 때 상행 종점과 하행 종점에 대한 관리가 굉장히 어려워졌던 것 같습니다.
  • 또한 다음과 같이 서비스에서 비즈니스 로직을 수행하는 순서를 지키고 싶었으나 시간 상 문제로 굉장히 날리면서 개발을 진행하게 되었던 것 같습니다.
    • 최대한 서비스에서 Persistence 로직을 통해 기존 도메인 객체들을 불러온다.
    • 해당 도메인 객체들을 통해 도메인 로직을 수행한다.
    • 도메인 로직을 수행한 뒤의 도메인 객체들의 상태를 Persistence 로직을 통해 저장한다.
    • 해당 부분들은 리팩토링을 진행하며 다시 수정해보도록 하겠습니다... 😭
  • 그리고 테스트 관련해서 이번에 통합테스트밖에 작성하지 못한게 많이 아쉽습니다... 리팩토링 과정 진행하면서 단위 테스트에 대한 고민을 해보면서 진행해보겠습니다...!

여러모로 너무 시간에 쫓기면서 구현한터라 아쉬움이 많이 남은 미션이 되었습니다... 현구막이 보기 너무 불편하실 것 같아 미리 양해를 구하고 싶습니다... 🙇 아무쪼록 이번 리뷰 잘 부탁드리겠습니다...!

@Hyeon9mak Hyeon9mak self-requested a review May 11, 2023 09:59
Copy link
Member

@Hyeon9mak Hyeon9mak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 히이로, 지하철 미션 구현 고생하셨어요.
리팩터링 과정에서 레벨1 때 연습한 객체지향 생활체조 규칙들을 다시 적용해보면 좋겠어요.
현재 코드 가독성을 개선하기 위한 부분을 중점적으로 코멘트 남겼어요.
비즈니스 로직을 잘 수행하기 위한 설계에 대한 부분은 리팩터링 후 다시 이야기 나눠보면 좋겠어요.

문제는 어느 정도 도메인을 완성하고 계층형 구조 설계를 고려하기 시작했을 때부터 발생했습니다.

복잡한 도메인을 코드로 풀어내는 과정에서 당연히 겪는 문제라고 생각합니다.
잘 시도해주셨고 "이렇게 도메인을 풀려고 하면 어렵구나." 를 기억하시면 될 것 같아요.

API의 URI들이 도메인과 전혀 맞지 않았던 것 같습니다.

여기서 이미 깨달으셨을 것 같은데요 ㅎㅎㅎ,
지하철역 애플리케이션을 통해 하고 싶은게(use-case) 무엇인지 파악하지 않고
현실 세상 속 내가 알고 있는 지하철 노선 관련 구조에 따라 모델 설계를 끝낸 후
use-case 를 거기에 끼워맞추려해서 생겨난 문제 같아요.

요구사항에 맞춰서 도메인 모델을 나눠야 어느정도 밸런스가 잡힐텐데,
이미 만들어놓은 도메인 모델로 요구사항을 해결하려다보니까
특정 모델의 역할이 비대해진 것 같아요.

또한 현재 요구사항에 존재하지 않는 API 들이 많이 생겨났는데요,
비즈니스 요구사항을 기술적으로 풀어내는게 기술자가 할 일인데
기술(설계)이 어려워서 요구사항을 맞추지 못하는 상황은 없어야 할 것 같아요.
이 부분은 꼭 리팩터링 해주세요.

얼마 전에 동욱님 발표를 듣고 정리한 내용이 있는데, 딱 이번 미션에 어울리는 내용 같아요.
https://hyeon9mak.github.io/avoid-dependencies-on-uncontrollable-factors/#%EC%9D%BC%EC%A0%95%EC%9D%80-%EC%A7%80%ED%82%A4%EC%A7%80%EB%A7%8C-%EB%B2%84%EA%B7%B8%EA%B0%80-%EB%A7%8E%EC%9D%80-%EA%B2%83-vs-%EC%9D%BC%EC%A0%95%EC%9D%80-%EB%AA%BB-%EC%A7%80%ED%82%A4%EC%A7%80%EB%A7%8C-%EB%B2%84%EA%B7%B8%EA%B0%80-%EC%97%86%EB%8A%94-%EA%B2%83

궁금하신 점 DM, 코멘트 남겨주세요 🙇‍♂️

README.md Outdated
Comment on lines 46 to 58
- [x] 역마다 연결된 구간 정보를 관리한다.
- [x] 새로운 구간을 생성한다.
- [x] 노선에 처음 구간이 추가될 때는 `null - 상행 종점`, `상행 종점 - 하행 종점`, `하행 종점 - null` 세 구간이 추가된다.
- [x] 노선에 역이 추가될 때는 `상행 방향 역 - 하행 방향 역`구간이 `상행 방향 역 - 새로운 역`과 `새로운 역 - 하행 방향 역`으로 나뉜다.
- [x] 구간 사이에 역이 추가될 때, `구간 내 두 역과 새로운 역 사이 거리 각각의 합 != 구간의 기존 거리` 인 경우 예외처리한다.
- [x] 역을 포함하는 구간들을 조회한다.
- [x] 기존 구간을 삭제한다.
- [x] 노선에서 역이 제거될 때는 `상행 방향 역 - 제거할 역`과 `제거할 역 - 하행 방향 역` 두 구간이 `상행 방향 역 - 하행 방향 역`으로 합쳐진다.
- [x] 제거되는 역과 상행 방향 역, 하행 방향 역 사이의 거리의 합이 합쳐진 구간에 저장되어야 한다.
- [x] 노선에 남은 마지막 역 두개를 제거할 때는 `null - 상행 종점`, `상행 종점 - 하행 종점`, `하행 종점 - null` 세 구간이 삭제된다.
- [x] 제거 처리 이후 구간이 두개만 남은 경우, 모든 구간을 삭제한다.
- [x] 노선을 구성하는 구간들을 반환한다.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

레벨1 규칙 잊지 않고 요구사항 정리하기 좋아요~!

Comment on lines 31 to 41
@PostMapping("/init-sections")
public ResponseEntity<Void> createInitSections(@Valid @RequestBody InitSectionRequest request) {
sectionService.saveInitSections(request);
return ResponseEntity.status(HttpStatus.CREATED).build();
}

@PostMapping("/section")
public ResponseEntity<Void> createSection(@Valid @RequestBody SectionRequest request) {
sectionService.saveSection(request);
return ResponseEntity.status(HttpStatus.CREATED).build();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요구사항에 주어진 API 는 아래와 같아 보이는데요.

  • 노선에 역 등록 API 신규 구현
  • 노선에 역 제거 API 신규 구현
  • 노선 조회 API
  • 노선 목록 조회 API

현재 구조는 백엔드가 해결해야할 로직을 프론트엔드에게 넘긴 상태네요. 😱

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 어디까지가 프론트의 책임이고 백엔드의 책임인지 조금은 모호하게 느껴지는 것 같습니다. 일반적으로 팀 회의를 통해 결정해서 팀마다 그 기준이 상이할 것이라 생각하고 있었습니다만 현구막이 리뷰해주신 것을 보고 궁금한 점이 생겼습니다!
    • 이번에 페어와 미션을 진행하면서 프론트 입장에서는 이미 어떤 역들이 어떤 순서로 노선을 구성하는지에 대한 정보를 알고 있는 상태로 역에 대한 작업을 수행할 것이라 생각했습니다.
    • 그래서 삽입/삭제 하고자 하는 역이 중간에 있는 역인지, 종점인지, 노선에 처음 추가되는 역인지 알고 요청을 할 수 있다고 가정했기에 처음 설계했던 API 구조가 나왔습니다.
    • 그런데 현구막은 처음 API를 봤을 때 어떤 포인트에서 백엔드의 책임이 프론트의 책임으로 넘겨둔 것이라고 느꼈는지 그 이유에 대해 질문드리고 싶습니다!

Copy link
Author

@MoonJeWoong MoonJeWoong May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음 리뷰를 받고 위에 댓글을 단 것처럼 생각을 했었습니다. 다만 리팩토링을 거치면서 이번 미션의 요구사항을 클라이언트가 (상행 방향 역, 하행 방향 역, 역간 거리) 정보를 담아서 노선에 역 등록 요청을 할 수 있어야 한다고 재정리 했습니다. LMS 위쪽 텍스트만 봤을 때는 클라이언트 요청 요구사항이 나와있지 않다고 생각했었는데 하단에 그림을 보니 이런 방식으로 요구사항을 정리할 수 있었던 것 같습니다.

클라이언트에서는 노선에 역을 등록 / 제거하기 위한 최소한의 정보만을 담아서 요청하도록 하고 등록 / 제거될 역이 중간역인지 종착역인지 판단하는 비즈니스 로직들을 모두 도메인에서 수행해서 작업을 처리할 수 있도록 했습니다!

Comment on lines +8 to +10
@RestControllerAdvice
public class GlobalExceptionHandler {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오~! 다른 controller 에 남아있는 exceptionHandler 도 옮겨줄 수 있겠네요. 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 뼈대코드를 제대로 확인하지 못했어서 ExceptionHandler 코드가 있는지 몰랐었네요 ㅎㅎ
해당 코드는 GlobalExceptionHandler로 옮겨서 처리할 수 있도록 했습니다!

Comment on lines 12 to 18
@Service
public class LineService {
private final LineDao lineDao;
private final H2LineDao lineDao;

public LineService(LineDao lineDao) {
public LineService(H2LineDao lineDao) {
this.lineDao = lineDao;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 지하철 노선에도 트랜잭션 관리가 필요해보이네요!
  • 지하철 노선 서비스 계층이 DB 종류를 알 필요가 있을까요? DB 가 변경되면 서비스 계층 코드도 함께 변경되겠네요 😱

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 현재 존재하는 Service 클래스에는 @transactional 어노테이션을 붙여줄 수 있도록 했습니다. ㅎㅎ
  • 엇... 기존 뼈대코드에서도 Dao를 인터페이스 타입으로 받도록 수정하려고 했었는데 누락했던 것 같습니다... 😭 해당 부분은 인터페이스 타입으로 받도록 수정했습니다!

Comment on lines 61 to 66
List<Section> newSections = sections.addSection(
newStation,
originalSection,
request.getUpwardDistance(),
request.getDownwardDistance()
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sections 일급 컬렉션에 새로운 section 을 추가했는데, List<Section> 이 나오는 흐름이 잘 이해되지 않아요.. ㅠㅠ

Copy link
Author

@MoonJeWoong MoonJeWoong May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 로직은 Sections 도메인 객체를 이용해 노선에 이미 존재하는 구간 사이에 역을 등록하는 과정을 수행하는 service 메서드입니다. 리팩토링을 거치면서 아예 판단하는 로직이 바뀌었긴 했는데 과거의 기억을 더듬어서 정리해봤습니다 ㅎㅎ...

  • 새로운 역이 사이에 등록되어야 할 구간을 찾아서 이를 이용해 Sections 도메인 인스턴스 생성
  • addSection 메서드를 통해 기존 구간을 쪼개서 기존 상행방향 역 - 신규 등록 역, 신규 등록 역 - 기존 하행방향 역새로운 두 개의 구간을 반환
  • 현구막이 이해하기 힘들다고 말씀해주신 로직 부분이 아마 여기 였던 것 같아요... ㅎㅎ

시간에 쫓기면서 하다보니 도메인 내부에서 완벽하게 수행된 이후의 도메인 상태에 대해 영속화 과정을 하는 방식을 고수하지 못하고 Service 단에서 이를 처리하려고 했었습니다. 또한 객체의 책임 분리 문제와 단일 메서드가 명령, 조회를 동시에 수행하는 문제 등이 겹쳐서 이런 혼종 코드가 탄생하게 되었던 것 같습니다. ㅎㅎ 😭

Comment on lines 5 to 10
public class Line {
private Long id;
private String name;
private String color;
private final Long id;
private final String name;
private final String color;

public Line() {
}

public Line(String name, String color) {
private Line(Long id, String name, String color) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

원시값을 모두 포장해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 도메인 객체에서 가지는 primitive 타입 property의 경우 포장 객체를 만들어서 대체했습니다. 동시에 가벼운 검증 로직 책임을 부여해볼까 했는데 워낙 역 이름과 색깔 등이 다양해서 해당 부분은 보류했습니다 ㅎㅎ 😄

@@ -38,11 +46,12 @@ public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
Line line = (Line) o;
return Objects.equals(id, line.id) && Objects.equals(name, line.name) && Objects.equals(color, line.color);
return Objects.equals(name, line.name) && Objects.equals(color, line.color);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

identifier 를 가진 엔티티 객체는 identifier 를 통해 나머지 정보들의 변화를 추적하는데요,
이 때문에 identifier 만 일치하면 서로 같은 데이터로 봐야할 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 line, station, section 등 전반적인 도메인에 걸쳐서 프로퍼티로 identifier를 가지고 있습니다.

도메인 객체를 설계하면서, 기존 데이터들이 저장되어 있는 상황에서 신규 데이터 생성 요청이 들어왔을 때 이에 대한 중복 검증을 어떻게 수행해야 할까를 많이 고민했습니다. 고민이 되었던 포인트는 클라이언트의 신규 생성 요청에 담긴 정보로 만든 도메인 객체는 아직 DB에 저장된 상태가 아니기에 id값이 null인 상태일 수 밖에 없다는 것이었습니다.

만약 이 상황에서 id를 equals의 기준으로 판단하도록 구현이 되어 있다면 다른 값들이 모두 같아서 중복 검증이 되어야 함에도 되지 않는 문제가 발생할 수 있을 것이라고 판단했습니다.

또한 비즈니스 규칙 상에서 판단하기에 id가 아니더라도 line 같은 경우 노선의 이름과 색깔이 같으면 동일한 객체로서 취급되어야 한다고 판단했습니다.

위와 같은 이유들로 id가 아닌 다른 property들이 동일하다면 같은 객체로 판단할 수 있게끔 equals를 재정의 했었습니다.

혹시 제가 판단한 논리중에 잘못되거나 놓친 부분이 있을까요? 생각을 정리하면서 equals 자체는 identifier로 판단하게끔 정의하고 비즈니스 규칙과 관련된 중복 검증을 위해 필요한 비교 기능을 따로 도메인 객체의 메서드로 만들어줬어야 했을까 하는 생각도 들었던 것 같습니다.

해당 부분은 현구막의 의견을 여쭙고 수정하고 싶어 일단은 놔두고 추후 수정해보도록 하겠습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일단 당장 생각이 드는 방법은 equals를 id 비교를 통해 우선적으로 판단하게끔 하고, 두 객체 중 한쪽이라도 id가 null이라면 비즈니스 로직 상 해당 객체들을 구분지을 수 있는 property들을 비교해 동등성을 비교하는 것인데 생각해본 코드를 같이 첨부해보겠습니다!

public boolean equals(Object o) {
        if (this == o) return true;
        if (o == null || getClass() != o.getClass()) return false;
        Line line = (Line) o;
        if (this.id != null && line.id != null) {
            return this.id.equals(line.id);
        }
        return Objects.equals(name, line.name) && Objects.equals(color, line.color);

line 도메인의 equals 메서드를 위 생각대로 정의해본 코드입니다. 현재 line과 station Service의 경우 기존 뼈대코드 그대로를 사용했기에 클라이언트의 신규 생성 요청을 Dao에 그대로 저장을 하고 있습니다. 하지만 직접 구현한 Section의 경우 반드시 한 번 도메인 객체화 과정을 거치고 영속화 과정을 거치도록 구현했습니다. 그렇기에 클라이언트의 신규 요청 정보로 생성된 도메인 객체에 대해서는 id가 아닌 다른 프로퍼티를 이용한 equals 비교 기능이 필요하다고 판단해 이런 방식으로 예시코드를 생각해봤습니다!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

도메인에 걸쳐서 프로퍼티로 identifier 를 가지고 있습니다.

도메인은 히이로가 프로그래밍을 통해 해결해야하는 문제(비즈니스 요구사항)을 의미한다고 생각하는데요,
도메인이 프로퍼티를 가졌다는게 어떤 느낌인지 잘 모르겠네요.. 😰 히이로가 생각하시는 도메인이 무엇인지
조금 더 구체적으로 설명해주시면 좋겠어요.

"다른 값들이 모두 같아서 중복 검증이 되어야 함" 은 비즈니스 로직 같아요.
엔티티를 관리하기 위한 id 동등성 부여와 비즈니스 정책에 의한 중복 검증이 동일시 될 필요가 있을까요?
id 를 통한 엔티티의 동등성을 비교하는 것과는 별개로,
중복을 검증해야하는 프로퍼티에 대해서만 해당 로직이 수행되면 될 것 같다는 생각이 드네요.

도메인 / 엔티티 / VO / DTO 단어에 대한 정의를 이야기해보면 좋겠어요.

Comment on lines 18 to 22
public void add(Line line) {
validateDuplicatedName(line);
validateDuplicatedColor(line);
lines.add(line);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일급 컬렉션 활용을 통한 검증 로직 관리 👍
앞으로 "노선을 추가할 때 중복되는 노선 이름도 허용되게 해주세요." 라고 요구사항이 변경되면
Lines#add 메서드만 확인하면 되겠네요.

Comment on lines 40 to 42
public Integer getDistance() {
return distance;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Integer 와 int 사용이 병행되는 것 같은데요, 통일해주면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

시간에 쫓겨서 정신없이 진행하다 보니 DTO에서 자료형 통일이 안된 부분이 있었네요... 😭
이번에 도메인 로직 수정하면서 DTO에 전반적인 수정이 있었고 해당 과정에서 DTO의 자료형은 Wrapper타입으로 통일하도록 했습니다!

부차적인 리팩토링 사항이긴 하지만 SectionEntity의 프로퍼티 타입은 id를 제외하고 항상 존재해야 하는 값이기에 primitive 타입을 사용하도록 같이 수정했습니다.

Comment on lines +9 to +24
@DisplayName("Distance는 ")
class DistanceTest {

@Test
@DisplayName("거리 정보를 갖는다.")
void distanceTest() {
assertDoesNotThrow(() -> Distance.from(3));
}

@Test
@DisplayName("거리 정보가 양수가 아닐 경우 예외처리한다.")
void invalidDistanceExceptionTest() {
assertThatThrownBy(() -> Distance.from(-1))
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("[ERROR] 거리는 음수 일 수 없습니다.");
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TDD 가 잘 지켜졌던 흔적이 보이네요 😄 👍

Copy link
Author

@MoonJeWoong MoonJeWoong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 현구막! 히이로입니다.

리뷰를 빠르게 해주셨음에도 불구하고 이것 저것 많이 생각하면서 진행하다보니 알아보거나 수정해야 할 부분들이 계속 많아져서 리뷰 요청이 많이 늦어졌습니다... 😭

전반적으로 도메인 비즈니스 로직을 재설계하고 전체적인 코드의 가독성을 최대한 높이기 위해 노력했던 리팩토링 기간이었습니다. 또한 각 계층에 대한 단위 테스트를 진행해보고 싶어서 이것 저것 많이 찾아보고 적용해보려고 했는데 들인 시간에 비해 결과가 많이 나오지 못한 것 같아 아쉽습니다... 해당 부분들은 다음 리팩토링 과정에서 다뤄보겠습니다!

리팩토링 과정을 거치면서 추가적으로 발생했던 질문들이 있어 같이 정리해서 PR 요청 드립니다. 감사합니다!

Comment on lines 31 to 41
@PostMapping("/init-sections")
public ResponseEntity<Void> createInitSections(@Valid @RequestBody InitSectionRequest request) {
sectionService.saveInitSections(request);
return ResponseEntity.status(HttpStatus.CREATED).build();
}

@PostMapping("/section")
public ResponseEntity<Void> createSection(@Valid @RequestBody SectionRequest request) {
sectionService.saveSection(request);
return ResponseEntity.status(HttpStatus.CREATED).build();
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 어디까지가 프론트의 책임이고 백엔드의 책임인지 조금은 모호하게 느껴지는 것 같습니다. 일반적으로 팀 회의를 통해 결정해서 팀마다 그 기준이 상이할 것이라 생각하고 있었습니다만 현구막이 리뷰해주신 것을 보고 궁금한 점이 생겼습니다!
    • 이번에 페어와 미션을 진행하면서 프론트 입장에서는 이미 어떤 역들이 어떤 순서로 노선을 구성하는지에 대한 정보를 알고 있는 상태로 역에 대한 작업을 수행할 것이라 생각했습니다.
    • 그래서 삽입/삭제 하고자 하는 역이 중간에 있는 역인지, 종점인지, 노선에 처음 추가되는 역인지 알고 요청을 할 수 있다고 가정했기에 처음 설계했던 API 구조가 나왔습니다.
    • 그런데 현구막은 처음 API를 봤을 때 어떤 포인트에서 백엔드의 책임이 프론트의 책임으로 넘겨둔 것이라고 느꼈는지 그 이유에 대해 질문드리고 싶습니다!

Comment on lines 6 to 14
@JsonNaming(PropertyNamingStrategies.KebabCaseStrategy.class)
public class SectionDeleteRequest {

private Long stationId;
private Long lineId;

public SectionDeleteRequest() {

}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

참조해주신 레퍼런스를 확인하고 request DTO 클래스의 기본 생성자를 전부 제거하려고 했습니다. 그런데 수정하지 않은 뼈대코드 부분인 StationRequest가 기본생성자가 없으면 뼈대코드 테스트가 실패하는 이슈가 있었습니다. 결국 기본 생성자를 제거하고 @JsonCreator 어노테이션을 사용해 해결하였으나 정확한 원인이 무엇인지는 파악하기 힘들었습니다... 😭

Comment on lines 6 to 14
@JsonNaming(PropertyNamingStrategies.KebabCaseStrategy.class)
public class SectionDeleteRequest {

private Long stationId;
private Long lineId;

public SectionDeleteRequest() {

}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

KebabCase로 요청을 받아야 한다는 요구사항은 없었습니다. 다만 지난 번 미션에서도 그렇고 일반적으로 웹 응답/요청간에 CamelCase를 잘 사용하지 않는다고 어디서 들었던 경험이 있어서 이렇게 작성했습니다... ㅎㅎ 일단 요구사항이 없었던 부분이었기에 다시 CamelCase를 사용하는 것으로 수정하였습니다!

Comment on lines 40 to 42
public Integer getDistance() {
return distance;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

시간에 쫓겨서 정신없이 진행하다 보니 DTO에서 자료형 통일이 안된 부분이 있었네요... 😭
이번에 도메인 로직 수정하면서 DTO에 전반적인 수정이 있었고 해당 과정에서 DTO의 자료형은 Wrapper타입으로 통일하도록 했습니다!

부차적인 리팩토링 사항이긴 하지만 SectionEntity의 프로퍼티 타입은 id를 제외하고 항상 존재해야 하는 값이기에 primitive 타입을 사용하도록 같이 수정했습니다.

@@ -38,11 +46,12 @@ public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
Line line = (Line) o;
return Objects.equals(id, line.id) && Objects.equals(name, line.name) && Objects.equals(color, line.color);
return Objects.equals(name, line.name) && Objects.equals(color, line.color);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 line, station, section 등 전반적인 도메인에 걸쳐서 프로퍼티로 identifier를 가지고 있습니다.

도메인 객체를 설계하면서, 기존 데이터들이 저장되어 있는 상황에서 신규 데이터 생성 요청이 들어왔을 때 이에 대한 중복 검증을 어떻게 수행해야 할까를 많이 고민했습니다. 고민이 되었던 포인트는 클라이언트의 신규 생성 요청에 담긴 정보로 만든 도메인 객체는 아직 DB에 저장된 상태가 아니기에 id값이 null인 상태일 수 밖에 없다는 것이었습니다.

만약 이 상황에서 id를 equals의 기준으로 판단하도록 구현이 되어 있다면 다른 값들이 모두 같아서 중복 검증이 되어야 함에도 되지 않는 문제가 발생할 수 있을 것이라고 판단했습니다.

또한 비즈니스 규칙 상에서 판단하기에 id가 아니더라도 line 같은 경우 노선의 이름과 색깔이 같으면 동일한 객체로서 취급되어야 한다고 판단했습니다.

위와 같은 이유들로 id가 아닌 다른 property들이 동일하다면 같은 객체로 판단할 수 있게끔 equals를 재정의 했었습니다.

혹시 제가 판단한 논리중에 잘못되거나 놓친 부분이 있을까요? 생각을 정리하면서 equals 자체는 identifier로 판단하게끔 정의하고 비즈니스 규칙과 관련된 중복 검증을 위해 필요한 비교 기능을 따로 도메인 객체의 메서드로 만들어줬어야 했을까 하는 생각도 들었던 것 같습니다.

해당 부분은 현구막의 의견을 여쭙고 수정하고 싶어 일단은 놔두고 추후 수정해보도록 하겠습니다!

Comment on lines 31 to 41
@PostMapping("/init-sections")
public ResponseEntity<Void> createInitSections(@Valid @RequestBody InitSectionRequest request) {
sectionService.saveInitSections(request);
return ResponseEntity.status(HttpStatus.CREATED).build();
}

@PostMapping("/section")
public ResponseEntity<Void> createSection(@Valid @RequestBody SectionRequest request) {
sectionService.saveSection(request);
return ResponseEntity.status(HttpStatus.CREATED).build();
}
Copy link
Author

@MoonJeWoong MoonJeWoong May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음 리뷰를 받고 위에 댓글을 단 것처럼 생각을 했었습니다. 다만 리팩토링을 거치면서 이번 미션의 요구사항을 클라이언트가 (상행 방향 역, 하행 방향 역, 역간 거리) 정보를 담아서 노선에 역 등록 요청을 할 수 있어야 한다고 재정리 했습니다. LMS 위쪽 텍스트만 봤을 때는 클라이언트 요청 요구사항이 나와있지 않다고 생각했었는데 하단에 그림을 보니 이런 방식으로 요구사항을 정리할 수 있었던 것 같습니다.

클라이언트에서는 노선에 역을 등록 / 제거하기 위한 최소한의 정보만을 담아서 요청하도록 하고 등록 / 제거될 역이 중간역인지 종착역인지 판단하는 비즈니스 로직들을 모두 도메인에서 수행해서 작업을 처리할 수 있도록 했습니다!

Comment on lines 12 to 18
@Service
public class LineService {
private final LineDao lineDao;
private final H2LineDao lineDao;

public LineService(LineDao lineDao) {
public LineService(H2LineDao lineDao) {
this.lineDao = lineDao;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 현재 존재하는 Service 클래스에는 @transactional 어노테이션을 붙여줄 수 있도록 했습니다. ㅎㅎ
  • 엇... 기존 뼈대코드에서도 Dao를 인터페이스 타입으로 받도록 수정하려고 했었는데 누락했던 것 같습니다... 😭 해당 부분은 인터페이스 타입으로 받도록 수정했습니다!

Comment on lines 61 to 66
List<Section> newSections = sections.addSection(
newStation,
originalSection,
request.getUpwardDistance(),
request.getDownwardDistance()
);
Copy link
Author

@MoonJeWoong MoonJeWoong May 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 로직은 Sections 도메인 객체를 이용해 노선에 이미 존재하는 구간 사이에 역을 등록하는 과정을 수행하는 service 메서드입니다. 리팩토링을 거치면서 아예 판단하는 로직이 바뀌었긴 했는데 과거의 기억을 더듬어서 정리해봤습니다 ㅎㅎ...

  • 새로운 역이 사이에 등록되어야 할 구간을 찾아서 이를 이용해 Sections 도메인 인스턴스 생성
  • addSection 메서드를 통해 기존 구간을 쪼개서 기존 상행방향 역 - 신규 등록 역, 신규 등록 역 - 기존 하행방향 역새로운 두 개의 구간을 반환
  • 현구막이 이해하기 힘들다고 말씀해주신 로직 부분이 아마 여기 였던 것 같아요... ㅎㅎ

시간에 쫓기면서 하다보니 도메인 내부에서 완벽하게 수행된 이후의 도메인 상태에 대해 영속화 과정을 하는 방식을 고수하지 못하고 Service 단에서 이를 처리하려고 했었습니다. 또한 객체의 책임 분리 문제와 단일 메서드가 명령, 조회를 동시에 수행하는 문제 등이 겹쳐서 이런 혼종 코드가 탄생하게 되었던 것 같습니다. ㅎㅎ 😭

Comment on lines 89 to 93
public void saveEndSection(EndSectionRequest request) {
Line line = lineDao.findById(request.getLineId());
Station originalEndStation = stationDao.findById(request.getOriginalEndStationId());
Station newStation = stationDao.findById(request.getStationId());

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SectionsService 에서 중구난방으로 수행하던 비즈니스 로직 관련 기능들을 전부 Sections 도메인 객체 내부에서 수행하도록 변경하면서 서비스 코드는 상대적으로 엄청 가벼워질 수 있었습니다. 아마 전체적으로 날림 코드였어서 Service 내부에서 객체지향적으로 코드를 작성해보라는 의도로 해주신 리뷰일 것이라고 이해를 했습니다.

리뷰로 정해주신 코드 구간들은 request dto를 이용해 도메인 객체들을 load 하는 부분입니다. 이 부분에서는 어쩔 수 없이 getter를 사용해야 한다고 생각했는데 혹시 위에서 제가 적은대로 전체적으로 객체지향을 고려해보라는 의도가 아니셨다면 어떤 부분에서 고려해보라고 말슴해주신 건지 한 번 더 여쭤보고 싶습니다!

Comment on lines 267 to 278
public List<LineStationsResponse> readAllStationsOfAllLines() {
List<Line> lines = lineDao.findAll();
List<LineStationsResponse> lineStationsResponses = new ArrayList<>();

for (Line line : lines) {
List<StationResponse> stationResponses = readAllStationsOfLine(line.getId());
LineStationsResponse lineStationsResponse = new LineStationsResponse(DtoMapper.convertToLineResponse(line), stationResponses);
lineStationsResponses.add(lineStationsResponse);
}

return lineStationsResponses;
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분은 서비스에서 판단하고 수행하던 비즈니스 로직 코드들을 Sections 도메인 객체 내부로 넣음으로써 해결할 수 있었습니다. 만약 기존 코드처럼 서비스가 굉장히 복잡한 상황에선는 말씀해주신 부분처럼 지하철 역, 구간 생성 / 제거 / 조회 기능들을 분리하는 것이 좋을 것 같습니다.

하지만 리팩토링 과정을 거치며 SectionService 코드가 굳이 분리를 안해도 될 정도로 굉장히 가벼워졌다고 생각했습니다. 그래서 Service의 각 기능들을 분리하는 것 보다는 Section에 대한 기능을 한 곳에 집약시켜두는 것이 더 장점이 크다고 판단했습니다.

혹시 제가 잘못 생각한 부분이 있거나 수정해야 할 부분이 있다면 알려주시면 감사하겠습니다! 😄

세부사항

- persistence 계층 로직 수행 시 사용하는 값 검증 기능 구현 (null checker)
- 존재하지 않는 역, 노선 조회 시 optional로 반환 후 값이 존재하지 않거나
  null일 때 예외 처리
- 존재하지 않는 역, 노선 조회 시 발생시킬 커스텀 예외 구현
- 예외 처리 관련 테스트 코드 작성
import java.util.Set;
import java.util.stream.Collectors;

public class Sections {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번 미션에서 복잡했던 것은 노선에 역을 등록하는 로직이였다고 생각이 듭니다. 이 신규 역 등록 로직을 차근차근 다시 분리하며 정리해보니 아래와 같이 굉장히 절차지향적으로 정리가 되었습니다. 그런데 이런 절차지향적인 코드를 객체지향으로 풀어내는 것이 정말 감이 안잡히는 것 같습니다.

일단 Sections 도메인 객체 내부에서 이 모든 비즈니스 로직들을 처리하도록 구현한 뒤에 메서드 분리를 시도해봤으나 그렇게 맘에 들 정도로 정리가 되지는 않았던 것 같습니다. 결국 신규 역 등록에 대한 절차지향적인 로직을 객체지향으로 풀어내는 것이 어려워서 계속 코드가 정리가 안되는 느낌이 드는 것 같습니다. 이럴 때 개발자는 어떤 방법으로 접근해봐야 할까요?

  • 새로운 구간을 생성한다.
    • 노선에 처음 역이 추가되는 경우
      • 상행 종점 - 하행 종점으로 구간이 생성된다.
    • 노선의 기존 역을 기준으로 추가되는 경우
      • 상행 방향 역이 신규역인 경우
        • 구간 사이에 역이 추가될 때
          • 하행 방향 역이 포함된 해당 노선의 구간을 찾아서 두 구간으로 나눠준다.
            • 신규역 - 기존 하행 방향 역, 기존 상행 방향 역 - 신규역 구간을 추가한다.
            • 기존 상행 방향 역 - 기존 하행 방향 역 구간을 삭제한다.
          • 종착 구간 마지막에 역이 추가될 때
            • 상행 방향 역이 포함된 해당 노선의 구간을 찾아서 두 구간으로 나눠준다.
              • 기존 상행 방향 역 - 신규역, 신규역 - 기존 하행 방향 역 구간을 추가한다.
              • 기존 상행 방향 역 - 기존 하행 방향 역 구간을 삭제한다.
          • 새로운 역과 기존 역 사이 거리 >= 구간의 기존 거리 인 경우 예외처리한다.
      • 하행 방향 역이 신규역인 경우
        • 구간 사이에 역이 추가될 때
          • 기존 하행 방향 역이 포함된 노선 구간이 1개인지 확인해서 상행 종착 구간인지 확인한다.
          • 상행 종착 구간임이 확인되면 입력된 신규역 - 기존 상행 종착 역 구간을 추가한다.
        • 종착 구간 마지막에 역이 추가될 때
          • 기존 상행 방향 역이 포함된 노선 구간이 1개인지 확인해서 하행 종착 구간인지 확인한다.
          • 하행 종착 구간임이 확인되면 입력된 기존 하행 종착 역- 신규역 구간을 추가한다.
        • 새로운 역과 기존 역 사이 거리 >= 구간의 기존 거리 인 경우 예외처리한다.
      • 상행역과 하행역이 이미 노선에 존재하는 경우 예외처리한다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

각 기능을 객체(클래스)로 분리할 히이로만의 기준을 확립해보면 어떨까요?
저는 객체지향을 하는 이유가 지금 보기 좋은 코드, 좋은 설계를 위해서보다 나중에 기능을 변경하거나 유지보수할 때
바라보고 이해해야 할 할 코드의 수를 줄이기 위해서 한다고 생각해요.
그렇지 않으면 로직 하나를 수정해야 할 때마다 연관(의존)된 모든 클래스 파일을 이해해야하는, 살아있는 레거시가 되기 떄문이에요.
결국 유지보수성을 위해서 객체지향을 지향해요.

그런데 한편으로는 절차지향적인 코드를 객체지향적으로 꼭 변경해야하는지 고민이 필요해보여요.
현재 절차적인 검증 로직들은 유지보수에 어려움이 큰가요?
전체적으로 검증 플로우를 이해하는데 더 도움이 되거나 하진 않나요?
어쩌면 절차지향적인 코드가 객체지향보다 유지보수에 뛰어날 때가 있지 않을까요?
https://youtu.be/dJ5C4qRqAgA?t=4795

Comment on lines +28 to +32
@PostMapping
public ResponseEntity<Void> createSection(@Valid @RequestBody SectionRequest request) {
sectionService.saveSectionInLine(request);
return ResponseEntity.status(HttpStatus.CREATED).build();
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SectionController에서 Post 명령을 수행할 때 생성된 구간의 id를 포함시켜서 CreatedURI를 메세지 헤더에 담아서 보내는 것이 일반적이라 생각이 되었습니다. 하지만 비즈니스 로직 상 section이 한 번의 요청으로 여러개 생성되어 버리는 경우는 어떻게 해야 하는지가 궁금했습니다.

지금처럼 도메인 로직 상 CreatedURI를 반환하지 않거나 다른 리소스의 URI를 반환하는 경우도 있는지 궁금합니다. 만약 후자처럼 다른 리소스의 URI를 반환한다면 지하철 노선의 전체 역을 조회할 수 있는 URI를 반환하게 시킬 수도 있겠다고 생각은 드는데 현구막은 어떻게 생각하시나요?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어쩔 땐 1개, 어쩔땐 2개를 반환하는 경우는 멱등성을 지키지 못하는 API 가 된다고 생각해요.
"라인에 역을 추가해서 구간도 함께 생성된다." 라는 API 가 존재할 때 결국 중요한 건 해당 구간 정보를 확인할 수 있는 경로를 제공해주는 건데,
그럴 때 역으로 정보를 제공하기 어렵다면 역과 구간 정보를 포함한 라인 정보를 반환해줄 수 있는 API 경로를 명시하는 것도 좋겠네요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 답변 감사합니다! 구간 생성 요청으로 새로운 구간이 추가된 노선의 전체 역들을 조회할 수 있는 URI를 헤더에 담아 반환할 수 있도록 수정했습니다!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이번 리팩토링 과정에서 컨트롤러, 서비스, 영속성 계층에 대한 테스트를 작성해보려고 했습니다. 그런데 서비스 계층에 대한 테스트는 제가 Bottom - Up 방식으로 도메인과 영속성 계층부터 구현을 하다보니 이미 서비스 계층에서 수행해야 할 기능 테스트가 이미 도메인 테스트에서 수행되었다고 판단이 들어서 넘어가기로 결정했습니다.

컨트롤러 테스트는 통합 테스트에서 대부분의 기능이 검증되고 있다고 판단해서 따로 @Valid 어노테이션을 통한 검증 기능이 제대로 동작하는지에 대한 테스트만 작성해보고자 했습니다.

영속성 계층 테스트는 작성해보고 싶었으나 해당 부분까지 테스트를 작성하기엔 시간이 너무 오래 걸릴 것 같다고 판단했습니다. 다음 리팩토링 과정에서 작성해보도록 하겠습니다!

Copy link
Member

@Hyeon9mak Hyeon9mak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 히이로, 1단계 피드백 적용 고생하셨어요.
질문주신 내용에 최대한 히이로가 생각을 더 해보실 수 있도록 답변을 남겼어요.
혹시나 답변이 불충분하다면 추가로 코멘트, DM 남겨주세요 🙇‍♂️

더 읽기 쉬운 코드, 유지보수하기 좋은 코드를 만들기 위한 여러가지 고민이 많아지는 미션 같아요.
동료 개발자를 설득할 수 있는 히이로만의 기준이 만들어지는 미션이 되면 좋겠네요!
다음 단계도 화이팅입니다 💪

Comment on lines +41 to +51
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
SectionEntity that = (SectionEntity) o;
return lineId == that.lineId
&& Objects.equals(id, that.id)
&& Objects.equals(upwardId, that.upwardId)
&& Objects.equals(downwardId, that.downwardId)
&& Objects.equals(distance, that.distance);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +19 to +23
@ExceptionHandler
public ResponseEntity<ErrorResponse> handleIllegalArgumentException(IllegalArgumentException exception) {
return ResponseEntity.badRequest().body(new ErrorResponse(exception.getMessage()));
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예상치 못한 예외도 함께 처리해줄 수 있으면 좋겠네요. (Exception.class)
이 때 사용자에게는 너무 자세한 서버 상태를 노출하지 않도록 커스텀한 메세지를 전달해주는게 좋겠어요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰해주신대로 Exception.class에 대한 ExceptionHandler를 추가하여 예상치 못한 예외 상황에 대해 InternalServerError를 반환하도록 수정했습니다!

Comment on lines +13 to +20
@RestController
@RequestMapping("/line-stations")
public class LineStationsController {

private final SectionService sectionService;

public LineStationsController(SectionService sectionService) {
this.sectionService = sectionService;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

서버 입장에서 프론트의 책임을 고민해줄 필요가 있을까요?
현재 프로젝트에 프론트 외 다른 클라이언트(모바일, TV, 차량용 등)가 추가될 때마다 책임을 고민해주어야 하는 걸까요? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분은 어떤 의도로 짚어주셨는지 제가 잘 이해를 못한 것 같습니다... 😢 LineStationController는 노선 별로 포함하는 역들에 대한 URI 리소스와 기존 노선 정보를 나타내는 lines URI 리소스를 분리하기 위한 의도로 작성했었습니다. 프론트의 책임을 고려하고 분리했던 의도는 없었는데 이 부분에 대해 조금만 더 상세한 설명을 요청드리고 싶습니다...! 🙇

import java.util.Objects;

public class Line {
private final Long id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id 를 포장하면 어떤 장점이 생길까요?


public class Sections {

public static final int MIDDLE_SECTION_SIZE = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

내부에서만 사용되는 값 같아요.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 인텔리제이의 상수화 기능을 사용하다 보니 미처 신경쓰지 못한 것 같습니다. 외부에서 사용하지 않는 상수들은 private화 했습니다!

Comment on lines +268 to +270
public int countOfStations() {
return sectionsByStation.keySet().size();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

만약 "구간을 추가할 때 역과 역사이 거리가 50km 를 넘어가면 추가되지 못하도록 검증로직을 넣어주세요." 라는 요구사항이 전달되면,
현재 클래스에서는 271 라인을 모두 확인해야겠네요. 😱 유지보수 하기 쉬운 코드와 단일 책임 원칙이란 무엇일까요?

Copy link
Author

@MoonJeWoong MoonJeWoong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

예전에 답변 코멘트를 달아뒀었는데 submit을 안해뒀었네요... 😅

Comment on lines +28 to +32
@PostMapping
public ResponseEntity<Void> createSection(@Valid @RequestBody SectionRequest request) {
sectionService.saveSectionInLine(request);
return ResponseEntity.status(HttpStatus.CREATED).build();
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 답변 감사합니다! 구간 생성 요청으로 새로운 구간이 추가된 노선의 전체 역들을 조회할 수 있는 URI를 헤더에 담아 반환할 수 있도록 수정했습니다!


public class Sections {

public static final int MIDDLE_SECTION_SIZE = 2;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 인텔리제이의 상수화 기능을 사용하다 보니 미처 신경쓰지 못한 것 같습니다. 외부에서 사용하지 않는 상수들은 private화 했습니다!

Comment on lines +19 to +23
@ExceptionHandler
public ResponseEntity<ErrorResponse> handleIllegalArgumentException(IllegalArgumentException exception) {
return ResponseEntity.badRequest().body(new ErrorResponse(exception.getMessage()));
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리뷰해주신대로 Exception.class에 대한 ExceptionHandler를 추가하여 예상치 못한 예외 상황에 대해 InternalServerError를 반환하도록 수정했습니다!

Comment on lines +13 to +20
@RestController
@RequestMapping("/line-stations")
public class LineStationsController {

private final SectionService sectionService;

public LineStationsController(SectionService sectionService) {
this.sectionService = sectionService;
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 부분은 어떤 의도로 짚어주셨는지 제가 잘 이해를 못한 것 같습니다... 😢 LineStationController는 노선 별로 포함하는 역들에 대한 URI 리소스와 기존 노선 정보를 나타내는 lines URI 리소스를 분리하기 위한 의도로 작성했었습니다. 프론트의 책임을 고려하고 분리했던 의도는 없었는데 이 부분에 대해 조금만 더 상세한 설명을 요청드리고 싶습니다...! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants