[디디] 지하철 경로 조회 - TDD 미션 제출합니다. #29
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- backgroundColor 필드 변수 추가
- 응답을 화면에 표시하지 못함
- 프론트에서 staion 이름이 아닌 id로 전달하도록 수정 - createLineStationRequest 를 id로 받도록 수정
- todo 삭제
- 공백 제거 - ControllerAdvice 추가
- 테스트 정상적으로 동작하도록 리팩토링 - data.sql 추가
- 최단 거리 구하는 로직 수정 - Line, Station 생성, 수정 시간 auditing 사용 - line update 로직에 색상 업데이트 추가 - exception 수정, 공백 제거 - Auditing configuration 추가 - test 로직 추가
- 최단 거리 구하는 로직 수정 - Line, Station 생성, 수정 시간 auditing 사용 - line update 로직에 색상 업데이트 추가 - exception 수정, 공백 제거 - Auditing configuration 추가 - transactional 추가 - test 로직 추가
- 해피케이스에 대한 테스트 구현 - 예외사항에 대한 테스트 구현
- GraphService 를 도메인 패키지로 이동 - 프론트 에러 처리방식 변경 - clean up
jihan805
suggested changes
May 16, 2020
src/main/java/wooteco/subway/admin/controller/PathController.java
Outdated
Show resolved
Hide resolved
- path 요청 POST방식에서 GET방식으로 수정 - 시간 관련 데이터 TimeEntity로 통합 - 가독성을 위해 스트림 사이 공백 추가 - 불필요한 생성자 파라미터 삭제 - PathType, GraphResponse˜ 클래스 패키지 이동 - GraphTest 추가 - Algorithm 확장성을 위한 수정 - Algorithm 테스트 추가
jihan805
suggested changes
May 18, 2020
| } | ||
| } | ||
|
|
||
| private PathResult mapToPahtResponse(Long source, Long target, |
There was a problem hiding this comment.
mapToPath (오타인것 같아요!!)
또한 아래 stream을 두번 돌리는 대신 for loop을 사용하는 것도 좋을 것 같아요 !
| private Set<LineStation> stations = new HashSet<>(); | ||
|
|
||
| public Line() { | ||
| } | ||
|
|
||
| public Line(Long id, String name, LocalTime startTime, LocalTime endTime, int intervalTime) { | ||
| public Line(Long id, String name, String backgroundColor, LocalTime startTime, |
There was a problem hiding this comment.
선언된 생성자가 많이 있는데 팩터리 메서드를 사용하는 방법도 잇을 것 같아요 !
| addLineStation(line8.getId(), jamsil.getId(), sukchon.getId(), 1, 10); | ||
|
|
||
| //when | ||
| PathResponse pathByDistance = findPath(jamsil.getName(), samjun.getName(), "distance"); |
There was a problem hiding this comment.
제 경우, 해당 부분에서 에러가 발생했고 에러 로그의 경우 PathResponse 의 기본 생성자가 없기에 발생한 에러였습니다.
추가로 확인 부탁드릴게요~!
- 메서드명 오타 수정 - 스트림을 for loop로 수정 - PathResponse 기본생성자 추가 - Line 생성자를 팩터리 메서드로 변경
jihan805
approved these changes
May 20, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요 리뷰어님! 디디입니다.
이번 미션 진행하면서 궁금했던점들을 정리해보았습니다.
첫번째로 최단경로를 위한 Graph를 만드는 클래스에 대한 질문인데요, Graph를 도메인이라고 봐야할지, 서비스라고 봐야할지, 정확하게 잘 모르겠습니다. 먼저 주어진 기본 코드로는 Service로 제공되었기 때문에 Service라고 하고 구현을 진행했는데요, 마지막에 Graph가 DB에 접근하는 경우도 없고, 실제로 맡은 책임이 도메인에 가까워 보여서 도메인으로 변경하였는데, 맞는 생각을 한건지 확신이 서질 않네요..😂Graph클래스가 도메인이 되는게 맞을까요?
두번째로 Graph를 빈으로 등록하고, 프로그램 로드 시점에 필드로 2가지 타입에 대한 그래프를 생성하여 상태로 가지게 된다면, 클라이언트가 여러명이 되어도 그래프를 다시 그릴 필요 없이 한 번만 그려놓으면 된다고 생각이 들었습니다. 하지만 이 그래프를 사용자들이 동시에 공유(변경)하는 경우? 문제가 발생하지 않을까요? 이런 경우엔 클라이언트 마다 매번 생성하는 방식을 사용하는게 좋을까요?
부족한점이 많은 코드입니다! 많은 피드백 부탁드립니다! 감사합니다!