[2단계 - 경로 조회 기능] 저문(유정훈) 미션 제출합니다. #205
Conversation
|
|
||
| @Transactional(readOnly = true) |
There was a problem hiding this comment.
실수 방지용으로 좋겠네요!
혹시 Transactional(readOnly=true) 설정의 유무에 따라 Service의 동작방식이 바뀔까요? ❓
There was a problem hiding this comment.
읽기 전용인 경우에는 데이터베이스에 lock을 걸지 않는 것으로 알고 있습니다.
따라서 성능적으로 약간?의 이득을 볼 수 있지 않을까하는 생각이 듭니다!
There was a problem hiding this comment.
넵 이후에 lock 에 대해서 더 공부해보시면서 더 고민해보시면 좋을 것 같아요 :)
| public void createSection(SectionCreateRequest request) { | ||
| Long lineId = request.getLineId(); | ||
| Line line = findLine(lineId); | ||
|
|
||
| public void createSection(final SectionCreateRequest request) { | ||
| Station leftStation = findStation(request.getLeftStationName()); | ||
| Station rightStation = findStation(request.getRightStationName()); | ||
| Distance distance = new Distance(request.getDistance()); | ||
| Section section = new Section(leftStation, rightStation, distance); | ||
| Section section = new Section(leftStation, rightStation, request.getDistance()); | ||
| Line line = findLine(request.getLineId()); | ||
|
|
||
| line.addSection(section); | ||
| sectionRepository.deleteAllByLineId(lineId); | ||
| sectionRepository.saveAllByLineId(lineId, line.getSections()); | ||
| sectionRepository.save(line); | ||
| } |
There was a problem hiding this comment.
서비스 분리(패키지 분리) 관점에서 질문드려요!
Line 과 Section을 별개로 분리해주셨는데요! 만약 LineService 안에 구현하는 것과 지금의 구현은 어떤 장단점이 있을까요?
There was a problem hiding this comment.
현재의 방식대로 분리했을 경우는 line에 대한 로직과 section에 대한 로직이 확실하게 구분되어 보인다는 장점이 있다고 생각합니다.
하나의 Service에 존재할 경우 여러가지 메서드들이 구현되어 있어 로직을 쉽게 판단하기 어렵다고 생각했습니다.
그러나 Line이 Section을 가지고 있다는 관점에서 생각하면 LineService에서 section에 대한 로직을 가지고 있는 것이 자연스러울 것 같기도 합니다.
지금까지는 구분감을 주기 위해 서비스를 분리하여 유지하는 것도 괜찮다고 판단하여 SectionService를 통해서 관리를 하였는데요.
완태께서는 어떻게 생각하시는지 궁금합니다!
There was a problem hiding this comment.
둘 다 가능할 것 같아요!
만약 제가 구현한다면, 지금의 귬에서는 저는 Line을 도메인의 루트로 두고 코드를 작성할 것 같다는 생각은 드네요 👍
There was a problem hiding this comment.
말씀해주신 방법의 이유도 충분히 이해가 됐습니다!
저는 코드를 작성하면서 LineService와 SectionService가 분리되어 있어
테스트 및 프로덕션 코드를 작성할 때 해당되는 부분을 찾아가기가 훨씬 쉽다고 느꼈습니다.
다만 완태께서 말씀해주신 방법의 이유를 충분히 알고, 두가지 방법 중 하나를 택한다면 둘 다 가능하다고 말씀하신 것처럼 선택이 될 것 같습니다.
따라서 이 부분은 그대로 유지하고 싶습니다..!
| public PathResponse findPath(final PathRequest request) { | ||
| Station fromStation = findStation(request.getFromStationName()); | ||
| Station toStation = findStation(request.getToStationName()); | ||
|
|
||
| WeightedMultigraph<Station, DefaultWeightedEdge> graph = makeGraph(); | ||
|
|
||
| DijkstraShortestPath<Station, DefaultWeightedEdge> dijkstraShortestPath = new DijkstraShortestPath<>(graph); | ||
| int pathDistance = (int) dijkstraShortestPath.getPathWeight(fromStation, toStation); | ||
| int fare = Fare.from(pathDistance).getValue(); | ||
| List<StationInfo> stationInfos = getStationInfos(fromStation, toStation, dijkstraShortestPath); | ||
|
|
||
| return new PathResponse(fare, pathDistance, stationInfos); | ||
| } |
There was a problem hiding this comment.
path와 관련된 로직이 복잡해진 것 같은데, 분리해보면 어떨까요?
테스트의 관점에서도 분리했을 때 장점이 있을 것 같아요!
There was a problem hiding this comment.
클래스로 따로 분리를 해봤습니다.
Service에 대한 로직이 좀 더 추상화되고,
특히 Service에 필요한 로직들만 더 잘 드러나는 것처럼 보이네요.
그리고 분리를 적용했기에 테스트도 훨씬 작은 단위로 수행할 수 있어보입니다!
| public boolean isLastStationAtRight(Station station) { | ||
| public boolean isLastStationAtRight(final Station station) { |
There was a problem hiding this comment.
또한 로직이 복잡하다는 기준을 어떻게 정해야할지 궁금했습니다.
복잡성에 대한 기준은 사람마다 다를 수 있지만, 한 눈에 코드의 흐름이 명확하게 보이면 좋지 않을까 해요! 100줄이 넘어간다면 가독성이 떨어진다 생각합니다.
한 가지 생각은 List<Section> 을 관리하는 별도의 객체를 둘 수도 있을 것 같아요.
There was a problem hiding this comment.
확실히 Sections를 통해서 분리하는 것이 각 객체의 역할이 한눈에 들어오는 것 같다고 느꼈습니다.
일급 컬렉션의 필요성을 다시한번 생각하게 되는 계기가 되었던 것 같네요..!
| @@ -19,16 +19,16 @@ public Line(final String name) { | |||
| this.sections = new LinkedList<>(); | |||
| } | |||
|
|
|||
| public Line(Long id, String name, LinkedList<Section> sections) { | |||
| public Line(final Long id, final String name, final LinkedList<Section> sections) { | |||
There was a problem hiding this comment.
LinkedList는 단순히 도메인 관점에서 바라봤을 때를 기준으로 작성했습니다.
ArrayList로 관리를 한다면 대부분이 인덱스로 관리가 될 것이라 생각했습니다.
또한 가장 앞과 가장 뒤의 원소를 지우는 경우가 존재한다고 생각했고,
인덱스가 아닌 구현된 메서드(ex. removeFirst(), addFirst(), ...)를 사용하여 가독성을 높일 수 있다고 생각했습니다.
또한 위와 같은 경우 자바의 LinkedList는 Doubly Linked List로 구현되어 있기에
크지는 않지만 성능 면에서의 이점도 가져갈 수 있다고 생각하여 사용하였습니다!
There was a problem hiding this comment.
또한 가장 앞과 가장 뒤의 원소를 지우는 경우가 존재한다고 생각했고,
장점도 될 수 있지만, 중간에 역을 추가하는 요구 사항도 있기에 고민되는 부분이긴 하네요 !
There was a problem hiding this comment.
중간에 역은 마찬가지로 인덱스로 관리된다는 점은 같다고 생각하는데,
그렇다면 처음과 마지막에서 장점을 가져갈 수 있는 LinkedList가 더 효율적이지 않나라는 생각이었습니다.
혹시 제가 생각하지 못하거나 잘못 생각한 부분이 있다면 수정하겠습니다!
안녕하세요 완태! 저문입니다 :)
1단계를 빠듯하게 진행해서 그런지 2단계에서 하고 싶었던 것들을 전부 적용해보지는 못했네요🥲
1단계 코멘트 중에서 어떻게 변경해야할지 감이 안왔던 부분이 있었는데요.
#48 (comment)
이 부분은 어떻게 책임을 분리하면 좋을지 잘 떠오르지 않았습니다..!
또한 로직이 복잡하다는 기준을 어떻게 정해야할지 궁금했습니다.
처음 이 부분을 고민했을 때는 크게 복잡하지 않다고 느꼈는데, 기준을 어떻게 잡으면 좋을지 궁금합니다.
#48 (comment)
또한 Repository의 책임에 대한 궁금증도 생겨서 생각을 코멘트에 남겨봤습니다!
2단계 리뷰도 잘 부탁드립니다🙇🏻♂️🙇🏻♂️