[Spring 경로 조회 - 1단계] 주디(윤주리) 미션 제출합니다. #198
Conversation
* refactor: remove frontend code * refactor: flatten project structure * docs: remove frontend guide * refactor: remove unusable resources * chore: Update versions
* feat: add cors configuration * refactor: add generic type to ResponseEntity
* docs: 기능 요구사항 작성 * feat: 지하철 역 관리 API 기능 * feat: 지하철 노선 등록 기능 * feat: 지하철 노선 목록 기능 * feat: 지하철 노선 조회 * feat: 지하철 노선 수정 * feat: 지하철 노선 삭제 * feat: Station 관리 Spring 애노테이션 적용 * feat: Line 관리 Spring 애노테이션 적용 * refactor: 불필요한 라이브러리 제거 및 공백 수정 * docs:기능 요구사항 업데이트 * refactor: StationController의 중복되는 API Mapping을 RequestMapping으로 변경 * refactor: LineController의 중복되는 API Mapping을 RequestMapping으로 변경 * refactor: 지하철역과 지하철 노선이 중복되는 경우를 잡는 로직 변경 * test: test 패키지의 schema.sql을 사용하여 Dao 객체 테스트 * test: Service 객체 테스트 조건 추가 * test: E2E테스트와 Dao 테스트에 @SQL을 통해 test Schema 연결 * test: E2E테스트에서 response body도 테스트하도록 변경 * test: Dao 테스트의 부모객체 생성 * refactor: 예외발생시 커스텀예외 추가 * feat: 지하철역 삭제요청시 지하철역 정보가 DB에 없으면 예외발생 * feat: LineService 예외 발생 추가 1. id를 통해 지하철 노선 조회시 id가 DB에 존재하지 않으면 예외발생 2. id를 통해 지하철 노선 삭제시 id가 DB에 존재하지 않으면 예외발생 3. id를 통해 지하철 노선 시 업데이트시 id가 DB에 존재하지 않으면 예외발생 4. 지하철 노선 업데이트시 중복되는 이름으로 업데이트시 예외발생 Co-authored-by: jaejae-yoo <[email protected]>
오류가 나는 테스트 및 프로덕션 코드 주석처리
1. 상행역 정보 저장 2. 하행역 정보 저장 3. 상행역과 하행역 사이의 거리 저장 오류가 나는 테스트 및 프로덕션 코드 주석처리
오류가 나는 테스트 및 프로덕션 코드 전체 수정
| import wooteco.subway.exception.datanotfound.LineNotFoundException; | ||
|
|
||
| @Repository | ||
| public class LineDaoImpl implements LineDao { |
There was a problem hiding this comment.
LineDaoImpl이라는 이름은, LineDao의 구현체 중 하나라는 것 외에 어떤 정보도 담지 못하고 있는 것 같아요. 어떤 식으로 이름을 붙여 주면 하는 일을 더 잘 드러낼 수 있을까요?
| public Line(String name, String color, int extraFare) { | ||
| this.name = name; | ||
| this.color = color; | ||
| this.extraFare = extraFare; | ||
| } | ||
|
|
||
| public Line(long id, String name, String color, int extraFare) { | ||
| this.id = id; | ||
| this.name = name; | ||
| this.color = color; | ||
| this.extraFare = extraFare; | ||
| } |
There was a problem hiding this comment.
생성자가 다른 생성자를 이용할 수도 있는데요, 이렇게 하면 어떤 장점이 있을까요?
There was a problem hiding this comment.
한 줄로 어떤 값이 들어가는지 표현할 수 있겠네요!
수정하겠습니다 :)
| @Override | ||
| public int deleteStation(long id) { | ||
| final String sql = "UPDATE station SET deleted = (?) WHERE id = (?)"; | ||
| return jdbcTemplate.update(sql, true, id); | ||
| } |
There was a problem hiding this comment.
Soft delete 방식을 사용하셨군요 👍
그런데, 이 방식을 도입하신 특별한 이유가 있을까요? 지금 STATION 테이블의 name 컬럼에는 유니크 키가 걸려 있는데, 그러면 어떤 문제가 발생할 수 있을까요?
There was a problem hiding this comment.
Soft delete 방식을 도입한 이유는 데이터를 백업하기도 쉽고, 삭제된 데이터가 궁금할 때 찾아서 볼 수 있다는 장점 때문에 도입하였습니다.
또한 실무를 경험해보지 못한 입장에서 (모든 실무에서 그런건 아니겠지만)최대한 실무에서 사용하는 방식을 사용해보고 싶었어요!
name 컬럼에 유니크 키가 걸려있을 때 생기는 문제는 db에서 중복 이름을 저장했을 때 db단에서 예외가 발생하는 거라고 생각해요.
저는 단점도 있지만 장점도 있다고 생각해서 사용했습니다.
장점
- 해당 컬럼이 unique해야한다는 것을 명시
- 혹시나 모를 중복이름이 저장됨을 방지
단점
- 예외를 지정해주지 못한다.
이렇게만 생각하다가 코니의 질문을 받고 든 의문은, 도메인 비지니스 로직을 통해 이미 중복을 검증하는데, db단에서 또 확인해줘야할 필요가 있을까? 라는 생각이 듭니다.
그래서 만약 중복을 확인하는 thread1 중복을 확인할 때는 db에 값이 들어있지 않았지만, 중복 확인이 끝난 후 thread2에서 저장을 했다면 db에서도 unique로 확인을 해줘야하는건가? 라는 생각이 들었습니다!
코니는 어떻게 생각하시나요?
| @RestControllerAdvice | ||
| public class ControllerAdvice { | ||
|
|
||
| @ExceptionHandler({DuplicateNameException.class}) | ||
| public ResponseEntity<String> handleDuplicateNameException(ClientException exception) { | ||
| return ResponseEntity.badRequest().body(exception.getMessage()); | ||
| } |
There was a problem hiding this comment.
@RestControllerAdvice와 @ExceptionHandler를 이용하여 예외를 잘 처리해 주셨네요 👍
다만 몇 가지 고민해 보아요.
- 모든 종류의 예외를 처리하는 방식이 동일한데 각각 메서드를 만들어야만 할까요? 다른 방법은 없을까요?
- 여기서 다루지 않고 있는 예외가 발생할 가능성은 없나요? 바로 위의 코멘트에서 남긴 상황(역 생성 후 삭제, 삭제된 역과 동일한 이름의 역 생성 요청)을 테스트해 볼까요?
There was a problem hiding this comment.
- 상태코드마다 exception을 묶는 것이 좋겠네요!
- soft delete를 적용하므로써 생기는 예외들과 어디서 발생할지 모르는 전체 예외도 잡아줘야할 것 같네요 :)
| private boolean hasDuplicateLine(final LineRequest lineRequest) { | ||
| return lineDao.findAll() | ||
| .stream() | ||
| .anyMatch(line -> line.getName().equals(lineRequest.getName())); | ||
| } |
There was a problem hiding this comment.
계속 페어가 이전 단계에서 작성한 코드에 대한 피드백이 나오고 있는데, 페어는 연대 책임이니까 😌 마저 남겨 봅니다.
중복을 확인하기 위해 모든 데이터를 조회해야만 할까요? 이렇게 하면 어떤 문제가 발생할 수 있을까요?
There was a problem hiding this comment.
FindAll를 과도하게 사용할 경우 서버에 무리가 올 수 있겠다는 생각이 드네요. LineRequest에서 name를 이용하여 exist 으로 확인해줄 수 있을 것 같습니다 :)
| assertAll(() -> assertThat(stations).isEqualTo(List.of(new Station(4L, "이대역"), new Station(5L, "학동역") | ||
| , new Station(6L, "이수역"), new Station(7L, "건대역")))); |
There was a problem hiding this comment.
expected되는 값을 빼고 어떤 값이 들어있는지 잘보이도록 수정했습니다.
뭔가 더 좋은 방법이 있을 것 같은데 떠오르지 않네요.. 😢
| private void createSection(final Long lineId, final SectionRequest sectionRequest) { | ||
| ExtractableResponse<Response> sectionResponse = RestAssured.given().log().all() | ||
| .body(sectionRequest) | ||
| .contentType(MediaType.APPLICATION_JSON_VALUE) | ||
| .when() | ||
| .post("/lines/" + lineId + "/sections") | ||
| .then().log().all() | ||
| .extract(); | ||
| } |
| @DisplayName("경로 거리가 10Km 이하면 1250이 부과된다.") | ||
| @Test | ||
| void calculateFare_basic() { | ||
| double distance = 11; | ||
| FareCalculator fareCalculator = new FareCalculator(distance); | ||
| int fare = fareCalculator.calculateFare(); | ||
|
|
||
| assertThat(fare).isEqualTo(1350); | ||
| } |
| @DisplayName("경로 거리가 10Km 초과이고 50Km 이하면 5km당 100원이 부과된다.") | ||
| @Test | ||
| void calculateFare_middle() { | ||
| double distance = 50; | ||
| FareCalculator fareCalculator = new FareCalculator(distance); | ||
| int fare = fareCalculator.calculateFare(); | ||
|
|
||
| assertThat(fare).isEqualTo(2050); | ||
| } |
There was a problem hiding this comment.
경곗값 테스트 👍 더 다양한 테스트 케이스로 확인해 보아도 좋을 것 같아요.
| public SubwayGraph() { | ||
| subwayGraph = new WeightedMultigraph(DefaultWeightedEdge.class); | ||
| } |
There was a problem hiding this comment.
제네릭이 구현해주는 부분이 없네요!..........
수정하였습니당!
|
코니 안녕하세요! 몇몇 리뷰에 제 생각과 관련된 질문을 했는데 봐주시면 감사하겠습니당! |
| @ExceptionHandler(Exception.class) | ||
| public ResponseEntity<String> handleException(Exception exception) { | ||
| return ResponseEntity.internalServerError().body(exception.getMessage()); | ||
| } |
There was a problem hiding this comment.
예상하지 못한 예외가 발생했을 때, 해당 예외에서 메시지를 바로 꺼내 클라이언트에 노출해도 괜찮을까요?
또한, 예외가 발생한 경우 서버 관리자가 이를 알 수 있어야 하지 않을까요? 지금은 예외가 발생하면 이 기록이 남을까요?
There was a problem hiding this comment.
예외가 발생한 경우 서버 관리자도 알도록 로그를 추가해야겠네요 :)
프론트에서도 어떤 예외가 발생했는지 알아야한다고 생각했는데, 생각해보니 서버에서 발생한 예외는 프론트에서 해결할 수 없으니, 상태코드로만 알려주어야 한다는 생각이 드네요!
| public LineRequest(String name, String color, Long upStationId, Long downStationId, int distance, int extraFare) { | ||
| validatePositive(distance, extraFare); |
There was a problem hiding this comment.
요청 객체의 생성자에서 값 검증을 하고 있군요. 그런데, 실제로 애플리케이션을 실행해 이 코드가 원하는 대로 잘 동작하는지 확인해 보셨나요?
또한, 도메인 객체에서는 값을 검증하지 않아도 괜찮을까요? 레벨 1에서 진행했던 많은 미션들과 거기서 배웠던 것들을 떠올려 볼까요?
| public List<Station> calculateShortestPath(final Station source, final Station target) { | ||
| SubwayGraph subwayGraph = initSubwayGraph(); | ||
| return subwayGraph.findShortestPath(source, target); | ||
| } |
There was a problem hiding this comment.
출발지와 도착지가 구간으로 연결되어 있지 않아 경로를 찾을 수 없는 경우 어떻게 되나요?
| subwayGraph.setEdgeWeight(subwayGraph.addEdge(section.getUpStation(), section.getDownStation()), | ||
| section.getDistance()); |
There was a problem hiding this comment.
이런 경우에는 변수 할당으로 한 번 끊어주는 편이 가독성이 더 좋은 것 같아요.
| package wooteco.subway.domain; | ||
|
|
||
| import java.util.List; | ||
| import org.jgrapht.GraphPath; | ||
| import org.jgrapht.alg.shortestpath.DijkstraShortestPath; | ||
| import org.jgrapht.graph.DefaultWeightedEdge; | ||
| import org.jgrapht.graph.WeightedMultigraph; |
There was a problem hiding this comment.
지금은 "도메인"이 외부 라이브러리에 직접 의존하고 있는 구조로 되어 있는데요, 이런 구조에서 발생할 수 있는 문제에는 어떤 것이 있을까요? 꼭 고칠 필요는 절대 없지만, 고민해 볼 가치는 있을 것 같아요.
| assertThat(fare).isEqualTo(actualFare); | ||
| } | ||
|
|
||
| @DisplayName("경로 거리가 50Km 이상이면 8km당 100원이 부과된다.") |
| assertThat(stations).isEqualTo(List.of(new Station(1L, "잠실역"), new Station(2L, "선릉역") | ||
| , new Station(3L, "강남역"), new Station(4L, "건대역"))); |
There was a problem hiding this comment.
여기도 이전에 남겼던 코멘트와 같은 방식으로 가독성이 좀 떨어지는 느낌인데요. 우선 아래와 같은 방법도 사용할 수 있을 것 같고, 조금 더 고민해 보거나 검색해 보면 더 나은 방법을 찾을 수 있을지도 몰라요!
assertThat(stations).containsExactly(
new Station(1L, "잠실역"),
new Station(2L, "선릉역"),
new Station(3L, "강남역"),
new Station(4L, "건대역")
);| @DisplayName("최단 경로를 구한다.") | ||
| @Test |
There was a problem hiding this comment.
이 로직은 우리 애플리케이션의 핵심 역할을 한다고 볼 수 있는데, 이 정도 테스트로 충분할까요? happy path도 조금 더 다양하게 확인해 보고, 예외 케이스도 고려하면 훨씬 좋지 않을까요?
| Station source = stationDao.findById(sourceStationId); | ||
| Station target = stationDao.findById(targetStationId); |
There was a problem hiding this comment.
저는 계층 마다 존재하는 이유가 있다고 생각해요.
가령, 서비스 계층은 dao들의 메서드들을 모아 transaction 단위를 만들고, 그 과정에서 비지니스 로직들을 처리한다고 생각해요.
그래서 계층에서 계층을 참조하는 것이 계층 간의 관계를 모호하게 한다고 생각합니다.
어느 정도는 유연하게 가져가야겠지만, 어떤 경계가 모호할 수록 그 부분을 구현하는데 시간이 걸린다고 생각해서 왠만하면 layer별로 방향성을 가지고 의존성을 주입해주는 것이 좋다고 생각했어요.
계층에서 계층을 참조하는 것이 계층 간의 관계를 모호하게 하는 것 같다고 하셨는데, 저는 과연 그러한가? 하는 생각이 들긴 하네요.
이렇게 구현하게 되었을 때, 단점은 하나의 service에 코드가 몰리게 되는 현상이 발생하고, 여러개의 dao가 하나의 service에 의존성이 주입되므로, 메서드 명이 길어지고(가령 find가 여러개의 도메인으로 나뉠 것 같아요. ex. findLine findStation) 가독성이 떨어지게 된다는 것이 있을 것 같아요!
하지만 하나의 service가 어떤 도메인만 사용해야하는 것은 아니므로 큰 문제는 없다고 느꼈습니다.
이젠 DAO를 꽤 사용해 보셨으니 어느 정도 아실 것 같은데, 데이터 접근 계층은 이름 그대로 데이터 접근에 관한 내용만을 다루는 것이 좋지요. 그래서 DAO에는 비즈니스 로직에 관한 용어 또는 내용을 드러내지 않는 것이 권장됩니다. 하지만 우리는 여러 상황에서 여기에 비즈니스 로직과 연결하여 의미 부여를 해야 하고, 그것이 주로 DAO를 이용하는 서비스 계층에서 이뤄지곤 하지요. 그래서 DAO만을 직접 사용하는 경우 이런 코드의 중복이 발생할 가능성이 크고, 그로 인해 각 객체의 책임이 불분명해지는 문제가 발생할 수 있을 것 같아요.
이런 문제들은 앞으로 남은 미션들을 통해 학습하거나 경험하시게 될 거라, 혹시나 지금 완전히 이해가 되지 않더라도 그렇구나~ 하고 넘어가셔도 됩니다.
There was a problem hiding this comment.
아직 각 객체의 책임이 불분명해지는 느낌이 잘 이해가 되지 않지만, 부지런히 경험해보겠습니다! 생각해볼 수 있는 좋은 리뷰 너무 감사드려요! :)
| @Override | ||
| public int deleteById(final Long id) { | ||
| final String sql = "UPDATE station SET deleted = (?) WHERE id = (?)"; | ||
| return jdbcTemplate.update(sql, true, id); | ||
| } |
There was a problem hiding this comment.
Soft delete 방식을 도입한 이유는 데이터를 백업하기도 쉽고, 삭제된 데이터가 궁금할 때 찾아서 볼 수 있다는 장점 때문에 도입하였습니다.
또한 실무를 경험해보지 못한 입장에서 (모든 실무에서 그런건 아니겠지만)최대한 실무에서 사용하는 방식을 사용해보고 싶었어요!name 컬럼에 유니크 키가 걸려있을 때 생기는 문제는 db에서 중복 이름을 저장했을 때 db단에서 예외가 발생하는 거라고 생각해요.
저는 단점도 있지만 장점도 있다고 생각해서 사용했습니다.장점
- 해당 컬럼이 unique해야한다는 것을 명시
- 혹시나 모를 중복이름이 저장됨을 방지
단점
- 예외를 지정해주지 못한다.
이렇게만 생각하다가 코니의 질문을 받고 든 의문은, 도메인 비지니스 로직을 통해 이미 중복을 검증하는데, db단에서 또 확인해줘야할 필요가 있을까? 라는 생각이 듭니다.
그래서 만약 중복을 확인하는 thread1 중복을 확인할 때는 db에 값이 들어있지 않았지만, 중복 확인이 끝난 후 thread2에서 저장을 했다면 db에서도 unique로 확인을 해줘야하는건가? 라는 생각이 들었습니다!
코니는 어떻게 생각하시나요?
제가 여기서 Soft delete와 유니크 키에 대한 코멘트를 남긴 의도는, 이 두 가지 각각에 대한 이야기를 하고 싶었던 것은 아니고 이 둘이 결합되었을 때의 문제를 함께 이야기하고 싶어서였어요. 이 둘이 결합되면서, 예를 들어 홍대입구 역을 한 번 생성한 후 삭제했다면 홍대입구 역을 다시 생성할 수가 없었죠.
지금은 이것을 해결하기 위해 저장하기 전에 저장하려는 역의 이름으로 검색을 해서 해당 항목을 삭제하고 있어요. 그런데, 이렇게 해도 괜찮을까요? 만약 실수로 중복된 역 이름을 입력했다가는 기존 역으로 구성된 구간 정보 등에 다 문제가 생길 텐데요. 게다가 Soft delete 방식을 도입한 의의도 무력화하는 것 같고요.
코니 안녕하세요 저는 주디라고 해요!
이번 미션은 페어의 코드로 진행했는데, 이전에 도메인 및 테스트를 너무 깔끔하게 설계해주셔서 빠르게 마무리 할 수 있었어요:)
바쁘신 와중에 제 코드 리뷰를 해주셔서 감사해요.
나중에 척척학사 개발자가 되어서 24배로 갚겠습니다 :)