-
Notifications
You must be signed in to change notification settings - Fork 179
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
Feature/step1 #273
base: homekeeper89
Are you sure you want to change the base?
Feature/step1 #273
Conversation
아! 리뷰 후에 읽다보니 지금 PR 에 대해 제대로 이해했습니다 ㅎㅎ
이부분은 정답이 없는 영역이고 팀 컨벤션에 따라 달라질 것 같은데요!
엇 혹시 제가 이해를 잘 못해서 조금만 더 구체적으로 설명해주실수있을까요?
아! 혹시 장훈님이 생각하시는 바운디드 컨텍스트는 무엇일까요?
이부분은 강의를 들으셨으면 궁금증이 해결되셨을 것 같은데요! |
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.
장훈님 안녕하세요 😄
이번 미션을 함께할 노웅래입니다
우선 PR 빠르게 올려주셔서 감사해요 👍
코멘트 달아드렸는데 궁금하신점이나 고민이 있으시면 편하게 연락주세요!
src/test/java/kitchenpos/products/application/tobe/ProductTest.java
Outdated
Show resolved
Hide resolved
src/main/java/kitchenpos/products/tobe/domain/InMemoryProductRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/kitchenpos/products/tobe/domain/ProductService.java
Outdated
Show resolved
Hide resolved
delete pd from service, not to need handle status from service
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.
장훈님 안녕하세요 😄
피드백 잘 반영해주셨습니다!
코멘트 작성한 것이 있는데 확인부탁드려요!
src/main/java/kitchenpos/products/tobe/domain/entity/Product.java
Outdated
Show resolved
Hide resolved
src/main/java/kitchenpos/products/tobe/domain/InMemoryProductRepository.java
Outdated
Show resolved
Hide resolved
[chore] apply interface
말씀 하신 내용을 우선 적용해보았습니다. 이번에 하면서 고민 되던 부분은
감사합니다. |
저장하는 것이 정말 도메인적으로 중요한 것인가? 고민해보시면 좋을 것 같아요.
도메인 서비스의 경우 레이어간의 협력보다는 여러 애그리거트간의 협력이 필요할 때 혹은 정책과 같이 절차지향적으로 관리하는 것이 유지보수에 더 도움이 되는 케이스일 때 주로 도메인 서비스를 사용해요!
어플리케이션서비스에서 저장을 해주면 되지 않을까 생각해요.
네네 맞아요! 👍 👍 |
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 void register(ProductRepositoryImpl repo) { | ||
Product product = new Product(this.getId(), this.displayedName, this.price, new Profanity()); | ||
repo.save(product); | ||
} | ||
|
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.
넵
List<String> wrongWords = new ArrayList<>(); | ||
wrongWords.add("wrong-name"); | ||
|
||
if (wrongWords.contains(word)) { | ||
throw new IllegalArgumentException("The word '" + word + "' is not allowed."); | ||
} |
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.
현재 비속어 검증하는 Profanity 대신 직접 비속어를 List 로 관리하도록 구현해주신것같은데요!
Profatity 대신 직접 구현해주신 의도가 있을까요?
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.
아 이거 외부 서비스 대체한다 생각하고 그냥 임시코드로 구현한 것이긴 했는데
여기도 repository 처럼 인터페이스를 활용한 DIP 형태로 구현하면 되겠죠?
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.
네네 기존 레거시에 인터페이스와 구현체 모두 있어요! 그것을 활용해보시면 좋을 것 같아요!
Product product = optionalProduct.get(); | ||
Product changedProduct = new Product(productId, new DisplayedName(name), product.getPrice()); | ||
|
||
this.productRepository.update(changedProduct); |
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.
가격 변경에 대한 로직이 현재 Product 가 아닌 ProductService 에서 수행되고 있는데요!
Product 가 가격변경에 대한 행위를 해보는 것은 어떨까요?
anemic domain model 로 주로 미션해주신 것같아서 교육 목적을 위해 rich domain model 도 경험해보시면 좋을것같아요!
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.
즉 가격 변경에 대한 도메인 검증은 entity 에서 하고 결과를 서비스가 저장한다, 이정도로 구분하면 될까요?
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.
animic domain model 과 rich domain model 에 대해 한번 찾아보시면 좋을 것 같아요!
각각의 장단점을 확인해보시면 좋을 것 같아요!
안녕하세요! 리뷰어님 많이 늦었습니다 😭
요구사항에 따른 미션 내용 올립니다.
다만 보시다시피 다 구현한게 아니라서 계속 수정할 예정입니다.
중간에 먼저 의견을 구하기 위해 올리는점 양해 부탁드립니다.
(또한 제가 자바가 주 언어가 아닌점도 이해해주시면 감사하겠습니다)
구현위한 요구사항은 아래 첨부하였습니다.
제가 의도한 컨셉은 다음과 같으며 궁금한점도 같이 작성했습니다.
Entity 와 VO 의 개념적 구분
궁금증 1) 제가 레이어 구분에 너무 익숙해져 있어서 그런데, 이렇게 서로 다른 속성에 따른 구분은 디렉토리나 패키지를 어떻게 구분하나요?
상품 가격 변경시 메뉴에 속한 상품 금액 합 ... 요구사항은 바운디드 컨텍스트로 구분하려고 했으나 못함
궁금증 2) 서비스, 바운디드 컨텍스트 등은 디렉토리나 패키지 상으로 어떻게 구분하나요?
제가 이렇게 레이어에 집착(?) 하게 된 이유가 네이밍 구분들 때문에 그렇습니다.
만에 하나 같은 상품(Product) 라고 레이어를 구분한다면 ProductService, ProductBoundedContext 등 다양하게 사용할 수 있는데 이런 구분점이 없다면 이름 짓는것때문에도 힘들어서요 😭
�만약 이런 계층 구분이 없다고 한다면 로직의 논리적 순서를 이해하기 어려울것 같은데 이 부분에 대해선 어떻게 생각하시나요!
예) 일반적으로는 controller > service > repo 이런식의 흐름이라 생각할 수 있지만
계층 구조가 없다면 controller 가 service 를 부를 수도, bounded context 를 부를 수도 있고 service 아래 bounded context 가 올수도 있고 등등 이 구조를 처음 접하는 개발자들이 헷갈려 할 수도 있겠다 생각이 들었습니다.
궁금증 3) 2와 연관된 질문이긴 한데, 저는 repo 의 역할을 데이터를 저장하는 인프라와의 I/O, 일종의 어댑터와 같은 역할로 하여 비즈니스적 의미를 내포하기 위해선 service 와 같은 상위 레이어를 한번 래핑해야한다 생각했습니다. 그렇다 보니 ProductService 의 getList 나 findById 등은 사실상 byPass 하는 형태로만 구현이 되는데 이런식의 구현은 안티패턴일까요?