[step1] 1단계 - 노선 관리 기능 구현 #216
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
- Line 엔티티를 통한 LineResponse 생성자 추가 및 관련 부분 리팩터링
- Line 조회 시, Station 조회 가 다량 발생 (N+1) 하여 fetch join 사용
junwoochoi
approved these changes
Jan 24, 2022
There was a problem hiding this comment.
안녕하세요! 1주차 리뷰를 맡은 최준우라고 합니다 😄
1단계 미션 완벽히 수행해주셨습니다!!
다음 미션도 화이팅입니다! 🚀
- 실패 시나리오를 정리하면서 인수테스트를 작성하는 것도 좋은 연습이 될 것 같습니다!
- 현재 요구사항에서는 필요하지 않지만, 추후 요구사항이 변경되면 추가해주시면 좋을 것 같습니다.
- 마찬가지로 현재 요구사항에서는 필요치 않지만 추후 요구사항이 생기면 추가해주시면 좋을 것 같습니다!
전반적으로 도메인에 관해 깊이 고민해보시다보니 다양한 예외 케이스나, 추가적으로 적용되어야할 요구사항 같은 점들이 마음에 걸리시는 것 같아요 😅
저희 미션의 최우선 목표는 인수테스트의 흐름을 이해하고, 경험하면서 다양한 경험치를 쌓아가는 걸 우선으로 하고 있어서, 주어진 요구사항안에서 구현 및 인수 테스트의 흐름을 따라가시는 것이 인수테스트에 대한 경험에 더 집중하실 수 있는 환경이 될 것 같습니다 😃
| private List<StationResponse> stations; | ||
|
|
||
| public LineIncludingStationsResponse(Line line) { | ||
| super(line); |
There was a problem hiding this comment.
LineIncludingStationsResponse가 LineResponse를 상속받는 형태로 구현하셨군요!
확실히 코드의 중복이 줄지만, 상속은 강한 결합이라서, 추후 LineResponse내부 특정 필드를 제거했지만, LineIncludingStationsResponse에서는 해당 필드가 그대로 남아있어야한다면 다시 떼어내기 힘들다는 점, 유의하면서 사용하시면 더 도움이 될 것 같습니다! 👍
Comment on lines
+15
to
+16
| @OneToMany(mappedBy = "line", fetch = FetchType.LAZY) | ||
| private List<Station> stations = new ArrayList<>(); |
Comment on lines
+12
to
+13
| @ManyToOne(fetch = FetchType.LAZY) | ||
| private Line line; |
There was a problem hiding this comment.
아마 추후 미션을 진행하면서 다룰 것 같은데, 해당 주차가 되기전에 미리 양방향 참조, 직접 참조, 간접 참조에 대해 찾아보시는 것도 추후 수업 및 미션 진행하면서 도움이 되실 것 같습니다! 😃
| } | ||
|
|
||
| @PutMapping(value = "/{id}") | ||
| public ResponseEntity<Void> updateLine(@PathVariable Long id, @RequestBody LineRequest lineRequest) { |
There was a problem hiding this comment.
Void로 응답의 body가 비어있을 것을 명시적으로 표현되어 있어서 좋네요 💯
Comment on lines
+30
to
+34
| assertThat(response.statusCode()).isEqualTo(HttpStatus.CREATED.value()); | ||
| assertThat(response.header("Location")).isNotBlank(); | ||
| Integer responseIntegerId = response.jsonPath().get("id"); | ||
| Long createdId = responseIntegerId.longValue(); | ||
| assertThat(createdId).isGreaterThan(0L); |
Comment on lines
+11
to
+71
| public class LineTestStep { | ||
|
|
||
| public static ExtractableResponse<Response> 지하철_노선을_생성한다(String lineColor, String lineName) { | ||
| Map<String, String> body = new HashMap<>(); | ||
| body.put("color", lineColor); | ||
| body.put("name", lineName); | ||
|
|
||
| return RestAssured | ||
| .given().log().all() | ||
| .body(body) | ||
| .contentType(MediaType.APPLICATION_JSON_VALUE) | ||
| .when() | ||
| .post("/lines") | ||
| .then().log().all() | ||
| .extract(); | ||
| } | ||
|
|
||
| public static ExtractableResponse<Response> 지하철_노선_목록을_조회한다() { | ||
| return RestAssured | ||
| .given().log().all() | ||
| .accept(MediaType.APPLICATION_JSON_VALUE) | ||
| .when() | ||
| .get("/lines") | ||
| .then().log().all() | ||
| .extract(); | ||
| } | ||
|
|
||
| public static ExtractableResponse<Response> 지하철_노선을_조회한다(Long id) { | ||
| return RestAssured | ||
| .given().log().all() | ||
| .accept(MediaType.APPLICATION_JSON_VALUE) | ||
| .when() | ||
| .get("/lines/" + id) | ||
| .then().log().all() | ||
| .extract(); | ||
| } | ||
|
|
||
| public static ExtractableResponse<Response> 지하철_노선을_수정한다(Long lineId, String lineColor, String lineName) { | ||
| Map<String, String> body = new HashMap<>(); | ||
| body.put("color", lineColor); | ||
| body.put("name", lineName); | ||
|
|
||
| return RestAssured | ||
| .given().log().all() | ||
| .body(body) | ||
| .contentType(MediaType.APPLICATION_JSON_VALUE) | ||
| .when() | ||
| .put("/lines/" + lineId) | ||
| .then().log().all() | ||
| .extract(); | ||
| } | ||
|
|
||
| public static ExtractableResponse<Response> 지하철_노선을_삭제한다(Long lineId) { | ||
| return RestAssured | ||
| .given().log().all() | ||
| .when() | ||
| .delete("/lines/" + lineId) | ||
| .then().log().all() | ||
| .extract(); | ||
| } | ||
| } |
There was a problem hiding this comment.
공통적으로 사용하는 테스트 메소드들을 Step클래스에 분리하여서 코드 재사용성과 가독성이 올라갔네요 👍
DaehunGwak
added a commit
to DaehunGwak/atdd-subway-map
that referenced
this pull request
Jan 24, 2022
- LineResponse 와 의존성 제거 next-step#216 (comment)
junwoochoi
pushed a commit
that referenced
this pull request
Jan 25, 2022
* refactor: LineIncludingStationsResponse 상속구조 해제 - LineResponse 와 의존성 제거 #216 (comment) * refactor: 지하철_노선_생성한_후_아이디_추출하기 스텝 추가 후 중복 부분 리팩터링 * refactor: 지하철역 요청 스텝 분리 후 관련 테스트 리팩터링 * feat: 중복이름으로 지하철역 생성 실패 기능 추가 실패 시, 409 Conflict Status Code 를 사용하도록 ControllerAdvice 사용 * feat: 중복이름으로 지하철 노선 생성 실패 기능 추가
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.
안녕하세요! 1단계 완성되어 공유드립니다~
궁금한 점이 있어 아래와 같이 정리하여 공유드립니다!