-
Notifications
You must be signed in to change notification settings - Fork 178
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
1단계 - 리팩터링(상품) #201
1단계 - 리팩터링(상품) #201
Conversation
- legacy 코드와 분리하기 위해 기존 파일을 모두 legacy/ 에 배치함 - Product 도메인 생성 - 0원 이상이라는 규칙을 갖는 ProductPrice 생성 - notEmpty 규칙을 갖는 Name을 통해 비속어가 없는 규칙까지 포함한 ProductName을 생성하는 ProductNameFactory추가 - 상품 가격이 변경될 때 메뉴의 노출 여부를 조정하기 위해 event를 발행하고, menu는 rearranger를 통해 메뉴의 노출여부를 재조정함
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.
안녕하세요. 🙇
상품 리팩토링 진행하시느라 고생하셨어요.
Code를 보니 누락된 Commit이 있는 것 같은데요
다시 확인 후 리뷰 요청해주세요.
@wkdchdaud123 말씀주시면 보강하도록 할게요! +) 에고 상품 조회가 빠졌군요. 추가하고 리뷰요청드릴게요! |
|
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.
바쁘신 와중에도 미션하시느라 고생이 많으세요 👍
몇가지 코멘트 달아놓았어요 🙇
확인부탁드려요
import kitchenpos.product.domain.Name; | ||
import kitchenpos.product.domain.ProductPrice; | ||
|
||
public interface ProductUseCase { |
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.
도메인 계층만 리팩토링 대상에 해당되기 때문인가요? 😄
우선 코드를 보고 헥사고날 아키텍처를 적용하신 것으로 이해를 했어요.
따라서 저는 port.out과 adapter.in, adapter.out이 있을 것으로 기대를 했는데 존재 하지 않는 것 같아요 😄
리뷰이님의 의중이 궁금하네요 😄
|
||
private long value; | ||
|
||
public static ProductPrice of(final long value) { |
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.
상품 도메인 리팩토링 결과로 빈약한 도메인에서 풍부한 도메인으로 변경되어 가는군요!
테스트 수정 및 추가가 필요하지 않을까요? 😄 (가격은 0보다 작을 수 없다
)
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.
테스트코드 추가했습니다 :)
@AttributeOverride(name = "value", column = @Column(name = "price")) | ||
private ProductPrice price; | ||
|
||
public static ProductNew newOf(final ProductName name, final ProductPrice price) { |
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.
중복을 임시로 피하시기 위해 ProductNew라는 이름을 사용하셨지만 그래도 정적 팩토리 메소드 이름까지 newOf일 필요가 있을까요? 😄
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.
앗 newOf
가 productNew 여서 new인건 아니고,
새로운 엔티티 생성은 newOf가 담당한다
는 의미를 전달하고 싶었어요!
저는 보통 생성의 책임은 private 생성자
에게 부여하고
무언가 책임을 갖고 생성을 위임하는 건(여기선 새로운 엔티티를 만든다는 책임) 정적 팩토리 메소드 of
로 만드는 편인데요.
JPA는 기본생성자가 필수로 있어야 하더라구요? (JPA 처음입니당..)
JPA기술로 인해 기본 생성자가 열려버려서, 어떻게 제한할까 하다가 새로운 엔티티 생성은 newOf를 통해 하라
는 의미를 전달하고 싶었습니다 :)
혹시 newOf가 와닿지 않을까요~?
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.
관례적으로 사용하는 정적 팩토리 메소드의 네이밍을 사용하면 어떨까요?
이펙티브 자바에서도 나오는 내용이고, 저희가 자주 사용하는 컬렉션 프레임워크 및 등의 기본 라이브러리의 구현을 보면 관례적인 네이밍을 가지고 가죠.
더불어 ProductName, ProductPrice에서는 of를 사용하고 있어 더 헷갈릴 수 있겠어요. 😄
https://github.com/next-step/ddd-tactical-design/pull/201/files/ad9adfb75b12fefecb7eb912643ae4b9585a491e#diff-869a3dd2f37fb9e41b0dc8e7a1e2b7eefebc462a21e63fc81eef42aec3dd64f3R14
https://github.com/next-step/ddd-tactical-design/pull/201/files/ad9adfb75b12fefecb7eb912643ae4b9585a491e#diff-47ac6760d583e35feb4e10512214b08da617990693d6df7483466fc745ff1ac5R13
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.
- 새로운 domain entity가 생성된다는 관점
- JPA 기술로 인하여 열려버린 기본생성자를 제한하고 싶음
- 프로젝트의 통일성 (domainFactory 에서도 create를 사용중임 (ProductNameFactory#create))
저는 위와 같은 관점으로 바라보고 있으니 오히려 create() 가 좋을 것 같네요!
return Objects.hash(id); | ||
} | ||
|
||
private ProductNew(final UUID id, final ProductName name, final ProductPrice price) { |
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.
코드는 글을 읽는 것과 동일하다고 생각해요.
클린코드의 형식 맞추기에서는 자바 참고해서 메소드 순서를 조정해볼 수 있을 것 같아요 😄
https://effortguy.tistory.com/193
대부분 관례적 위와 동일하게 하는데요
전체적으로 보면 메소드의 순서에 대한 통일성이 없는 것 같아요 😄
다시 고민해보고 독자를 위한 방향으로 수정해보아요
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.
구글 포매터를 써서, 구글에선 어떤 가이드인지 보았는데 딱히 없네요 'ㅂ'
https://google.github.io/styleguide/javaguide.html#s3.4-class-declaration
가이드 주신대로 오라클 가이드 기반으로 수정하겠습니다 :)
|
||
product.changePrice(price); | ||
|
||
publisher.publish(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.
전체적으로 변수명에 대해서 독자를 위해 생각해보면 좋을 것 같아요 😄
https://tecoble.techcourse.co.kr/post/2020-04-24-variable_naming/
가령 다른 이벤트를 발행하는 것이 생기면 그때는 publisher라는 변수명은 어떻게 되어야 하나요?
publisher.publish가 독자에게 전달해주는 의미는 뭔가요?
독자가 순서를 따라 흐름대로 글을 읽다가 다시 이전 페이지로 돌아가서 읽어야 하는 수고가 생기지는 않을까요? 😄
publisher외 다른 변수들도 전체적으로 고민을 해보는게 좋을 것 같아요 ㅎㅎ
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.
앗 클래스이름이 표현해 준다고 생각했어요 (ProductPriceChangeEventPublisher publisher)
하지만 말씀하신대로 다른 이벤트를 발행하는 것이 생기면 그때는 publisher라는 변수명
은 매우 모호할 것 같습니다.
다시 한 번 보니 지금 UseCase 자체가 SRP를 지키고 있지 않아서 혼란스러운 상황이 생기는 것 같아요.
UseCase 를 분리하여 ProductPriceChangeUseCase
로 분리가 된다면,
다른 이벤트를 발행할 일이 없고, 그 때는 클래스이름
이 문맥을 한정지어주니 변수명을 간소하게 가져가도 될 것 같은데, 어떻게 생각하실까요?
(코드에 한번 반영해보겠습니다~!)
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.
네 그렇게 하는 것도 좋을 것 같아요 다만
publisher.publish는 독자가 코드를 읽는데 있어 어떠한 도움도 주지 않는 것 같아요 😄
저라면 productPriceChangeEvent라는 이름을 사용하겠습니다.
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.
오 productPriceChangeEvent.publish(id)
좋네요!👍
|
||
private final String value; | ||
|
||
public Name(final String value) { |
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.
ProductName과 Name을 분리함으로써 어떤 이점을 얻을 수 있나요? 😄
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.
1. 멘탈모델의 일치
Name
은 대부분의 사람이 가진 이름
이라는 멘탈모델과 일치하는 개념을 부여했어요.
(없을 수 없다)
ProductName
은 상품 컨텍스트에서의 이름
이라는 개념을 부여했어요
(Name이긴 하지만, 비속어가 포함되지 않는다)
2. 도메인 모델의 순수성 보장
ProductName
은 비속어가 포함되지 않는다
는 규칙을 가지는데요,
ProductName
을 전역적으로 사용하게 된다면,
ProductName
내에서 ProductPurgomalumChecker
를 통해 비속어 검사를 해줘야 합니다.
하지만 이는 도메인 모델의 순수성을 해친다고 생각했고, ProductName의 생성은 ProductNameFactory를 통해서만 할 수 있게 제한했습니다.
그러다보니 ProductName
이 생성되기 전, 이름
을 표현할 수 있는 또 다른 VO가 필요하게 되어서 Name
을 만들게 되었고, 인자명은 nameCandidate
로 명명하게 되었습니다. (productNameCandidate가 더 나을 것 같기도 하네요)
결론 : 사람들의 멘탈모델 일치와, 도메인모델의 순수성 보장이 제가 생각하는 이점입니다 :) 틀린 부분이 있거나 다른 의견이 있으면 말씀주세요 :)
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.
그렇군요!
-
저는 지금 리뷰이님께서 말씀해주신 내용을 듣지 않고는 "왜 이 컨텍스트에 Name은 2개가 있을까?"
ProductName과 Name 다른 뜻의 차이를 가지고 있을 것이라고 생각을 했어요.
더불어 지금 용어 사전에도 해당 내용이 정리가 되어 있지 않기 때문에 예상이 불가능했죠 ㅎㅎ
그리고 하나의 컨텍스트에서 꼭 동일한 용어에 대해 다른 의미로 다루어 진다면 컨텍스트가 분리가 될 수 있는 신호라고 볼 수 도 있었죠?
정말 그래도 Name이라는 명칭을 쓰는게 좋을까요? Infra로 부터 분리하기 위한 DTO 같은 것이 필요하셨던 것은 아닌가요? 😄 -
ProductName에서 ProductPurgomalumChecker를 통해 비속어 검사를 하면 왜 도메인 모델의 순수성을 해칠까요?
이미 port로 구성 해주셨기에 변경에 자유롭지 않을까요?
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.
제가 생각한 도메인모델의 순수성에 대해 이야기하면 의견주신 1,2번 모두 답변이 될 것 같습니다~
먼저 순수성에 대해 이야기해보자면,
제가 생각하는 순수한 도메인 모델은
외부세계와 통신하는 것이 없고 primitive type이나 또 다른 도메인 모델에만 의존하고 있는 상태
라고 생각합니다.
이런 순수한 도메인 모델을 통해 도메인을 쉽게 이해할 수 있게 해주고 스터빙 없는 단위테스트까지 가능하게 하여 전체적인 비즈니스를 이해하는데 복잡성을 줄여준다고 생각합니다.
ProductPurgomalumChecker
은 domain layer의 밖에 있고 결국 외부세계와 이어져있기 때문에 도메인 모델인 ProductName
이 ProductPurgomalumChecker
를 갖고 있으면 순수하지 않은 상태라고 보았습니다.
다만 이렇게 순수성을 따지게 되면, ProductName
이 비속어를 포함하지 않는다
는 규칙을 완전하게 가지고 있을 수가 없습니다.
그래서 ProductName 생성자
의 접근제어자를 default로 제한했고, ProductName
의 규칙을 지켜서 만들어주는 ProductNameFactory#create()을 만들었습니다.
(즉, 만들어진 ProductName
의 사용은 자유롭더라도 생성은 ProductNameFactory#create()을 통하여 만들 수밖에 없게 강제하였습니다.)
위와 같은 생각으로 ProductName
의 생성은 Domain Layer로 한정지었기 때문에
ProductName
이 될 무언가가 필요했구요.
Name
이라는 ProductName
이 될 수 있는 후보자를 만들게 되었습니다.
물론 말씀하신대로 infra로 부터 분리하는 관점만 갖는다면 String name으로도 충분할 것 같습니다.
하지만 VO로써 존재하는 것이 강력하다고 생각하기 때문에
Name을 global VO로 올리고, Product, Mene의 후보가 된다는 규칙을 부여하고 용어집에도 작성하였습니다 :)
(전체적인 용어집 정리는 필요할 것 같네요. 점진적으로 용어집은 수정해가겠습니다)
@wkdchdaud123 분명히 푸시를 하는데 왜 인텔리제이가 이렇게 Ignored 색깔을 보여주지 했는데.. ㅋㅋㅋㅋㅋ 설마해서 봤더니 이렇게 되어있네요 ( |
- 클래스 내 선언순서 조정 - 파라미터 이름 변경
- ProductUseCase에 있는 책임을 분리함
- 도메인 테스트코드 작성 - 파라미터 유효성 검사를 위한 Utils 생성
- 누락된 테스트코드 추가 - ContainsProfanityException 추가
- usecase 테스트코드 작성 - 테스트 도우미 fixtures, accessor 추가
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.
피드백 잘 반영해주셨어요 👍
몇가지 코멘트 남겨두었어요.
확인부탁드려요 🙇
#201 (comment)
이쪽도 함께 확인해주세요.
import org.junit.jupiter.params.ParameterizedTest; | ||
import org.junit.jupiter.params.provider.NullSource; | ||
|
||
@DisplayNameGeneration(ReplaceUnderscores.class) |
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.
👍
@AttributeOverride(name = "value", column = @Column(name = "price")) | ||
private ProductPrice price; | ||
|
||
public static ProductNew newOf(final ProductName name, final ProductPrice price) { |
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.
관례적으로 사용하는 정적 팩토리 메소드의 네이밍을 사용하면 어떨까요?
이펙티브 자바에서도 나오는 내용이고, 저희가 자주 사용하는 컬렉션 프레임워크 및 등의 기본 라이브러리의 구현을 보면 관례적인 네이밍을 가지고 가죠.
더불어 ProductName, ProductPrice에서는 of를 사용하고 있어 더 헷갈릴 수 있겠어요. 😄
https://github.com/next-step/ddd-tactical-design/pull/201/files/ad9adfb75b12fefecb7eb912643ae4b9585a491e#diff-869a3dd2f37fb9e41b0dc8e7a1e2b7eefebc462a21e63fc81eef42aec3dd64f3R14
https://github.com/next-step/ddd-tactical-design/pull/201/files/ad9adfb75b12fefecb7eb912643ae4b9585a491e#diff-47ac6760d583e35feb4e10512214b08da617990693d6df7483466fc745ff1ac5R13
|
||
private final String value; | ||
|
||
public Name(final String value) { |
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.
그렇군요!
-
저는 지금 리뷰이님께서 말씀해주신 내용을 듣지 않고는 "왜 이 컨텍스트에 Name은 2개가 있을까?"
ProductName과 Name 다른 뜻의 차이를 가지고 있을 것이라고 생각을 했어요.
더불어 지금 용어 사전에도 해당 내용이 정리가 되어 있지 않기 때문에 예상이 불가능했죠 ㅎㅎ
그리고 하나의 컨텍스트에서 꼭 동일한 용어에 대해 다른 의미로 다루어 진다면 컨텍스트가 분리가 될 수 있는 신호라고 볼 수 도 있었죠?
정말 그래도 Name이라는 명칭을 쓰는게 좋을까요? Infra로 부터 분리하기 위한 DTO 같은 것이 필요하셨던 것은 아닌가요? 😄 -
ProductName에서 ProductPurgomalumChecker를 통해 비속어 검사를 하면 왜 도메인 모델의 순수성을 해칠까요?
이미 port로 구성 해주셨기에 변경에 자유롭지 않을까요?
checkNotNull(id, ID); | ||
checkNotNull(price, PRICE); |
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.
추가로 이런 유효성 검사는 리팩토링을 통해 풍부한 도메인 모델로 변경된 곳에서 하는 게 적절할까요?
아니면 application에서 하는게 적절할까요?
리뷰이님의 생각이 궁금합니다.
만약 다른 유스케이스가 추가되는 곳에서 해당 유효성 체크를 추가해줘야 하는데 누락할 수 도 있겠군요?
추후 요구사항이 변경되어 제거될 필요가 있는 유효성 체크를 제거해야할 때는 어떨까요?
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.
유효성 검사는 리팩토링을 통해 풍부한 도메인 모델로 변경된 곳에서 하는 게 적절할까요? 아니면 application에서 하는게 적절할까요?
이 유효성 검사는 java언어의 한계로 인해 발생하는 문제를 막고 퍼블릭 인터페이스 계약을 지키기 위한 것입니다.
ProductPriceChangeUseCase#change(@NotNull final UUID id, @NotNull final ProductPrice price)
위와 같은 인터페이스 계약이 있을 때, 이를 보장해주고 & 계약이 지켜지지 않았을 때 빠른 실패 처리를 할 수 있게 합니다.
비즈니스를 갖지 않고 오로지 public interface에 대한 유효성검사만 수행해주므로 ParameterValidateUtils
라는 클래스 이름을 갖게 했고, 도메인 모델에 대한 검사는 이 클래스를 통하지 않고 도메인 안에서 따로 수행해주고 있습니다.
다른 유스케이스가 추가되는 곳에서 해당 유효성 체크를 추가해줘야 하는데 누락할 수 도 있겠군요?
네네 이건 같이 개발하는 사람 끼리의 합의 정도로밖에 강제할 수 없어서 충분히 누락할 수 있습니다.
추후 요구사항이 변경되어 제거될 필요가 있는 유효성 체크를 제거해야할 때는 어떨까요?
말씀주신 상황은 public interface가 변경되었다는 의미이기 때문에 이 때는 당연히 구현 코드들도 변경되어야 한다고 생각합니다 :)
- of -> create로 변경
- eventPublisher.publish -> productPriceChangeEvent.publish 변경
업무와 병행하다보니 재리뷰 요청까지 좀 오래걸렸습니다 :) #201 (comment) |
VO를 명시적으로 표현하지 않아도 현재는 충분히 알 수 있습니다만, |
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.
피드백 잘 반영해주셨습니다 💯
많은 생각을 해볼 수 있는 미션이 되셨기를 바래요.
코멘트 추가한 내용이 있어요
확인 부탁드리며 다음 미션 진행을 위해 이만 머지할게요. 👍
지난 과제
next-step/ddd-strategic-design#320
next-step/ddd-strategic-design#329
지난 과제에서 만든 구조로 이번 과제를 진행했습니다.
사전작업
과제
Product 도메인 생성
질문