3단계 - 구간 추가 기능 과제 PR 요청 드립니다! #1013
Conversation
- 정상적으로 동작하지 않는 앞으로 작성할 인수테스트 메소드 템플릿 작성
- 하행역부터 시작해서 중간에 새로운 역을 등록하는 구간 등록 기능 구현 - 상행역부터 시작해서 중간에 새로운 역을 등록하는 구간 등록 기능 구현 - 구간 등록 및 확인의 간편함을 위해 Section 내 순서 값 추가 (sequence) - 우선 구간 등록 후 순서만을 확인하는 테스트 코드 작성
- 프론트엔드 영역 실행 후 정상 동작 테스트 진행 - 추가 작업으로 프로젝트 전역에서 사용하는 dto를 request 라는 명칭으로 변경 - response와 통일성을 맞추고, 기존 dto와 request를 병행해서 사용하는 것을 막기 위함
- 역을 등록하는 로직을 실행하기 전 한번에 전체 거리를 계산해서 초과되는지 동일한지 여부를 결정하도록 개선 - 역을 등록하기 위해 거리를 하나씩 더하거나 빼면서 내부 반복마다 검증하는 부분을 제거하여 성능 개선 및 오류가 발생할 수 있는 여지 제거 -
- 이전 커밋의 검증 로직 개선으로 인해 발생한 오류 수정.
- 현 시점에서 새로운 역 등록시 거리를 표현하는 기능은 외부로 제공하지 않기 때문에 (클라이언트상) 우선 해당 테스트 조건은 제거함. - 현재 각 조건마다 초과 조건에 대해 HttpStatus Code로 인지 가능
There was a problem hiding this comment.
이직하고 정말 바쁘셨을 텐데 돌아와서 꿋꿋하게 과제를 수행해주셨군요!
보통 이직에 성공하면 학습 열정도 식기 마련인데 정말 대단하십니다 😁
이번 과제에 주어진 요구사항이 생각보다 되게 복잡해요.
개인 스케줄도 바쁜데 요구사항의 난이도가 높은 편이라 수강생 분들의 발목을 잡기도 하죠 ㅠㅠ
그래서 더욱 고민이 많으셨을 것 같아요.
몇 가지 의견을 남겨보았는데요,
제 피드백이 taewonh님의 고민을 줄여드릴 수 있었으면 좋겠네요.
확인 후 다시 요청 부탁드립니다.
| @@ -1,6 +1,6 @@ | |||
| package nextstep.subway.line; | |||
|
|
|||
| class CreateLineDto { | |||
| class CreateLineRequest { | |||
There was a problem hiding this comment.
빈 생성자가 없어 일부 테스트에서 오류가 발생합니다.
아마 실제로 실행을 하더라도 이 클래스를 사용하는 곳에선 전부 오류가 날거에요.
@RequestBody 가 어떤 방식으로 생성되는지를 공부해보시면 도움이 됩니다.
| @@ -17,8 +18,8 @@ public LineController(LineService lineService) { | |||
| } | |||
|
|
|||
| @PostMapping("/lines") | |||
There was a problem hiding this comment.
이 클래스에서 /lines 가 공통으로 사용됩니다.
클래스 레벨의 @RequestMapping 으로 묶어주는건 어떨까요?
| Station upStation = findStationById(dto.getUpStationId()); | ||
| Station downStation = findStationById(dto.getDownStationId()); | ||
| Distance distance = new Distance(dto.getDistance()); | ||
| public LineResponse registerSection(long id, CreateSectionRequest request) { |
There was a problem hiding this comment.
이 메소드도 단위 테스트를 추가하는건 어떤가요?
로직이 그리 어렵진 않지만 작동 여부 자체는 테스트가 필요해보여요.
| public void replace(Section newSection, Distance totalSectionDistance) { | ||
| Distance excludeSectionDistance = totalSectionDistance.subtract(distance); | ||
| newSection.distance = newSection.distance.subtract(excludeSectionDistance); | ||
| newSection.upStation = upStation; | ||
| newSection.sequence = sequence; | ||
| upStation = newSection.downStation; | ||
| distance = distance.subtract(newSection.distance); | ||
| } |
| public List<Map<String, Object>> getStationsResponse() { | ||
| return getStations().stream() | ||
| .map(Station::toMapForOpen) | ||
| .collect(Collectors.toList()); | ||
| } |
There was a problem hiding this comment.
엔티티에서 response를 아는건 영역 위반입니다.
레이어드 아키텍처 기반의 코드는 표현 -> 서비스 -> 도메인 -> 영속 순으로 흐르는게 좋아요.
직접적으로 반환하진 않아도 아예 모르는게 더 적절합니다.
여기서는 차라리 Section 에 속한 Station 목록을 반환한 것으로 대체할 수 있지 않을까 싶어요.
| } | ||
|
|
||
| public List<Map<String, Object>> getStations() { | ||
| private boolean addFirstSection(Section section) { | ||
| if (sections.size() == 0) { |
There was a problem hiding this comment.
| if (sections.size() == 0) { | |
| if (sections.isEmpty()) { |
isEmpty 가 조금 더 의미 전달에 좋지 않을까요?
|
|
||
| private boolean extendDownSection(Section section) { | ||
| int sectionLength = sections.size(); | ||
| if (sections.get(sectionLength - 1).isExtendDownStation(section)) { |
There was a problem hiding this comment.
이 부분을 메소드화하면 코드 이해에 도움이 많이 될 것 같습니다.
종점역인가? 를 적절한 메소드로 표현해보시는건 어떤가요?
| private void sort() { | ||
| sections.sort(Comparator.comparingInt(Section::getSequence)); | ||
| } | ||
|
|
||
| public void add(Section section) { |
There was a problem hiding this comment.
로직이 복잡한 이유 중 하나가 매번 if 문에서 boolean을 내뱉고 return 하는 구조라서 그런 것 같아요.
첫 로직만 보더라도 "비어 있는 Sections면 Sectino을 그대로 삽입한다" 가 조금 더 자연스러워 보입니다.
그리고 action-validation-sort-action 순으로 가고 있는데요,
validation을 먼저 해도 전혀 이상하지 않겠네요.
new ArrayList() 로 시작해서 빈 배열이어도 NPE가 나지 않습니다.
안녕하세요!
이번 3단계 구간 추가 기능 과제가 완료되어 PR 요청드립니다!
이번 과제에서 구간을 새로 추가할 때 추가 가능한 조건과 검증해야 하는 코드가 너무너무 복잡해서 시간이 너무 오래 걸려버렸네요.. 죄송합니다.. 거기다 최근에 다른 회사로 이직하게 되면서 시간도 너무 부족했었구요..
다음 과제부터는 조금 빠르게 피드백 반영하고 제출하도록 하겠습니다..!