Skip to content
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단계 - 리팩터링(상품) #206

Open
wants to merge 2 commits into
base: kymiin
Choose a base branch
from
Open

Conversation

kymiin
Copy link

@kymiin kymiin commented Sep 12, 2023

상품 도메인 리팩토링 진행해 보았습니다.

첫 리뷰인데 잘 부탁드려요~

Copy link
Member

@Hyeon9mak Hyeon9mak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 경윤님, 리팩터링 미션을 함께할 최현구입니다. 🙇‍♂️
상품 리팩터링 잘 진행해주셨어요.
몇 가지 코멘트 남겼으니 확인해주세요.

Comment on lines 3 to 11
import kitchenpos.menus.domain.Menu;
import kitchenpos.menus.domain.MenuProduct;
import kitchenpos.menus.domain.MenuRepository;
import kitchenpos.products.domain.Product;
import kitchenpos.products.domain.ProductRepository;
import kitchenpos.products.tobe.domain.Product;
import kitchenpos.products.tobe.domain.ProductDomainService;
import kitchenpos.products.tobe.domain.ProductRepository;
import kitchenpos.products.infra.PurgomalumClient;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(다음 미션 내용이지만) products 패키지는 코드상에서 상품이라는 바운디드 컨텍스트를 나타내주는 방법이라고 생각해요.
상품 내부에서 다른 컨텍스트인 메뉴를 직접 참조하지 않도록 개선해볼 수 있는 방법이 여러가지 있을텐데요,
여러가지 방식을 고민해보시면서 리팩터링 해보는 것도 재밌을 것 같아요.

public BigDecimal getChangedPrice() {
return changedPrice;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UNIX의 표준으로는 파일 마지막에 개행이 있어한다고 해요. 잠재적인 문제가 될 수 있기 때문에 Github 에서도 EOF 알림을 띄우는 것 같아요. (https://seongwon.dev/Git/20220303-%ED%8C%8C%EC%9D%BC%EC%9D%98_%EB%A7%88%EC%A7%80%EB%A7%89_%EA%B0%9C%ED%96%89/)

Editor > General > Ensure every saved file ends with a line break 를 체크하시면 파일 마지막에 자동으로 개행이 적용됩니다.

image

Comment on lines +32 to +34
protected Product() {
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

기본생성자 접근제한 막아주기 👍 👍

Comment on lines +15 to +18
@Id

@Column(name = "id", columnDefinition = "binary(16)")
private UUID id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id 를 통해 나머지 값들의 변화를 추적하는 엔티티 객체이므로, id 에 대한 동등성을 부여해줘도 좋겠네요.

Comment on lines +21 to +26
@Transactional
public void changePrice(Product product, BigDecimal price) {
product.changePrice(price);
eventPublisher.publishEvent(new ProductPriceChangedEvent(product.getId(), price));

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 이벤트에서 변경된 가격을 함께 가져가고 있는데요,
만약 상품 가격 변경과, 상품 가격 변경 이벤트 처리 사이에
다른 이유로 상품 가격이 또 변경되면 일관성이 깨진다는 부담도 존재하는 것 같아요.
경윤님은 이 부분에 대해 어떻게 생각하실까요?

Comment on lines +26 to +30
public Product(String name, BigDecimal price, PurgomalumClient purgomalumClient) {
this.id = UUID.randomUUID();
this.name = new ProductName(name, purgomalumClient);
this.price = new ProductPrice(price);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생성자에서 미리 포장해둔 VO 를 전달 받는 경우 Product 에서 PurgomalumClient 에 대한 의존이 사라질 수도 있곘어요!
다만 이 부분은 상위 계층에서 Product 외 다른 VO 들에 대한 의존이 시작된다는 의미이므로, 어느정도 트레이드 오프가 있겠어요.

Comment on lines +34 to +49
@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (!(o instanceof ProductName)) {
return false;
}
ProductName that = (ProductName) o;
return Objects.equals(name, that.name);
}

@Override
public int hashCode() {
return Objects.hash(name);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꼼꼼한 동등성 부여 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants