-
Notifications
You must be signed in to change notification settings - Fork 153
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
Step2 리펙터링(메뉴) 입니다. #103
base: baekseungyeol
Are you sure you want to change the base?
Step2 리펙터링(메뉴) 입니다. #103
Conversation
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.
2단계 미션 잘 해주셨어요 👍
몇 가지 코멘트 남겼어요~ 확인하고 다시 요청주세요 🙇
@@ -0,0 +1,25 @@ | |||
package kitchenpos.products.tobe.domain; | |||
|
|||
import kitchenpos.products.tobe.support.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.
Value 객체가 잘못된 것 같아요
support 패키지를 찾을 수 없네요
import javax.persistence.Embeddable; | ||
|
||
@Embeddable | ||
public class DisplayedName extends Value<DisplayedName> { |
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.
상속을 사용한 이유가 있을까요?
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.
앗 제가 Value 오브젝트를 빼먹고 안올렸었네요
해당 미션은 3주차 강의를 듣고 진행한거라 강의때 나왔던 Value 오브젝트를 직접 적용해보았는데요~
DisplayedName, MenuPrice 등 Value object라고 마커 인터페이스를 남길 수 있고 Value object에 기준인 동등성에 보장(equals, hashcode)을 한곳에 모아서 관리하기 위한 하나의 괜찮은 방법이라고 생각해서 구현 적용 해보았었습니다~
@@ -0,0 +1,71 @@ | |||
package kitchenpos.products.tobe.domain; |
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.
상품과 메뉴의 컨텍스트에 대해서 고민해보면 좋을 것 같아요
기존의 Menu 컨텍스트와 Product 컨텍스트가 하나의 바운디드 컨텍스트로 가져가도 되는지 고려해보면 좋을 것 같아요 :)
this(id, name, new MenuPrice(price), menuProducts); | ||
} | ||
|
||
public Menu(final UUID id, final DisplayedName name, final MenuPrice price, final List<MenuProduct> menuProducts) { |
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.
테스트외에 사용하지 않는다면 접근 제어자를 활용해 제어해보는 것이 어떨까요?
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.
현재 테스트에서만 사용하고 있지만 테스트에서 사용하려면 결국 public 으로 두어야 하지 않나요?? 어떤 방법으로 제어가 가능할까요?
} | ||
|
||
; |
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.
코드를 깔끔하게 정리해 보아요 :)
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.
피드백 반영 잘해주셨어요 👍
몇 가지 생각해볼 코멘트 남겼어요~
확인하고 다시 요청 주세요 🙇
궁금한 사항들이 있으면 언제든 슬랙을 통해 DM 주세요~
|
||
import javax.persistence.*; | ||
import java.util.List; | ||
|
||
@Embeddable | ||
public class MenuProducts { | ||
public class MenuProducts extends Value<MenuProducts> { |
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.
menu
도메인이 별도의 서버로 분리가 된다면 Value 에 대한 의존성은 어떨까요?
중복을 제거하기 위한 상속은 좋으나
BoundedContext
단위로 도메인을 생각해보면 좋을 것 같아요
import java.util.Arrays; | ||
import java.util.Objects; | ||
|
||
public abstract class Value<T extends Value<T>> { |
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.
추상화를 잘 표현해주셨어요 👍
하지만 util 및 support 와 같이 여러 객체에서 사용을 한다면 어떤 단점이 있을까요?
OOP 와 DDD 의 차이를 한번 생각해 보아요 :)
Step2 리펙터링(메뉴) 입니다! (__)