[1단계 - 지하철 정보 관리 기능] 깃짱(조은기) 미션 제출합니다. #35
Conversation
|
|
||
| for (DefaultWeightedEdge edge : graph.incomingEdgesOf(station)) { | ||
| edgesToRemove.add(edge); | ||
| distanceBetweenUpLineStationAndStation = (int) graph.getEdgeWeight(edge); |
There was a problem hiding this comment.
이렇게 되면 distanceBetweenUpLineStationAndStation가 계속 덮어씌워지는데 문제 없나요?
graph.incomingEdgesOf(station) 가 하나뿐이라고 가정을 내리신거라면, 그 이외에 상황에서는 에러가 난다고 코드로 분명하게 써주는것이 좋아보입니다.
| edgeDao.deleteEdgesIn(line); | ||
|
|
||
| for (final Station station : allStationsInOrder) { | ||
| final long stationOrder = allStationsInOrder.indexOf(station); |
| validateDistance(distance); | ||
|
|
||
| // 기존 역: upLineStation, 새로운 역: downLineStation | ||
| if (graph.containsVertex(upLineStation)) { |
| import java.util.Set; | ||
|
|
||
| public class SubwayGraph { | ||
| private final DefaultDirectedWeightedGraph<Station, DefaultWeightedEdge> graph = new DefaultDirectedWeightedGraph<>(DefaultWeightedEdge.class); |
| import java.util.Set; | ||
|
|
||
| public class SubwayGraph { | ||
| private final DefaultDirectedWeightedGraph<Station, DefaultWeightedEdge> graph = new DefaultDirectedWeightedGraph<>(DefaultWeightedEdge.class); |
There was a problem hiding this comment.
DefaultWeightedEdge 말고 부모클래스인 IntrusiveWeightedEdge로 value 타입을 변경하고, stationEdge를 IntrusiveWeightedEdge를 상속해서 구현하면 DefaultWeightedEdge, stationEdge 둘다 받을 수 있ㄴ는 형태가 되겠네요!
|
|
||
| @ExceptionHandler(IllegalArgumentException.class) | ||
| public ResponseEntity<Object> handleIllegalArgumentException(final IllegalArgumentException exception) { | ||
| return ResponseEntity.badRequest().body(exception.getMessage()); |
There was a problem hiding this comment.
IllegalArgumentException 를 클라이언트 오류인 400으로 내려도 될까요? 이것도 500으로 내려야할 것 같습니다.
개인적으로 저는 400은 client request에 대한것, controller 내부로 들어와서부터는 500으로 내려야 하지 않을까 생각합니다!
400이 내려가면 정말 클라이언트가 요청하는 잘못된 bad request또는 잘못된 path params 으로 생기는 클라쪽 실수 에러인지, 서버에서 발생하는 에러인지 파악하는데도 어려움울 줄 수있는 상황이 있지 않을까 합니다.
이렇게되면 경계가 명확히 나뉘지 않는것 같아서, 에러가 클라이언트의 잘못된 요청인지 서버내에서 발생하는 에러인지 상황파악하는데 어려움이 있지 않을까 해서요.
의견있으면 적어주세요~!
There was a problem hiding this comment.
IllegalArgumentException은 제 코드 상에서 사용자가 존재하지 않는 역의 Id를 입력하면서 section을 추가하라고 한다던가 하는 경우에 IAE를 발생시키고, 400대 에러로 처리하고 있습니다.
이 경우에 클라이언트의 실수라고 보고, 400대 에러를 보내도 괜찮지 않을까요..?
도메인 내부에서 발생하는 IAE에 대해서는 service로부터 타고 온 것이니 400대인지 500대인지 아직 저도 명확하지가 않아서 좀 더 생각해봐야 할 것 같아요..!
There was a problem hiding this comment.
IAE는 표준 예외라 다른 라이브러리나, 외부 코드에서도 충분히 발생할 수 있어서,
클라이언트의 잘못을 확실히 분리하기 위해서
몇 가지 커스텀 예외를 추가하는 방향으로 리팩터링했습니다!
| | GET | /lines/{lineId} | 200 | 특정 노선을 조회한다. | | ||
| | POST | /lines | 201 | 노선을 생성한다. | | ||
| | POST | /lines/{lineId}/stations | 201 | 특정 노선에 역을 추가한다. | |
There was a problem hiding this comment.
line에 1,2 distance:3 추가 후에 line에 1,3 distance:1 추가 하면
새로운 역의 거리는 기존 두 역의 거리보다 작아야 합니다. 라는 에러가 나오네요
There was a problem hiding this comment.
정합서이 깨지는 값들이 디비에 들어가고 있습니다. 거리가 0인 것들은 존재해서는 안되는데, 들어가고 있네요. 갈래길도 디비에 등록되고 있습니다.
이거 확인부탁드려요
There was a problem hiding this comment.
@Test
void addTwoStationsInLineTest() {
final long station1Id = 역_생성하고_아이디_반환(EXPRESS_BUS_TERMINAL_REQUEST);
final long station2Id = 역_생성하고_아이디_반환(SAPYEONG_STATION_REQUEST);
final long newStationId = 역_생성하고_아이디_반환(NEW_STATION_REQUEST);
final long lineId = 노선_생성하고_아이디_반환(LINE_NINE_CREATE_REQUEST);
노선에_최초의_역_2개_추가_요청(lineId,
new InitialSectionCreateRequest(
lineId, station1Id, station2Id, 3
));
final ExtractableResponse<Response> response = 노선에_역_1개_추가_요청(station1Id, newStationId, lineId, 1);
assertThat(response.statusCode()).isEqualTo(CREATED.value());
}요청을 인수테스트로 해봤는데, 저는 에러가 나지 않는 것 같아요...! 어떤 문제인지 너무 복잡해서 파악이 잘 안되네요...
또 거리를 0으로 하고 추가하니 400 상태코드를 잘 반환하고 있어요..!
인수 테스트 말고 혹시 어떤 방식으로 테스트하셨는지 알려주실 수 있을까요??
지난번에 delete 관련된 dao를 구현하지 않았던게 문제가 아닐까 추측합니다@......
There was a problem hiding this comment.
0으로 직접 요청하는것이 아니라 정상적으로 역을 등록하게 될때, 디비에 자동으로 들어가고 있었습니다!
There was a problem hiding this comment.
라이브러리 관련 문제 해결하면서 같이 해결된 것 같습니다..!
혹시 또 잘못되고 있으면 알려주세요!
| final LineResponse lineResponse = lineService.createInitialSection(id, request); | ||
| return ResponseEntity | ||
| .created(URI.create("/" + lineResponse.getId() + "/stations")) | ||
| .body(lineResponse); |
There was a problem hiding this comment.
create에 대해서, 상태 코드 201을 반환하면서 리소스의 Location을 보내주고, 생성된 리소스도 함께 보내주고 있습니다.
관련해서 크루들과 이야기를 해봤는데,
-
Location과 리소스 모두 전달파
Location만 주면, 클라이언트가 리소스가 필요할 때 주소로 굳이 한 번 더 조회하는 쿼리를 날려야 하므로, lineResponse 내에 id가 들어있기 때문에 전송하는 데이터가 중복되더라도 Location, 생성된 데이터를 모두 반환한다. -
Location만 전달파
리소스가 필요한 경우 확인 가능한 경로를 알려주니, 필요할 테면 쓰라는 뜻이므로 URI만 보내는 것으로 충분하다.
서브웨이는 어떻게 생각하시나요?
저는 일단 둘 다 보내게 만들었어요. 만드는 방법을 알면 빼는건 쉬워서요!
There was a problem hiding this comment.
클라이언트의 상황에 따라 다를것 같은데요. 저는 2번형태로 드릴 것 같아요! save요청 성공하면 id를 줘서 page에서 reload하게끔 사용하지 않을까 합니다! 근데 이건 어떻게 하든 상관없을것 같아서 따로 말씀 안드렸어요~!
| @@ -1,24 +1,33 @@ | |||
| package subway.domain; | |||
|
|
|||
| import subway.entity.LineEntity; | |||
|
|
|||
| import java.util.Objects; | |||
|
|
|||
| public class Line { | |||
There was a problem hiding this comment.
맞습니다.... 저도 Line이 List<Station> 형태로 각 노선에 속한 역들을 가지고 있는 것이 도메인의 관점에서 더 좋다고 생각해요.
| } | ||
|
|
||
| @Override | ||
| public boolean equals(final Object o) { |
There was a problem hiding this comment.
- 요청이 들어오는 방향에서는 id가 null인데, 이것만으로 데이터를 구분할 수가 없지 않을까요?
- LineEntity 역시 id만으로 구분하게 되면, 자바 코드 내에서는 데이터베이스에서와 달리 Id가 겹치지 않도록 관리하는 강제성이 없는데 괜찮을까요?
- id만으로 식별하라는 말은 equals, hashcode 재정의를 id에 대해서만 하면 된다는 말로 이해해도 될까요?
저는 Line domain 객체는 Controller -> service -> repository까지 id가 null인 상태로 사용하고,
repository, dao 사이에서는 LineEntity로 변환해서 사용중이에요.
후에 저장하게 되면 id를 갖게 되고, 그 뒤로는 id를 가진 Line 도메인 객체로 사용하게 됩니다.
| return graph.vertexSet().contains(station); | ||
| } | ||
|
|
||
| public Station findStationBefore(final Station station) { |
There was a problem hiding this comment.
네 이 클래스 너무 복잡해서, 저도 중복되는 부분을 제거하고 싶었는데!
피드백 감사합니다!
안녕하세요 서브웨이!
저는 깃짱이라고 합니다.
지난번에 샘플 코드와 제 코드가 섞여서 리뷰하기 매우 어려웠을 것 같아요 죄송합니다ㅠㅠ
샘플 코드를 다 날려버리고, 다시 돌아가는 애플리케이션을 만들었어요.
궁금한 점은 댓글로 해당 코드에 남겨놨어요!