[희망 도서] 포도당 희망도서 추가/조회/소프트 삭제 제출합니다.#6
[희망 도서] 포도당 희망도서 추가/조회/소프트 삭제 제출합니다.#6backlo wants to merge 26 commits intowoowacourse:developmentfrom backlo:development
Conversation
MrKwon
left a comment
MrKwon
left a comment
There was a problem hiding this comment.
크.. 도서관리 프로젝트의 첫 PR과 첫 머지를 기록하게 될 포도당 🍇👍
잘 완성해서 미래의 크루들에게 우리의 이름을 알려보자구요 !
그런 의미에서 몇가지 코멘트를 달아보았어요 !
고생하셨습니다.
추가로 귀찮으시겠지만 각 클래스 앞에
/*
* @(#)[클래스명].java
*
* v 1.0.0
*
* 2019.01.01
*
* Copyright (c) 2019 Milzipmoza Developers. [작성자]
* Woowacourse, Seoul, KOREA
* All right Reserved
*/
이것도 달아주세요 !
| private Long id; | ||
|
|
||
| @Lob | ||
| @Column(name = "IMAGE") |
There was a problem hiding this comment.
Column 명도 컨벤션을 따르는 쪽으로 해서 Pascal case 로 하는게 좋을 것 같아요 !
| then(). | ||
| assertThat(). | ||
| statusCode(is(200)) | ||
| .body("title", equalTo(NEW_TITLE)); |
There was a problem hiding this comment.
사소하지만, body 앞에 .(점) 이 내려와 있어요. 확인 부탁드립니다.
| get("/wishes"). | ||
| then(). | ||
| assertThat(). | ||
| statusCode(is(200)). |
There was a problem hiding this comment.
assertj의 Assertions.assertThat() 메서드가 then() 에 내장되어 있어서 assertThat() 이 없어도 테스트가 가능해요.
그리고 statusCode() 안에 is() 가 없어도 검증하는 로직이 수행됩니다. 이 역시 윗줄의 이유랑 같아요 !
| @DisplayName("타이틀을 통해 희망도서를 찾는다.") | ||
| void findByTitle() { | ||
| wishRepository.save(wish); | ||
| Wish foundWish = wishRepository.findByTitle(TITLE).orElseThrow(() -> new WishNotFoundException("희망도서를 찾을 수 없습니다.")); |
There was a problem hiding this comment.
의도적으로 한 줄 안에 작성한 것이 아니라면 줄 바꿈을 해도 좋지 않을까요??
| import static io.restassured.RestAssured.given; | ||
|
|
||
|
|
||
| public class Utils { |
There was a problem hiding this comment.
WishUtils 로 이름을 변경하는 것은 어떻게 생각하시나요? 이름이 모호한 것 같아요 !
| @Getter | ||
| public abstract class BasicEntity { | ||
|
|
||
| @Column(name = "is_active") |
There was a problem hiding this comment.
이것도 컨벤션을 따라 파스칼 케이스로 이름이 작성되면 좋을 것 같아요.
| } | ||
|
|
||
| public WishResponseDto save(WishRequestDto wishRequestDto) { | ||
| Wish saveWish = Wish.builder() |
There was a problem hiding this comment.
필드 하나당 한줄로 작성하는 것이 좋지 않을까요??
| @Column(name = "TITLE", length = 100, unique = true) | ||
| private String title; | ||
|
|
||
| @Column(name = "AUTHOR", length = 20) |
There was a problem hiding this comment.
length 는 50으로 하는 것이 좋을 것 같아요. 제가 예전에 20으로 이야기 했다면 죄송..😂
간혹 여러 명의 일본인 저자와 여러명의 역자가 있는 경우 20을 넘어갈 가능성이 높다고 생각해서요 !
| @Column(name = "IMAGE") | ||
| private String image; | ||
|
|
||
| @Column(name = "TITLE", length = 100, unique = true) |
There was a problem hiding this comment.
unique 속성을 isbn 으로 옮겨주세요. ㅎㅎ
책 이름은 같을 수 있지만 isbn 은 전 세계 모든 책이 각각 유일하게 가지고 있는 번호라 isbn 에 unique 속성이 있는 것이 좋을 것 같아요.
| import org.springframework.web.bind.annotation.*; | ||
|
|
||
| @RestController | ||
| @RequestMapping("/wishes") |
There was a problem hiding this comment.
/api/wishes 로 매핑하는 것이 좋을 것 같아요.
우리가 이 프로젝트를 잘 구현해 두면 저희의 api를 다른 프로젝트에 제공해줄 수도 있으니까요. ㅎㅎ
| @@ -0,0 +1,7 @@ | |||
| package com.woowacourse.tecobrary.exception; | |||
|
|
|||
| public class WishNotFoundException extends RuntimeException{ | |||
There was a problem hiding this comment.
"요청한 책을 찾을 수 없습니다." 메세지가 반복 사용되니까 아래처럼 상수처리를 하면 어떨까요? 이렇게 해놓으면 나중에 테스트할 때에도 유용한거 같아요.
public static final String WISH_NOT_FOUND_MESSAGE = "요청한 책을 찾을 수 없습니다."
public WishNotFoundException() {
super(WISH_NOT_FOUND_MESSAGE);
}
| } | ||
|
|
||
| @AfterEach | ||
| void tearDown() { |
There was a problem hiding this comment.
@DataJpaTest를 사용하면 테스트마다 롤백되기 때문에 이부분은 필요없을것 같아요
| public static String createWishBook(String baseUrl, WishRequestDto wishRequestDto) { | ||
| RestAssured.baseURI = baseUrl; | ||
|
|
||
| return given(). |
3 participants
No description provided.