[2단계 - 경로 조회 기능] 조이(김성연) 미션 제출합니다. #188
Conversation
| private PathResponse buildPathResponse(final Sections sections, final PathRequest request) { | ||
| Station source = pathRepository.findStationById(request.getSourceStationId()); | ||
| Station destination = pathRepository.findStationById(request.getDestinationStationId()); | ||
| SubwayGraph graph = new SubwayGraph(sections.getSections()); | ||
| List<Station> path = graph.getPath(source, destination); | ||
| int distance = graph.getWeight(source, destination); | ||
| Fare fare = fareCalculator.calculate(distance); | ||
| return PathResponse.of(path, distance, fare); | ||
| } |
There was a problem hiding this comment.
메서드를 추상화시켜보면 어떨까요?
현재 buildPathResponse에서 경로를 찾는 모든 로직을 절차적으로 처리해주고 있는 것 같아요. 🧐
There was a problem hiding this comment.
의견 주셔서 감사합니다 또링!
그런데 제가 어떤 의미로 말씀하신건지 이해를 잘 못 하겠습니다..ㅠ
우선 제 나름대로 이해를 해보고, Path 도메인을 구현해서, 생성 시 내부에서 로직을 처리하도록 해보았는데요!
혹시 말씀하신 의미가 이런 것이 아니라면 조금 더 설명을 부탁드려도 될까요?!🥲
There was a problem hiding this comment.
위 코드를 보면 경로를 찾기 위해 객체간의 협력이 보이지 않고 직접 객체의 데이터를 꺼내서 절차적으로 처리해주고 있는데요, 이 부분을 개선해보면 어떨까 했어요~
현재 path 도메인을 구현해서 경로에 관한 책임을 부여하셨는데, 제가 의도한 방향과 비슷하게 리팩터링 해주신 것 같아요 😃👍
이렇게 되면 path를 구하는 로직은 path에서만 알면되고 path를 가져다 사용하는 service에서는 몰라도 됩니다.
(추가로 덧붙이자면, 메서드를 추상화한다는 buildPathResponse()에서 알 필요가 없는 로직을 추상화시키자 (=객체 내부로 감추자)라는 의미였습니다)
| @Bean | ||
| public FareStrategy fareStrategy() { | ||
| return new DistanceStrategy(); | ||
| } | ||
|
|
||
| @Bean | ||
| public FareCalculator fareCalculator() { | ||
| return new FareCalculator(fareStrategy()); | ||
| } |
There was a problem hiding this comment.
DistanceStrategy를 빈으로 등록시켜주셨네요!
어떤 이유로 빈으로 등록해주셨나요?
또 @Bean 과 @Component는 어떤 차이가 있을까요?
There was a problem hiding this comment.
요금 계산에 대한 정책이 바뀌게 되면 코드를 수정해줘야 할 것 같아서 스프링 DI를 활용하고자 했습니다!
@Bean의 경우 개발자가 컨트롤이 어려운 것들을 (예를 들면 외부 라이브러리) 빈으로 등록하고 싶은 경우 사용하고, @Component의 경우 개발자가 직접 컨트롤할 수 있는 클래스에 사용합니다.
두 애너테이션의 차이를 고려해보면 @Component를 사용하는 것이 더 적절할 것 같습니다. 수정하겠습니다!
제가 두 애너테이션에 대한 차이를 잘 몰랐던 것 같습니다. 이번 기회에 차이를 생각해볼 수 있어서 좋았습니다. 감사합니다!
| public interface FareStrategy { | ||
| Fare calculate(int value); | ||
| } |
There was a problem hiding this comment.
3단계 요구사항을 적용하기 위해 미리 요금 정책을 추상화 시키셨군요! 👍
| @Override | ||
| public Fare calculate(final int distance) { | ||
| validateDistance(distance); | ||
|
|
||
| if (distance <= BASE_THRESHOLD) { | ||
| return BASIC_FARE; | ||
| } | ||
|
|
||
| if (distance <= ADDITIONAL_THRESHOLD) { | ||
| int surcharge = calculateSurcharge(distance, BASE_THRESHOLD, FIRST_STANDARD); | ||
| return BASIC_FARE.sum(new Fare(surcharge)); | ||
| } | ||
|
|
||
| int surcharge = calculateSurcharge(ADDITIONAL_THRESHOLD, BASE_THRESHOLD, FIRST_STANDARD) | ||
| + calculateSurcharge(distance, ADDITIONAL_THRESHOLD, SECOND_STANDARD); | ||
| return BASIC_FARE.sum(new Fare(surcharge)); | ||
| } | ||
|
|
There was a problem hiding this comment.
이런 if-else 구조는 OCP를 위반하게 만드는 패턴 중 하나인데요, 어떻게 개선할 수 있을까요?
이전 미션에서 적용한 전략패턴을 도입해보면 어떨까요?
There was a problem hiding this comment.
또링이 의견 주신대로 확장 시에 좋지 않을 수 있겠네요..!
SurchargesCalculator들을 만들어 distance에 따라서 적용될 수 있도록 코드를 변경해보았습니다.
그런데 하나 고민이 되는 부분이 있습니다.
변경된 코드에서는 거리별 구간 요금을 계산하는 distanceStrategy 에서 사용되는 SurchargesCalculator가 추가되었습니다.
그리고 그 구현체인 BasicSectionCalculator, FirstSectionCalculator, SecondSectionCalculator도 추가로 구현하였습니다.
그런데 이를 적용하면서 든 생각은 전략 패턴 + 전략 패턴을 적용하니 가독성도 좋고 확장 시에도 용이할 것 같지만, 만약 전략이 다양해지면 클래스와 인터페이스가 너무 많아지는 것이 아닌지 생각이 듭니다.
현재는 전략 패턴을 적용하는 사이즈가 크지 않고 공부를 하는 과정이지만, 더 큰 서비스를 만든다면 어떨지 궁금해서 질문드립니다!
클래스와 인터페이스의 수가 많아진다는 것은 그에 따른 비용이 증가하는게 아닌가.. 생각이 들어서요.
현업에서는 이런 장단점을 고려했을 때, 전략 패턴을 많이 사용하시는지 궁금합니다!
There was a problem hiding this comment.
좋은 고민 해주셨네요! 💯
말씀해주신것처럼 전략패턴을 사용하다보면 인터페이스와 클래스가 많아지는 단점이 있습니다. 하지만 이게 정말 단점일까 생각해보면, 전 그렇지 않다고 생각해요. 해당 클래스는 하나의 책임을 가진 객체로 충분히 목적을 잘 수행하고 있다고 생각합니다.
클래스와 인터페이스의 수가 많아진다는 것은 그에 따른 비용이 증가하는게 아닌가.. 생각이 들어서요.
여기서 말씀주신 비용은 클래스, 인터페이스를 새로 생성하는 비용과 로직 전반을 살펴보기 위해서는 관련된 수많은 파일들을 열어 읽어야하는 그런 비용을 의미하신거겠죠?
다른 사람의 코드를 유지보수했던 경험상 하나의 메서드에 if-else로 정책이 나열된 로직보다 전략패턴이 적용된 로직이 읽기에도, 수정하기에도 편했던 것 같아요. 그래서 저도 이런식으로 많이 구현하고 있구요.
(여기서부터는 제 경험담이라 스킵하셔도 됩니다)
저도 입사하고 가장 고민했던 부분이 어떻게 변경에 유연한 코드를 짤 수 있을까 였는데요,
사용자가 3번 이상 주문하면 3000원 쿠폰을 준다. 와 같은 요구사항이 있을때, ~하면 ~한다와 같이 조건으로 빼낼 수 있는 부분은 모두 전략패턴을 적용해보곤 했던것 같아요.
현업에서는 생각보다 기존 정책이 변경되거나 새로운 정책이 추가되는 일이 잦아서 이런 구조가 유지보수에 굉장히 도움이 되더라구요. (물론 파일이 많아지는 만큼 패키지 구조는 잘 잡혀있어야겠죠~!)
아직은 공감이 안되실 수 있는데, 지금 조이가 작성해주신것처럼 무조건 패턴을 적용하기보다는 장단점을 비교해보시면 조이만의 기준을 잡아갈 수 있을거라 생각합니다~!
| import java.util.List; | ||
| import org.jgrapht.alg.shortestpath.DijkstraShortestPath; | ||
| import org.jgrapht.graph.DefaultWeightedEdge; | ||
| import org.jgrapht.graph.WeightedMultigraph; | ||
|
|
||
| public class SubwayGraph { | ||
| private final WeightedMultigraph<Station, DefaultWeightedEdge> graph = new WeightedMultigraph<>( | ||
| DefaultWeightedEdge.class); | ||
| private final DijkstraShortestPath<Station, DefaultWeightedEdge> dijkstraShortestPath; |
There was a problem hiding this comment.
도메인이 외부 라이브러리와 강하게 결합되어 있어요. 이런 경우 어떤 문제가 발생할까요?
개선할 수 있는 방법은 없을까요?
There was a problem hiding this comment.
도메인과 외부 라이브러리가 강하게 결합되어 있는 경우 유지보수 관점에서 문제가 있을 것 같습니다.
만약 요구사항이 변경된다면 해당 코드와 관련 코드들을 전체적으로 변경해야하는 문제가 발생할 것 같습니다.
예를 들면 다른 라이브러리를 이용하게 되거나, 라이브러리를 사용하지 않고 직접 알고리즘 구현하는 등의 경우를 생각해볼 수 있을 것 같습니다. 그렇게 되면 도메인의 상당 부분을 수정해야하기 때문에 많은 비용이 들 것 같습니다!
그래서 인터페이스를 통해 분리해보는 방법을 사용해보았습니다.
SubwayGraph 인터페이스를 생성하여 graph 변경에 용이하도록 구현해보았습니다!
혹시 더 좋은 방법이 있다면 알려주시면 적용해보겠습니다:)
| @Override | ||
| public Fare calculate(final int distance) { | ||
| validateDistance(distance); | ||
|
|
||
| if (distance <= BASE_THRESHOLD) { | ||
| return BASIC_FARE; | ||
| } | ||
|
|
||
| if (distance <= ADDITIONAL_THRESHOLD) { | ||
| int surcharge = calculateSurcharge(distance, BASE_THRESHOLD, FIRST_STANDARD); | ||
| return BASIC_FARE.sum(new Fare(surcharge)); | ||
| } | ||
|
|
||
| int surcharge = calculateSurcharge(ADDITIONAL_THRESHOLD, BASE_THRESHOLD, FIRST_STANDARD) | ||
| + calculateSurcharge(distance, ADDITIONAL_THRESHOLD, SECOND_STANDARD); | ||
| return BASIC_FARE.sum(new Fare(surcharge)); | ||
| } | ||
|
|
There was a problem hiding this comment.
또링이 의견 주신대로 확장 시에 좋지 않을 수 있겠네요..!
SurchargesCalculator들을 만들어 distance에 따라서 적용될 수 있도록 코드를 변경해보았습니다.
그런데 하나 고민이 되는 부분이 있습니다.
변경된 코드에서는 거리별 구간 요금을 계산하는 distanceStrategy 에서 사용되는 SurchargesCalculator가 추가되었습니다.
그리고 그 구현체인 BasicSectionCalculator, FirstSectionCalculator, SecondSectionCalculator도 추가로 구현하였습니다.
그런데 이를 적용하면서 든 생각은 전략 패턴 + 전략 패턴을 적용하니 가독성도 좋고 확장 시에도 용이할 것 같지만, 만약 전략이 다양해지면 클래스와 인터페이스가 너무 많아지는 것이 아닌지 생각이 듭니다.
현재는 전략 패턴을 적용하는 사이즈가 크지 않고 공부를 하는 과정이지만, 더 큰 서비스를 만든다면 어떨지 궁금해서 질문드립니다!
클래스와 인터페이스의 수가 많아진다는 것은 그에 따른 비용이 증가하는게 아닌가.. 생각이 들어서요.
현업에서는 이런 장단점을 고려했을 때, 전략 패턴을 많이 사용하시는지 궁금합니다!
| @Bean | ||
| public FareStrategy fareStrategy() { | ||
| return new DistanceStrategy(); | ||
| } | ||
|
|
||
| @Bean | ||
| public FareCalculator fareCalculator() { | ||
| return new FareCalculator(fareStrategy()); | ||
| } |
There was a problem hiding this comment.
요금 계산에 대한 정책이 바뀌게 되면 코드를 수정해줘야 할 것 같아서 스프링 DI를 활용하고자 했습니다!
@Bean의 경우 개발자가 컨트롤이 어려운 것들을 (예를 들면 외부 라이브러리) 빈으로 등록하고 싶은 경우 사용하고, @Component의 경우 개발자가 직접 컨트롤할 수 있는 클래스에 사용합니다.
두 애너테이션의 차이를 고려해보면 @Component를 사용하는 것이 더 적절할 것 같습니다. 수정하겠습니다!
제가 두 애너테이션에 대한 차이를 잘 몰랐던 것 같습니다. 이번 기회에 차이를 생각해볼 수 있어서 좋았습니다. 감사합니다!
| import java.util.List; | ||
| import org.jgrapht.alg.shortestpath.DijkstraShortestPath; | ||
| import org.jgrapht.graph.DefaultWeightedEdge; | ||
| import org.jgrapht.graph.WeightedMultigraph; | ||
|
|
||
| public class SubwayGraph { | ||
| private final WeightedMultigraph<Station, DefaultWeightedEdge> graph = new WeightedMultigraph<>( | ||
| DefaultWeightedEdge.class); | ||
| private final DijkstraShortestPath<Station, DefaultWeightedEdge> dijkstraShortestPath; |
There was a problem hiding this comment.
도메인과 외부 라이브러리가 강하게 결합되어 있는 경우 유지보수 관점에서 문제가 있을 것 같습니다.
만약 요구사항이 변경된다면 해당 코드와 관련 코드들을 전체적으로 변경해야하는 문제가 발생할 것 같습니다.
예를 들면 다른 라이브러리를 이용하게 되거나, 라이브러리를 사용하지 않고 직접 알고리즘 구현하는 등의 경우를 생각해볼 수 있을 것 같습니다. 그렇게 되면 도메인의 상당 부분을 수정해야하기 때문에 많은 비용이 들 것 같습니다!
그래서 인터페이스를 통해 분리해보는 방법을 사용해보았습니다.
SubwayGraph 인터페이스를 생성하여 graph 변경에 용이하도록 구현해보았습니다!
혹시 더 좋은 방법이 있다면 알려주시면 적용해보겠습니다:)
| private PathResponse buildPathResponse(final Sections sections, final PathRequest request) { | ||
| Station source = pathRepository.findStationById(request.getSourceStationId()); | ||
| Station destination = pathRepository.findStationById(request.getDestinationStationId()); | ||
| SubwayGraph graph = new SubwayGraph(sections.getSections()); | ||
| List<Station> path = graph.getPath(source, destination); | ||
| int distance = graph.getWeight(source, destination); | ||
| Fare fare = fareCalculator.calculate(distance); | ||
| return PathResponse.of(path, distance, fare); | ||
| } |
There was a problem hiding this comment.
의견 주셔서 감사합니다 또링!
그런데 제가 어떤 의미로 말씀하신건지 이해를 잘 못 하겠습니다..ㅠ
우선 제 나름대로 이해를 해보고, Path 도메인을 구현해서, 생성 시 내부에서 로직을 처리하도록 해보았는데요!
혹시 말씀하신 의미가 이런 것이 아니라면 조금 더 설명을 부탁드려도 될까요?!🥲
| private PathResponse buildPathResponse(final Sections sections, final PathRequest request) { | ||
| Station source = pathRepository.findStationById(request.getSourceStationId()); | ||
| Station destination = pathRepository.findStationById(request.getDestinationStationId()); | ||
| SubwayGraph graph = new SubwayGraph(sections.getSections()); | ||
| List<Station> path = graph.getPath(source, destination); | ||
| int distance = graph.getWeight(source, destination); | ||
| Fare fare = fareCalculator.calculate(distance); | ||
| return PathResponse.of(path, distance, fare); | ||
| } |
There was a problem hiding this comment.
위 코드를 보면 경로를 찾기 위해 객체간의 협력이 보이지 않고 직접 객체의 데이터를 꺼내서 절차적으로 처리해주고 있는데요, 이 부분을 개선해보면 어떨까 했어요~
현재 path 도메인을 구현해서 경로에 관한 책임을 부여하셨는데, 제가 의도한 방향과 비슷하게 리팩터링 해주신 것 같아요 😃👍
이렇게 되면 path를 구하는 로직은 path에서만 알면되고 path를 가져다 사용하는 service에서는 몰라도 됩니다.
(추가로 덧붙이자면, 메서드를 추상화한다는 buildPathResponse()에서 알 필요가 없는 로직을 추상화시키자 (=객체 내부로 감추자)라는 의미였습니다)
| @Override | ||
| public Fare calculate(final int distance) { | ||
| validateDistance(distance); | ||
|
|
||
| if (distance <= BASE_THRESHOLD) { | ||
| return BASIC_FARE; | ||
| } | ||
|
|
||
| if (distance <= ADDITIONAL_THRESHOLD) { | ||
| int surcharge = calculateSurcharge(distance, BASE_THRESHOLD, FIRST_STANDARD); | ||
| return BASIC_FARE.sum(new Fare(surcharge)); | ||
| } | ||
|
|
||
| int surcharge = calculateSurcharge(ADDITIONAL_THRESHOLD, BASE_THRESHOLD, FIRST_STANDARD) | ||
| + calculateSurcharge(distance, ADDITIONAL_THRESHOLD, SECOND_STANDARD); | ||
| return BASIC_FARE.sum(new Fare(surcharge)); | ||
| } | ||
|
|
There was a problem hiding this comment.
좋은 고민 해주셨네요! 💯
말씀해주신것처럼 전략패턴을 사용하다보면 인터페이스와 클래스가 많아지는 단점이 있습니다. 하지만 이게 정말 단점일까 생각해보면, 전 그렇지 않다고 생각해요. 해당 클래스는 하나의 책임을 가진 객체로 충분히 목적을 잘 수행하고 있다고 생각합니다.
클래스와 인터페이스의 수가 많아진다는 것은 그에 따른 비용이 증가하는게 아닌가.. 생각이 들어서요.
여기서 말씀주신 비용은 클래스, 인터페이스를 새로 생성하는 비용과 로직 전반을 살펴보기 위해서는 관련된 수많은 파일들을 열어 읽어야하는 그런 비용을 의미하신거겠죠?
다른 사람의 코드를 유지보수했던 경험상 하나의 메서드에 if-else로 정책이 나열된 로직보다 전략패턴이 적용된 로직이 읽기에도, 수정하기에도 편했던 것 같아요. 그래서 저도 이런식으로 많이 구현하고 있구요.
(여기서부터는 제 경험담이라 스킵하셔도 됩니다)
저도 입사하고 가장 고민했던 부분이 어떻게 변경에 유연한 코드를 짤 수 있을까 였는데요,
사용자가 3번 이상 주문하면 3000원 쿠폰을 준다. 와 같은 요구사항이 있을때, ~하면 ~한다와 같이 조건으로 빼낼 수 있는 부분은 모두 전략패턴을 적용해보곤 했던것 같아요.
현업에서는 생각보다 기존 정책이 변경되거나 새로운 정책이 추가되는 일이 잦아서 이런 구조가 유지보수에 굉장히 도움이 되더라구요. (물론 파일이 많아지는 만큼 패키지 구조는 잘 잡혀있어야겠죠~!)
아직은 공감이 안되실 수 있는데, 지금 조이가 작성해주신것처럼 무조건 패턴을 적용하기보다는 장단점을 비교해보시면 조이만의 기준을 잡아갈 수 있을거라 생각합니다~!
안녕하세요 또링, 조이입니다!
이번 리뷰도 잘 부탁드리겠습니다🙂
좋은 하루 되세요!
현재 코드에서 Lombok을 사용하고 있습니다. 기능 구현이 우선이라고 생각하여 아직 제거하지 못 하였으나, 이번 리뷰 요청 이후 리팩터링 과정에서 제거해보고자 합니다.
지난번에
station의 id 가 식별자의 역할을 제대로 하고 있는가?에 대한 질문을 드렸었습니다. 이에 대한 수정 역시 리팩터링 과정에서 해결해보려고 합니다.