[2단계 - 경로 조회 기능] 제이미(임정수) 미션 제출합니다. #195
Conversation
보다 적절한 패키지로 이동
- 엔티티 생성으로 변화된 타입 및 로직 수정
- LineResponse와 LineSectionResponse 분리
LineDuplicatedException -> DuplicatedException
README.md
Outdated
| - [ ] 요금 조회 기능 구현 | ||
| - [ ] 경로 조회 시 요금 정보를 포함하여 응답합니다. | ||
| - 요금 계산 방법 | ||
| - 기본운임(10㎞ 이내): 기본운임 1,250원 | ||
| - 이용 거리 초과 시 추가운임 부과 |
There was a problem hiding this comment.
꼼꼼한 요구사항 작성 👏
구현완료된 기능은 체크해주세요 ~
| if (stationDao.findByName(stationRequest.getName()) | ||
| .isPresent()) { |
There was a problem hiding this comment.
exists 쿼리를 통해 한 번에 존재여부 확인을 할 수도 있겠네요 ㅎㅎ
There was a problem hiding this comment.
존재 여부에 대한 확인만 필요하다면 굳이 findBy~를 통해 찾아와 레코드를 가져오고 객체를 생성하는 과정을 수행하는 것보다는 확인만 하고 데이터를 가져오지 않는 방법의 성능이 더 낫겠네요!
좋은 팁 감사합니다!!
| private List<Line> findLines() { | ||
| List<LineEntity> lineEntities = lineDao.findAll(); | ||
| return lineEntities.stream() | ||
| .map(lineEntity -> | ||
| new Line(lineEntity.getId(), lineEntity.getName(), lineEntity.getColor(), | ||
| new Sections(sectionDao.findSectionsByLineId(lineEntity.getId())) | ||
| ) | ||
| ) | ||
| .collect(Collectors.toList()); | ||
| .map(lineEntity -> findLineById(lineEntity.getId())) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
lineDao에서 findAll을 하고 다시 그 결과에서 id를 꺼내 findById 쿼리를 한 번 더 요청하도록 하는 이유가 있나요?
DB 재요청없이 LineEntity > Line으로 변환만 해줘도 될 것 같아요
There was a problem hiding this comment.
맞습니다..!
Line을 만들 때 Sections를 생성할 때 Section도 DB에서 가져와야 해 바로 findLineById()를 통해 가져오도록 한 것 같습니다.
그래서 Sections를 생성하는 코드를 메서드로 분리해 해당 메서드를 호출하는 방식으로 수정했습니다!
| private void validateDuplicateLine(LineRequest request) { | ||
| Optional<LineEntity> optionalLineByName = lineDao.findByName(request.getName()); | ||
| optionalLineByName.ifPresent(findUser -> { | ||
| throw new LineDuplicatedException("이미 존재하는 노선 이름입니다."); | ||
| throw new DuplicatedException("이미 존재하는 노선 이름입니다."); | ||
| }); | ||
| Optional<LineEntity> optionalLineByColor = lineDao.findByColor(request.getColor()); | ||
| optionalLineByColor.ifPresent(findUser -> { | ||
| throw new LineDuplicatedException("이미 존재하는 노선 색입니다."); | ||
| throw new DuplicatedException("이미 존재하는 노선 색입니다."); |
There was a problem hiding this comment.
꼼꼼한 중복검증 좋네요 👍🏻 findBy 대신 exists 쿼리를 통해 존재여부를 한 번에 확인할 수도 있습니다 ~
| public ResponseEntity<LineSectionResponse> findLineById(@PathVariable Long id) { | ||
| return ResponseEntity.ok( | ||
| lineService.findLineResponseById(id) | ||
| ); | ||
| } | ||
|
|
||
| @PutMapping("/{id}") |
There was a problem hiding this comment.
아래 코멘트가 안달려서 여기 남길게요 :)
LineController 54-57번 라인의 SQLException 핸들러 함수는 (미션 뼈대코드로 보이는데)
GlobalHandler로 이동시키지 않고 여기에 남겨둔 이유가 있나요?
여기서 잡혀서 메시지 없이 400 코드만 오는 경우가 있는 것 같아요 ~
There was a problem hiding this comment.
아뇨…!
Exception 핸들러들을 옮긴다고 했는데 미처 옮기지 못한 핸들러가 있었네요..
해당 핸들러도 GlobalHandler로 이동시켜 두었습니다.
| public SectionEntity(Long id, Long lineId, Long upStationId, Long downStationId, Integer distance) { | ||
| this.id = id; | ||
| this.lineId = lineId; | ||
| this.upStationId = upStationId; | ||
| this.downStationId = downStationId; | ||
| this.distance = distance; | ||
| } | ||
|
|
||
| public SectionEntity(Long lineId, Long upStationId, Long downStationId, Integer distance) { | ||
| this.lineId = lineId; | ||
| this.upStationId = upStationId; | ||
| this.downStationId = downStationId; | ||
| this.distance = distance; | ||
| } |
There was a problem hiding this comment.
감사합니다!
생성자 체이닝에 대해 처음 알게 되었네요.
생성자 체이닝으로 나타낼 수 있는 부분들(LineEntity, SectionEntity)에 적용해 보았습니다!
src/main/java/subway/domain/Map.java
Outdated
| import org.jgrapht.graph.DefaultWeightedEdge; | ||
| import org.jgrapht.graph.WeightedMultigraph; | ||
|
|
||
| public class Map { |
There was a problem hiding this comment.
Map은 Java에서 제공하는 컬렉션 이름이기 때문에 클래스명으로 사용하면 혼동을 줄 수 있습니다
There was a problem hiding this comment.
맞습니다..!
SubwayMap으로 수정했습니다.
| private Section(Long id, Station upStation, Station downStation, Integer distance) { | ||
| this.id = id; | ||
| this.upStation = upStation; | ||
| this.downStation = downStation; | ||
| this.distance = distance; | ||
| } | ||
|
|
||
| public static Section from(SectionStationMapper sectionStationMapper) { |
There was a problem hiding this comment.
정적 팩토리 메서드와 생성자를 잘 활용하시는 것 같아요 💯
| @Override | ||
| public String toString() { | ||
| return "Section{" + | ||
| "upStation=" + upStation + | ||
| ", downStation=" + downStation + | ||
| ", distance=" + distance + | ||
| '}'; | ||
| } |
There was a problem hiding this comment.
모든 도메인 클래스에 toString을 오버라이딩 하시는 이유가 있나요?
There was a problem hiding this comment.
만약 toString을 오버라이딩 하지 않는다면 객체를 출력했을 대 해시코드가 출력됩니다.
하지만 toString을 재정의한다면 테스트 및 디버깅을 수행할 때 toStirng으로 설정해 둔 것과 동일하게 내부 필드들의 값을 한 번에 확인할 수 있습니다.
이번 미션에서 테스트 및 디버깅 수행 시 toString을 설정하지 않아 불편했었기 때문에 모든 도메인에 toStirng을 오버라이딩하여 사용하게 되었습니다.
| private void addSectionBasedOnUpStation(Section sectionToAdd, Station upStation) { | ||
| Optional<Section> findSection = sections.stream() | ||
| .filter(section -> section.isUpStation(upStation)) | ||
| .findFirst(); | ||
| if (findSection.isPresent()) { | ||
| Section originalSection = findSection.get(); | ||
| Station downStationIdOfOrigin = originalSection.getDownStation(); | ||
| Station downStationIdOfToAdd = sectionToAdd.getDownStation(); | ||
| Integer revisedDistance = findRevisedDistance(sectionToAdd, originalSection); | ||
| Section revisedSection = Section.of(originalSection.getId(), downStationIdOfToAdd, downStationIdOfOrigin, revisedDistance); | ||
| sections.remove(originalSection); | ||
| sections.add(sectionToAdd); | ||
| sections.add(revisedSection); | ||
| return; | ||
| } | ||
|
|
||
|
|
||
| sections.stream() | ||
| .filter(section -> section.isDownStation(upStation)) | ||
| .findFirst() | ||
| .ifPresent(section -> sections.add(sectionToAdd)); |
There was a problem hiding this comment.
구간 관련 로직에 레벨 1에서 학습한 내용이 잘 지켜지지 않고 있어요 😢
- 함수(또는 메서드)의 길이가 10라인을 넘어가지 않도록 구현한다.
- 함수(또는 메소드)가 한 가지 일만 하도록 최대한 작게 만들어라.
There was a problem hiding this comment.
이번 미션에서 특히 잘 지켜지지 못한 부분이네요...
구현이 바빠질수록 기본적인 것에 대해 지키지 못하는 것 같아요 😥
말씀해 주신 부분이 제대로 적용되지 못한 부분들에 대해 리팩토링을 통해 메서드 분리 등을 수행했습니다..!
Map -> SubwayMap
|
안녕하세요 바다! 피드백 주신 내용에 대해 적용하고 답은 코멘트로 달아두었습니다! 의문점1. Repository를 두는 것이 좋은가?확장성을 생각한다면 respointory를 두는 것이 더 좋을 것입니다. 하지만 repository를 사용했을 때의 단점은 추상화 단계가 한 단계 더 생겨 복잡성이 증가하게 될 것이라 생각합니다. 그런데 언제부터 repoistory를 만들어 사용해야 하는 가에 대한 기준을 아직 잡기가 어려운 상태입니다. 2. 정책에 대해 전략 패턴을 사용하는 것이 좋은가?현재 저는 정책이 하나이기 때문에 굳이 interface를 만들어 전략 패턴을 고려하는 것이 불필요하다고 생각했습니다. 하지만, 확장성을 생각해 전략이 추가될 수 있다고 생각한다면, interface를 사용해 전략 패턴을 고려하는 것이 좋을 것이라 생각합니다. 실제로 구현은 하지 못했지만 3단계 미션은 나이와 노선에 대한 요금 정책을 추가하는 것이었습니다. 3. 인수 테스트와 통합 테스트현재 통합 테스트에서 DB에 값이 제대로 들어 갔음에 대해 이미 단위테스트에서 이루어졌다 판단해 생략하게 되었습니다. 또한, 인수 테스트는 일련의 시나리오, 통합 테스트는 단위 테스트들의 연결이 잘 되는지에 대한 검증이라 생각합니다. 테스트는 각자 불안하지 않을 정도로 하면 된다고 듣기는 했지만 바다의 기준이 궁금해 여쭤봅니다! |
| if (lineDao.isExistId(lineId)) { | ||
| return; | ||
| } | ||
| throw new NotFoundException("해당 노선이 존재하지 않습니다."); |
There was a problem hiding this comment.
반대의 경우를 확인하는 함수(isNotExistId)를 정의해줘도 좋을 것 같아요 ~
| public SectionEntity(Long lineId, Long upStationId, Long downStationId, Integer distance) { | ||
| this(null, lineId, upStationId, downStationId, distance); |
| private final NamedParameterJdbcTemplate jdbcTemplate; | ||
| private final SimpleJdbcInsert insertAction; | ||
|
|
||
| private RowMapper<LineEntity> rowMapper = (rs, rowNum) -> | ||
| private final RowMapper<LineEntity> rowMapper = (rs, rowNum) -> |
There was a problem hiding this comment.
- Repository를 두는 것이 좋은가?
확장성을 생각한다면 respointory를 두는 것이 더 좋을 것입니다.
이번 미션에서 Service는 dao와 domain 관련된 로직이 섞여 있는 상태입니다.
Repository를 조사해 보았을 때 repository는 db에 좀 더 가까워 dao 관련 로직 및 entity를 domain으로 변환해 주는 등에 대한 로직을 옮길 수 있으리라 생각합니다.
또한, 검증과 같이 중복되는 코드도 존재하기에 해당 부분들을 줄일 수 있다는 장점이 있습니다.
하지만 repository를 사용했을 때의 단점은 추상화 단계가 한 단계 더 생겨 복잡성이 증가하게 될 것이라 생각합니다.
그런데 언제부터 repoistory를 만들어 사용해야 하는 가에 대한 기준을 아직 잡기가 어려운 상태입니다.
바다의 경우 언제 repository가 있어야 한다고 생각하시나요?
이미 dao가 db 관련 로직을 분리해서 담고 있는데 또 repository를 분리해야 하느냐에 대한 질문이신거죠?
정답은 아니지만 제 생각을 말씀드리면 이번 미션에서는 dao와 repository를 모두 구현할 필요는 없을 것 같아요
지금 단계에서는 두 가지를 굳이 구분하려하거나 책임을 나누려하기 보다는
이 클래스들을 service 레이어에서 분리한 이유는 무엇인지, 어떤 책임을 주고 싶은지 에 대해서만
학습하고 넘어가면 좋을 것 같습니다 ~
| @@ -5,11 +5,11 @@ | |||
| import org.jgrapht.graph.DefaultWeightedEdge; | |||
| import org.jgrapht.graph.WeightedMultigraph; | |||
|
|
|||
| public class Map { | |||
| public class SubwayMap { | |||
There was a problem hiding this comment.
- 정책에 대해 전략 패턴을 사용하는 것이 좋은가?
현재 저는 정책이 하나이기 때문에 굳이 interface를 만들어 전략 패턴을 고려하는 것이 불필요하다고 생각했습니다.
또한, 불필요한 인터페이스를 생성함으로써 클래스의 수가 늘어나 비용이 증가한다고 생각했습니다.
하지만, 확장성을 생각해 전략이 추가될 수 있다고 생각한다면, interface를 사용해 전략 패턴을 고려하는 것이 좋을 것이라 생각합니다.
실제로 구현은 하지 못했지만 3단계 미션은 나이와 노선에 대한 요금 정책을 추가하는 것이었습니다.
이와 같이 전략이 추가될 것을 고려해 인터페이스를 만들어 두는 것이 좋을까요?
또한, 실제로 현업에서도 전략 패턴을 많이 사용하시는지 궁금합니다!
저도 제이미의 의견에 대부분 동의를 하는데요.
한 가지 경우만 있는 상황에서 나중에 어떤 케이스가 추가될지 모르는데 미리 추상화를 해둔다는 건 거의 불가능하다고 생각합니다
추상화는 최소 두 개 이상의 경우가 있어서 그들의 공통점을 말할 수 있을 때 하는 것이 좋다고 생각해요
현업에서도 구현하는 사람마다 각자의 취향대로 구현하기 때문에 정해진 정답은 없습니다 ㅎㅎ
| @DisplayName("최단 경로에 대한 역 목록과 가격을 반환해준다") | ||
| @Test | ||
| void findShortestPath() throws Exception { |
There was a problem hiding this comment.
- 인수 테스트와 통합 테스트
현재 통합 테스트에서 DB에 값이 제대로 들어 갔음에 대해 이미 단위테스트에서 이루어졌다 판단해 생략하게 되었습니다.
그래도 한 번 더 테스트 수행 시 db에 값이 제대로 들어갔는지 등에 대해 확인해주는 것이 좋을까요?
또한, 인수 테스트는 일련의 시나리오, 통합 테스트는 단위 테스트들의 연결이 잘 되는지에 대한 검증이라 생각합니다.
이때, 시나리오가 어느정도여야 하는지 헷갈리는 상태입니다.
시나리오라고 하면 노선 생성 -> 역 생성 -> 구간 생성과 같이 일련의 작업이 있어야 하는 걸까요?
테스트는 각자 불안하지 않을 정도로 하면 된다고 듣기는 했지만 바다의 기준이 궁금해 여쭤봅니다!
사실 이 마지막 말이 정답이고 위에 고민들에 대한 답을 알고 계신 것 같아 저의 기준에 대해서만 말씀드려볼게요
우테코 과정에서는 사용자가 시도할 수 있는 케이스들에 대해서는 대부분 시나리오 테스트를 하려고 했던 것 같아요
ex. 회원가입 > 로그인 > 목록 조회 > 단건 조회 > 상품 담기 > 장바구니 이런식으로 순서대로
동작이 잘 되는지를 체크하기 위한 테스트들을 진행했습니다
지하철 미션 같은 경우는 노선생성 > 노선조회, 노선생성 > 역 생성 > 구간 생성 > 경로조회 와 같은 시나리오들이 있을 것 같아요
현업에 와서는 모든 케이스에 대해서 테스트는 사실 힘들긴 합니다 😂
또, QA와 같이 테스트만을 위한 인력도 따로 계시기 때문에 시나리오 테스트에 시간을 쏟기 보다는
단위테스트를 더 꼼꼼하게 한다던가 성능적으로 개선할 부분에 더 신경을 쓰게 되는 것 같아요
안녕하세요 바다!
2단계 경로 조회 기능 PR 요청드립니다.
1단계 머지가 빨리된 만큼 2단계 pr 요청을 드리고 싶었는데 생각보다 늦어졌네요 😭
1단계 리팩토링을 수행하면서 서비스의 로직을 도메인으로 넘기는 것 등에 대해 고민들을 수행해 보았습니다.
이 과정에서 생각보다 시간이 많이 흘러 2단계 요청을 마지막 날에야 요청드리게 되었습니다.
늦게 요청했음에도 아직 많이 부족한 코드인 상태인데 2단계 리뷰 잘 부탁드립니다..!
* 테스트의 경우 단위 테스트만 진행한 상태로 통합, 인수 테스트도 추가할 예정입니다.