-
Notifications
You must be signed in to change notification settings - Fork 280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Step1] 경로 찾기 - 지하철역 관리 #34
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요. 리뷰어 이동규입니다. 😀
기능 단위로 커밋을 나누신 점, 메서드를 작은 단위로 나누신 점이 인상적이네요 👍
그리고 패키지 구조도 도메인 기반으로 나누셨는데요.
현재 Station 은 단순 CRUD만 있는데, controller/service/repository/domain 로 추상을 한단계 더 나누는 것이 의미가 있을지, repository는 domain layer의 생애주기를 관리하는데 domain package와 추상 수준이 같은건 아닐지, 현재 애플리케이션에서 Service와 Controller를 구분하는 것이 오버엔지니어링은 아닐지 등에 대해서도 한번 고민해보시는 것도 좋을거 같아요.
몇가지 간단한 피드백 남겨두었는데요. 확인해보시고 궁금한 점 있으시면 메시지 남겨주세요~
한 주의 시작이네요! 좋은 하루 보내시길 바랍니다. 😀
compileOnly("org.projectlombok:lombok") | ||
testCompileOnly("org.projectlombok:lombok") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
public List<Station> list() | ||
{ | ||
List<Station> returnList = stationRepository.findAll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List, Set 등 자료구조를 지칭하는 단어를 클래스나 변수이름으로 사용하는 것을 지양하고 있습니다.
사용하는 자료구조가 변경되면 변경의 범위가 외부로 전파될 수 있기 때문입니다.
List와 같은 복수의 데이터의 경우 복수형을 쓰는 것이 더 적합하다고 생각합니다.
/* | ||
1. 지하철역 등록 | ||
*/ | ||
@PostMapping("/create") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
REST API 설계시에,
- URI는 정보의 자원을 표현해야 한다.(리소스명은 동사보다는 명사를 사용)
- 자원에 대한 행위는 HTTP Method(GET, POST, PUT, DELETE 등)으로 표현
stations URI에 POST 요청을 보내는 것만으로도 생성의 의미를 표현할 수 있습니다.
POST와 PUT 사용에 대해서는 해석의 여지가 있습니다.
보통 PUT은 수정, POST는 생성으로 많이 사용합니다.
다만, HTTP 완벽가이드 등에서는 아래와 같이 제시하기도 합니다.
- PUT은 URI가 존재하면 update 없으면 insert를 한다.
- POST는 URI가 collection일 경우 collection에 추가하고 element일 경우 해당 URI를 collection으로 간주하여 그 하위에 들어간다.
이 경우엔, PUT을 사용하여 리소스를 생성할 경우, 요청하는 쪽에서 ID를 알고 있어야 합니다.
반면, POST로 생성을 하려면 ID를 모르는 상태에서 URI에 collection을 지정해야 한다.
ex) "PUT /articles/1" 하면 1번 글이 생성되고, "POST /articles" 하면 서버에서 할당한 번호의 글이 생성된다.
HttpHeaders headers = new HttpHeaders(); | ||
headers.setLocation(URI.create("/stations/list" + createdStation.getId())); | ||
ResponseEntity<Station> returnEntity = new ResponseEntity<>(createdStation, headers, HttpStatus.CREATED); | ||
return returnEntity; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return returnEntity; | |
return ResponseEntity | |
.created(URI.create(BASE_STATIONS_URL + createdStation.getId())) | |
.contentType(MediaType.APPLICATION_JSON) | |
.body(createdStation); |
public static BodyBuilder created(URI location) {
BodyBuilder builder = status(HttpStatus.CREATED);
return builder.location(location);
}
@Column(length = 20, nullable = false) | ||
private String name; | ||
|
||
@Builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
{ | ||
testCreateStation(STATIOIN_NAME); | ||
|
||
webTestClient.delete().uri(BASE_URL + "/delete/" + TEST_ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ID가 반드시 1임을 어떻게 보장할 수 있을까요
현재 DirtiesContext를 사용하기에 전체 테스트가 통과하나, 이는 ApplicationContext를 매번 재생성하기에 비용이 큰 테스트 방식입니다. F.I.R.S.T를 보장하기 위해서는 어떻게 작성하는 것이 좋을지 고민해보는 것도 좋을거 같아요.
https://pivotal.io/application-modernization-recipes/testing/spring-boot-testing-best-practices
@@ -0,0 +1,38 @@ | |||
package atdd.station.Service; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
package atdd.station.Service; | |
package atdd.station.service; |
@@ -0,0 +1,19 @@ | |||
package atdd.station.domain.dto; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dto는 presentation layer에 해당하니 controller 하위 패키지에 있는 것은 어떨까요
/* | ||
1. 지하철역 등록 | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 주석보다는 코드로 의도를 드러내는 것을 선호하는 편입니다 ㅎ;
@Autowired | ||
private WebTestClient webTestClient; | ||
|
||
@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
junit5의 경우 @DisplayName
애노테이션을 활용하여 의도를 드러낼 수도 있습니다.
https://www.journaldev.com/21674/junit-display-name-displayname
No description provided.