Skip to content

1단계 - 리팩터링 (상품)#326

Open
seovalue wants to merge 9 commits intonext-step:seovaluefrom
seovalue:step1
Open

1단계 - 리팩터링 (상품)#326
seovalue wants to merge 9 commits intonext-step:seovaluefrom
seovalue:step1

Conversation

@seovalue
Copy link

안녕하세요, 리뷰어님!

제가 미션을 잘 이해한게 맞는지 모르겠지만,
우선 저는 영속성엔티티와 도메인엔티티를 분리하는 데에 초점을 맞췄어요.

  • Product: 도메인 엔티티
  • ProductRecord: 영속성 엔티티
  • ProductRepository: 도메인 엔티티 반환
  • ProductJpaRepository: 영속성 엔티티 반환

그리고 Service로 들어가는 Input과 Output의 dto를 분리했어요.

다만 이 부분에서 현재 깨지는 테스트코드가 존재하는데요,
product.changePrice를 했을 때 menu의 display도 변경되어야하는데, product를 도메인엔티티로 변환하다보니 위에서 save를 호출해주더라도 menu까지 전파가 안되더라고요!ㅎㅎ
혹시 이 부분을 어떻게 해결할 수 있을지 의견을 주실 수 있으실까요?

fyi. kotlin이 더 익숙해서 kotlin과 java를 혼용했는데 이 점 양해부탁드립니다!

@sihoony
Copy link

sihoony commented Feb 17, 2025

tobe 패키지에 만들었다면 Menu와 충돌이 나지않았을 것 같아요~~ 😭
다시한번 진행해보면 어떨까요~? 😄

Copy link

@sihoony sihoony left a comment

Choose a reason for hiding this comment

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

안녕하세요~! 민정님,
프로젝트 오픈으로 인하여 리뷰가 늦어졌네요! 🙏
민정님의 의견을 듣고싶은 부분, 개선했으면 좋을 것 같은 부분에 코멘트를 남겼어요.
생각해보시고 반영해주시면 됩니다! 즐거운 하루 되세요~! 😄

@@ -1,11 +1,17 @@
package kitchenpos.products.application;
Copy link

Choose a reason for hiding this comment

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

이번 미션 항목들은 각 kitchenpos.products.tobe.* 위치해야해요~!

@@ -0,0 +1,18 @@
package kitchenpos.products.domain

class ProductName private constructor(
Copy link

Choose a reason for hiding this comment

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

VO로 분리해주셨네요~! 👍


import java.math.BigDecimal

class ProductPrice private constructor(
Copy link

Choose a reason for hiding this comment

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

👍 👍

@Table(name = "product")
@Entity
public class Product {
public class ProductRecord {
Copy link

Choose a reason for hiding this comment

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

분리의 의도에 따라서 다르겠지만, 영속성엔티티와 도메인엔티티가 같은 패키지에 있어야할지도 고민해보면 좋을 것 같습니다!!
민정님이 생각하시는 의도는 무엇일까요~!?

Comment on lines +14 to +24
override fun findById(id: UUID?): Optional<Product> {
TODO("Not yet implemented")
}

override fun findAll(): MutableList<Product> {
TODO("Not yet implemented")
}

override fun findAllByIdIn(ids: MutableList<UUID>?): MutableList<Product> {
TODO("Not yet implemented")
}
Copy link

Choose a reason for hiding this comment

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

아직 구현하지 못한 부분일까요~!? 불필요한 부분일까요~!?

@@ -1,16 +1,21 @@
package kitchenpos.products.application;
Copy link

Choose a reason for hiding this comment

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

Product 도메인을 만들면서 각각의 테스트를 만들어보면 어떨까요~!?
ProductName, ProductPrice, Product가 대상이 될 수 있겠네요~! 😄

import java.util.*

@Repository
class ProductRepositoryImpl(
Copy link

Choose a reason for hiding this comment

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

Repository와 RepositoryImpl를 패키지로 분리해보면 어떨까요~!?
저는 컨버팅까지는 너무 오버스펙으로 생각하고 인터페이스와 구현체에 대한 명확한 분리를 통해서 Domain을 유지하고 있습니다~!

@seovalue
Copy link
Author

ㅋㅋㅋ제가 요구사항을 눈감고 읽었군요.. tobe로 옮기고 주신 리뷰 반영해서 새로 해보겠습니당

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.

2 participants