[1단계 - 상품 관리 기능] 제이(이재윤) 미션 제출합니다. #195
Conversation
There was a problem hiding this comment.
안녕하세요 제이, 리뷰어 시카입니다. 😄
요구사항에 맞춰 잘 구현해주셨네요! 👍
몇 가지 코멘트 남겨드렸으니 2단계에서 같이 반영 부탁드려요!
이 번 단계에서 리뷰를 이어가기 보다는, 다음 단계도 반영한 후에 같이 진행하면 좋을 것 같아서요 ㅎㅎ
추가로 궁금한 점이 생기면 언제든지 DM 주세요 ㅎㅎ
수정 혹은 삭제를 할 때, 조회를 한 후 상품이 있는지 없는지 체킹하고 또 한 번 수정 혹은 삭제 쿼리를 날려도 되나요?
먼저 답을 드리면 일단 체크를 해도 되고, 안해도 된다고 생각합니다 ㅎㅎㅎ
- 상품이 있는지 없는지, 요청한 상품이 맞는지 등등의 비즈니스 요구사항이 있다면 다른 방법으로 처리할 수 있을까요??
- 수정과 삭제를 분리해서 생각해봐도 좋을 것 같아요! 클라이언트가 수정 요청을 했을 때는 반영 여부를 궁금해 할 텐데요...! 삭제의 경우도 궁금해할까요??
일단은 AfterEach로 테스트가 종료될 때마다 데이터를 다 지워버리는데 이렇게 했을 때, 혹시나 다른 문제점이 없을지 궁금합니다.
여러가지 이슈가 있을 수 있겠지만, 대표적으로 다른 테스트에서 같은 entity를 의존하는 경우 문제가 있을 수 있지 않을까요?? ㅎㅎ
만약 마이페이지 같은 것이라면, /pages/{username} 과 같이 유저의 이름만 입력하면 나의 정보를 보여주기 싫지만 누구든 다 볼 수 있지 않을까?!
따라서 DTO에 id값 같은 식별값을 함께 넣어주는 건 어떻게 생각하시는지 궁금합니다.
필요한 경우 dto에 id값을 넣어주어야 할 떄도 있겠지만, 생각하신 것처럼 RESTful 원칙적으로는 올바른 방향은 아닌 것 같아요 ㅎㅎ
이 부분은 2단계를 진행하면 해결이 될 것 같아서 그 때 다시 이야기 나눠봐요 😄
처음에는 Mock을 이용해서 스터빙을 통해서 getName() 과 같은 것을 임의로 주었지만, 결국에 테스트가 불안정해질 수 있기 때문에 모든 인자를 받는 생성자를 추가하였습니다.
이에 대해서 어떻게 생각하시나요?
결국 테스트를 위해서 비즈니스 코드에 수정사항이 생기는 것이기 때문에 좋은 방법은 아니지만, 때떄로 선택할 수 도 있는 영역이라고 생각은 하고있습다 ㅎㅎ 모두가 그렇게 만들어진 생성자는 사용하지 않는 것에 대해 인지하고 있다면요!
다만, 그런 경우 휴먼에러가 발생할 확률이 높기때문에 현재 상태에서 아래 질문을 진행하시면서 비즈니스 레벨에서 AllArguConstructor가 필요한 이유를 찾아보면 좋을 것 같아요!
| @org.springframework.web.bind.annotation.ControllerAdvice | ||
| public class ControllerAdvice { |
There was a problem hiding this comment.
ControllerAdvice.class와 어노테이션 이름이 똑같아서 생기는 문제인데요 ㅎㅎㅎ
클래스 이름을 역할에 맞게 명시적으로 바꿔보면 어떨까요???
| @PostMapping | ||
| public ResponseEntity<Void> createProduct(@RequestBody @Valid final ProductCreateRequestDto productCreateRequestDto) { | ||
| productService.createProduct(productCreateRequestDto); | ||
|
|
||
| return ResponseEntity.status(HttpStatus.CREATED) | ||
| .build(); | ||
| } | ||
|
|
||
| @PutMapping | ||
| public ResponseEntity<Void> editProduct(@RequestBody @Valid final ProductEditRequestDto productEditRequestDto) { | ||
| productService.editProduct(productEditRequestDto); | ||
|
|
||
| return ResponseEntity.status(HttpStatus.OK) | ||
| .build(); | ||
| } | ||
|
|
||
| @DeleteMapping("/{id}") | ||
| public ResponseEntity<Void> deleteProduct(@PathVariable final Long id) { |
There was a problem hiding this comment.
- Post, Put, Delete의 경우 HttpStatus 이외에 아무것도 보내지 않는 이유가 있을까요??
- 객체 지향적으로 생각했을 때
return void method와 비교해서 고민해보면 어떨까요?
There was a problem hiding this comment.
- RESTful API에서 POST 응답에서는 201 상태코드만을 반환한다고 나와있습니다.
사실 이건 원칙이라 제 생각에는 팀원과의 협의에 따라서 응답값을 내려줘도 괜찮다고 생각합니다!
또한, Put 같은 경우에는 사용자에게 수정된 정보를 반환하는 경우가 많기 때문에 변경된 부분 + 변경되지 않은 부분까지 주는 것이 좋을 것 같습니다. (하지만 이번 미션에서는 그런 부분이 보이지 않아서 반환하지는 않았습니다!)
마지막으로 Delete 또한 상태코드만을 내려주면서 프론트서버에서 이를 이용해서 사용자에게 응답을 내려줄 수 있다고 생각해서 반환하지 않았습니다!
- 객체끼리의 상호작용이 중요한 객체지향 관점에서 봤을 때 메서드가 로직을 수행한 후 값을 반환하면 상태를 외부에서 검증할 수 있기 때문에 반환하지 않는 것보다는 안전하다고 생각합니다. (즉, 반환 값이 명확하다면 다른 객체와 상호작용할 때 더 안전하고 제어하기 쉽다는 뜻입니다!)
ResponseEntity<Void>의 경우 반환 결과를 주는 것이기 때문에 아무 값을 반환하지 않는 void보다 좋다고 생각합니다.
There was a problem hiding this comment.
음... 이 부분은 제가 알고 있는 것과 달라서요 미리 답변 드립니다 ㅎㅎㅎ
테코톡 영상을 참고해 보면 어떨까요?ㅎㅎㅎ
| public Product(final Long id, final Name name, final String imgUrl, final Price price) { | ||
| this.id = id; | ||
| this.name = name; | ||
| this.imgUrl = imgUrl; | ||
| this.price = price; | ||
| } | ||
|
|
||
| public static Product from(final Long id, final String name, final String imgUrl, final int price) { | ||
| return new Product(id, new Name(name), imgUrl, new Price(price)); | ||
| } | ||
|
|
||
| public static Product from(final String name, final String imgUrl, final int price) { |
There was a problem hiding this comment.
정적 팩터리 메서드 사용 👍
생성자의 접근제어자도 public인 이유가 있나요??
There was a problem hiding this comment.
꼼꼼하게 확인 했어야 했는데 이부분을 놓쳤습니다..!
말씀하신대로 외부에서 정적 팩토리 메서드를 사용해서 객체를 생성하기 때문에, 생성자의 접근 제어자는 private로 변경하였습니다.
| public ProductCreateRequestDto() { | ||
| } |
There was a problem hiding this comment.
- 지금 이 생성자를 제거하면 동작하지 않을까요 ???
- NoArguConstructor를 사용하지 않고 json 역직렬화를 하는 방법을 공부해보면 어떨까요?? ㅎㅎ
There was a problem hiding this comment.
-
현재에선 동작하지 않습니다! 제가 공부하기론, JSON데이터에서 JAVA객체로 역직렬화를 하는 과정에서 리플렉션을 이용할 때 기본 생성자를 사용하기 때문입니다.
다시 코드를 살펴보니, 기본 생성자의 접근제어자를 private로 바꾼다면, 외부에서 null인 객체를 생성하지는 않을 것 같습니다! -
다른 방법으로는
@JsonCreator어노테이션과 함께@JsonProperty어노테이션을 사용하면서 명시해주는 방법이 있습니다. (찾아보니 이렇게 사용하면 final 키워드를 붙일 수 있다는 장점이 있지만, 제가 느낀 단점으로는 어노테이션 범벅으로 코드가 지저분해지는 것 같습니다.)
또 다른 방법으로는 setter 사용이 있습니다. 이 또한 리플렉션 과정에서 값을 주입할 수 있다는 점을 확인했습니다. :)
제가 빈생성자로 코드를 작성한 가장 큰 이유는 가독성 때문이었습니다!
감사합니다 🙇🏻♂️
| private ProductResponseDto(final Long id, | ||
| final String name, | ||
| final String imgUrl, | ||
| final int price) { | ||
| this.id = id; |
There was a problem hiding this comment.
파라미터 정렬이 일반적이진 않은 것 같아요 ㅎㅎㅎ
컨벤션에 따라 어떻게 하셔도 상관은 없지만, 전체적으로 통일보면 어떨까요??
There was a problem hiding this comment.
전체적으로 한 줄로 나열했는데, 이 부분에서만 개행을 추가 했군요..!
리팩토링 진행하겠습니다!
꼼꼼한 리뷰 감사합니다 :)
| CREATE TABLE product | ||
| ( | ||
| id BIGINT AUTO_INCREMENT, | ||
| name VARCHAR(45) NOT NULL, |
There was a problem hiding this comment.
- VARCHAR(45)인데 영문, 한글, 이모티콘 등등일 때 각각의 최대 사이즈에 대해 찾아보면 어떨까요??
- 1을 바탕으로 Name의 최대값도 검증해보면 어떨까요??
There was a problem hiding this comment.
차지하는 공간에 대해 학습을 해보니,
영문은 한 글자당 1바이트를 차지, 한글은 한 글자당 3바이트 차지, 이모지는 한 개당 4바이트를 차지함을 확인했습니다!
정리하면 VARCHAR(45) 기준으로
한글은 최대 15자,영문은 최대 45자,이모지는 최대 11개입력을 할 수 있습니다.
따라서 Name의 최대값 검증에서 이를 이용해서 검증을 진행해보겠습니다.
(이모지가 4바이트로 가장 크니 최대 10자의 제한을 걸어버리면 한글, 영어, 이모지 모두 10자까지 가능합니다!)
시카, 안녕하세요! 제이입니다.
이번 미션을 구현하면서 다음과 같은 궁금즘이 있었습니다.
저는 상품을 수정 혹은 삭제를 할 때
id값을 기준으로 먼저 조회를 해서 null값인지 체크하고 예외를 발생시키고, 값이 있다면 수정 혹은 삭제의 과정을 진행합니다.사실 update 혹은 delete 쿼리를 한 번만 날려도 될 것 같아서 바꿀 수는 있었지만, 그렇게 되면 입력에서 오는 예외 값을 처리할 수 없었기 때문에 바꾸지 않았습니다.
그래서 저의 질문을 정리하면 다음과 같습니다.
또 다른 질문으로는,
RestAssured를 사용하면서 트랜잭션이 적용되지 않아서 테스트에 들어간 데이터가 롤백되지 않는 문제를 확인했습니다.그래서 일단은 AfterEach로 테스트가 종료될 때마다 데이터를 다 지워버리는데 이렇게 했을 때, 혹시나 다른 문제점이 없을지 궁금합니다.
그리고 또다른 질문으로는
/products/{id}와 같이 param으로 id값을 전해주는 것이 좋을지, dto에 id 값을 한 번에 담아서/products로 보낼지 고민이 됐습니다.일단 RESTful 원칙에 입각하여
/products/{id}방식으로 진행했지만, 이런 문제점이 우려되었습니다.마지막 질문으로
RequestDto에 기본 생성자만을 구현했습니다.
모든 인자를 받는 생성자가 비즈니스 로직에서 사용되는 일이 없어서 큰 문제가 되지 않았지만, 테스트하는 과정에서 테스트가 너무 힘들다는 문제가 있었습니다.
처음에는 Mock을 이용해서 스터빙을 통해서 getName() 과 같은 것을 임의로 주었지만, 결국에 테스트가 불안정해질 수 있기 때문에 모든 인자를 받는 생성자를 추가하였습니다.
이에 대해서 어떻게 생각하시나요?