Conversation
Rok93
left a comment
Rok93
left a comment
There was a problem hiding this comment.
수민님 안녕하세요. 도메인 주도 설계 기본 요소 미션을 함께하게된 김경록입니다.
1단계 미션 잘 구현해주셨습니다만 현재 애플리케이션이 기동하지 않는 것으로 보이네요. 😢
애플리케이션이 기동되는 상태로 변경 부탁드릴게요.
추가적으로 기존의 API를 리팩터링하신 코드로 대체해보시면 좋을 것 같아요.
또 테스트 코드 또한 기존의 서비스 로직이 아닌 리팩터링한 서비스 로직으로 대체했을 때, 기존의 동작을 동일하게 수행하는지를 확인해주셨으면합니다. 🙏
이 미션은 실제로 서비스중인 코드를 수정한다고 생각하고 최대한 리팩터링을 진행해주시면 좋을 것 같아요. 😃
그 외에도 몇몇 코멘트 남겨두었으니 확인 부탁드릴게요.
| import java.util.Objects; | ||
| import java.util.UUID; | ||
|
|
||
| @Service |
There was a problem hiding this comment.
현재 애플리케이션 기동이 안되고있는 것 같아요.
다음 리뷰 요청 때는, 애플리케이션이 기동되는 상태가 되도록해주세요. 😃
| } | ||
|
|
||
| @Transactional | ||
| public Product create(final Product request) { |
There was a problem hiding this comment.
기존의 서비스 로직을 사용하던 위치들을 리팩터링한 로직들을 대신 사용하도록 변경해주세요.
지금의 상태에서는 리팩터링한 코드들이 이전처럼 잘 동작할지에 대한 확신이 들지 않는데요. 🤔
테스트 코드 또한 변경된 ProductService로 대체해주신 뒤, 테스트 코드가 잘 돌아가는지 확인이되면 좋을 것 같아요. 😃
| throw new IllegalArgumentException(); | ||
| } | ||
| final Product product = productRepository.findById(productId) | ||
| .orElseThrow(NoSuchElementException::new); |
There was a problem hiding this comment.
예외가 발생했을 때, 적절한 에러 메시지를 던져준다면 에러메시지를 통해서 문제의 원인을 파악하기가 더 수월해지지 않을까요? 😃
| .orElseThrow(NoSuchElementException::new); | |
| .orElseThrow(() -> new NoSuchElementException("상품이 존재하지 않습니다. productId: " + productId)); |
| import jakarta.persistence.Embeddable; | ||
| import kitchenpos.products.infra.PurgomalumClient; | ||
| @Embeddable | ||
| public class Name { |
There was a problem hiding this comment.
README.md에 적힌 용어 사전, 모델링을 봤을 때는 Name이 아닌 DisplayedName으로 정의해야할 것 같은데요. 🤔
혹시 이전 미션 코드 or 문서를 가지고와야하는 상황이라면 해당 PR close 후, 0단계 PR을 올려주세요!
| } | ||
| final Product product = productRepository.findById(productId) | ||
| .orElseThrow(NoSuchElementException::new); | ||
| product.setPrice(price); |
There was a problem hiding this comment.
setter를 이용하기보다는, 변경의 의미를 드러낼 수 있도록 적절히 메서드명을 네이밍해보면 좋을 것 같습니다.
| product.setPrice(price); | |
| product.changePrice(price); |
|
|
||
| @Entity | ||
| @Table(name = "product") | ||
| public class ProductEntity { |
| import jakarta.persistence.Embeddable; | ||
| import kitchenpos.products.infra.PurgomalumClient; | ||
| @Embeddable | ||
| public class Name { |
| import java.math.BigDecimal; | ||
| import java.util.Objects; | ||
| @Embeddable | ||
| public class Price { |
There was a problem hiding this comment.
추후 다른 컨텍스트에서도 Price가 등장할 것 같은데요.
모두 같은 Price를 쓰는걸까요? 그게 아니라면 네이밍적으로 구분을 해두는게 좋지 않을까요? ex) ProductPrice
| final BigDecimal price = request.getPrice(); | ||
| if (Objects.isNull(price) || price.compareTo(BigDecimal.ZERO) < 0) { | ||
| throw new IllegalArgumentException(); | ||
| } | ||
| final String name = request.getName(); | ||
| if (Objects.isNull(name) || purgomalumClient.containsProfanity(name)) { | ||
| throw new IllegalArgumentException(); | ||
| } |
There was a problem hiding this comment.
Price, Name 값 객체를 생성하기만해도 이 검증은 자연스럽게 수행될 것 같은데요. 🤔
두 값 객체를 생성하여 product의 생성자 파라미터를 통해 주입해서 Product 객체를 생성하도록 변경해보시면 좋을 것 같습니다. 😉
|
안녕하세요 @Rok93 리뷰어님 ! 혹시 이런 상황일 때는 |
|
@sumiini 님 안녕하세요. 기존 Product에 의존하는 로직들이 많다보니 말씀하신 것처럼 어디서부터 수정해야할지 고민이되실 수도 있을 것 같아요. 🤔 저라면 Product의 내부의 상태 값의 타입은 바뀌더라도, 기존의 동작을 유지하도록 할 것 같아요. 생성자 오버로딩, getter, setter의 반환 타입 혹은 메서드 파라미터를 기존과 동일하게 받되, 내부적으로는 VO 객체로 변환해서 할당하도록 수정한다거나, (물론 추후 setter는 가급적 제거를 하도록 리팩터링하고 싶을 것 같아서, 아마 Fixture함수도 지금은 setter를 통해서 fixture를 생성하고있을 것 같은데 setter가 기존처럼 사용해도 동일하게 동작하도록 호환성만 유지해준다면 당장에 컴파일 에러가 나는 부분은 적을 것 같아요. 🤔 일단 지금처럼 최대한 기존의 로직들이 동작하도록 변경을 가하고 또 다음 단계로 하나씩 수정해나가는 방향으로 점진적인 리팩터링을 해가는 것이 좋다고 생각해요. 정말정말 최후의 수단으로 너무 의존성이 많이 엮여있어 어렵다면 별도의 Product 객체를 만들어서 완전히 새것으로 갈아 엎은 뒤, 변경을 해줄 것 같네요. |
2 participants
안녕하세요 리뷰어님.
products 패키지 하위에 tobe.domain 패키지를 만들었습니다.
JpaProductRepository 구현체 생성하였습니다.
잘 부탁드립니다 🙂