Skip to content
This repository has been archived by the owner on Aug 13, 2022. It is now read-only.

[#21] Item domain 개발 #27

Open
wants to merge 20 commits into
base: feature/23
Choose a base branch
from
Open

[#21] Item domain 개발 #27

wants to merge 20 commits into from

Conversation

jjeda
Copy link
Collaborator

@jjeda jjeda commented Oct 4, 2019

  • 상품엔티티 개발
  • 상품 리포지토리 개발
  • 상품 서비스 개발

- 상품엔티티 개발
- 상품 리포지토리 개발
- 상품 서비스 개발
@jjeda jjeda requested a review from f-lab-dev October 4, 2019 10:35
@jjeda jjeda self-assigned this Oct 4, 2019
this.price = price;
this.stockQuantity = stockQuantity;
this.account = account;
account.getItems().add(this);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@f-lab-dev
JPA에서는 연관관계의 주인쪽에서만 write 가 가능한것으로 알고있습니다. 따라서 주인쪽에서 save 로직을 실행하면 다른쪽에서도 DB 콜이 되는것으로 알고있습니다. 하지만 객체지향적으로는 양쪽다 넣어주어야하고 보통 묶어서 메서드를 만든다고 공부하였습니다
52 ~53 line 처럼 생성시에 53번 라인처럼 넣어주는것이 적절한지 궁금합니다

Copy link
Member

Choose a reason for hiding this comment

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

이렇게 하면 비즈니스 로직이 엔티티에 들어가게 됩니다. 분리한다음 해당 로직은 Item을 생성하는 코드에서 트랜잭션을 통해 묶어주는게 좋을 것 같네요.

@Embeddable
@AllArgsConstructor
@NoArgsConstructor
@NoArgsConstructor(access = AccessLevel.PROTECTED)
Copy link
Member

@f-lab-dev f-lab-dev Oct 5, 2019

Choose a reason for hiding this comment

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

주석까지 너무 좋습니다 👍


public Category(String name) {
this.name = name;
this.itemCategories = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

다른 파일에서는 리스트를 선언구문에서 초기화를 해주셨는데 여기서는 생성자에서 해주신 이유가 있을까요?

this.price = price;
this.stockQuantity = stockQuantity;
this.account = account;
account.getItems().add(this);
Copy link
Member

Choose a reason for hiding this comment

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

이렇게 하면 비즈니스 로직이 엔티티에 들어가게 됩니다. 분리한다음 해당 로직은 Item을 생성하는 코드에서 트랜잭션을 통해 묶어주는게 좋을 것 같네요.

this.itemCategories = new ArrayList<>();
}

public void addItemCategory(ItemCategory itemCategory) {
Copy link
Member

Choose a reason for hiding this comment

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

이 메소드 또한 엔티티의 역할이 아니라 비즈니스 로직으로 생각해야할 것 같네요~

/**
* 상품을 주문 or 취소 시 stock 감소 or 증가 로직
*/
public void addStock(int quantity) {
Copy link
Member

Choose a reason for hiding this comment

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

이런 오퍼레이션들은 모두 서비스 레이어에서 해주는게 좋을 것 같습니다

private final ItemRepository itemRepository;

@Transactional
public void saveItem(Item item) {
Copy link
Member

Choose a reason for hiding this comment

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

제 생각으로는 Item 엔티티는 ItemService에서만 사용되어야하고, 클래스 외부에서 입력받거나 클래스 외부로 리턴해줄때에는 별도의 DTO로 변환해주어야 할 것 같습니다.

엔티티 객체는 하이버네이트의 프록시객체라서 외부에 그대로 공개할 경우 잘못된 연산을 행할 걱정이 많은 객체입니다.
(이 클래스를 사용하는 다른 클래스가 전부 재웅님이 짠 코드라도 ItemService의 관점에서 봐야합니다. 즉 ItemService 이외의 클래스는 모두 외부라고 보셔야합니다)
여기나 다른 부분도 Item 엔티티는 다른 클래스에서 그 존재를 알 수 없도록 외부에 공개하지 않는게 좋을 것 같네요

Copy link
Member

Choose a reason for hiding this comment

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

추가적으로 다른 리뷰와 엮어서 생각해보면 이 메소드에 비즈니스 로직이 없이 save만 해주는 이유가 책임의 분리가 잘 안돼있기 때문입니다.

받은 정보를 바탕으로 Item 엔티티를 생성하고 저장하는 행위는 이 메소드의 역할인 것 같은데 다른 메소드에서 Item 엔티티를 생성하고 있으니 실제 비즈니스로직이 나오지 않는 것입니다.

객체지향책을 보면서 더 공부해볼 예정이지만 이런 메소드에 책임에 대해서도 고민해보시면 좋을 것 같습니다~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

도메인 모델 패턴에 대한 글을 읽고 엔티티에 비즈니스 로직을 담아서 해보려고했는데 레이어드 아키텍처도 잘 이해하지 못하고 하려니 어렵네요.. DTO 생성해주고 서비스단에서 비즈니스로직 만드는 코드로 다시 바꿔보겠습니다~ DDD는 다음 레벨에서..

primary key (order_item_id),
foreign key (item_id) references item (item_id),
foreign key (order_id) references orders (order_id));
count integer not null,
Copy link
Member

Choose a reason for hiding this comment

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

수량은 quantity로 표현해주는게 좋을 것 같습니다~
개인적으로는 count같은 예약어를 사용하는 것은 피하려고 하고있습니다.

jjeda added 4 commits October 6, 2019 21:35
- ItemDto 추가
- 비즈니스 로직 entity -> service
- 주문 관련 domain Dto 생성
- 주문 레포지토리 생성
- 주문 서비스 생성
- 주문 생성 & 취소 메서드
- 주문 서비스 수정
- 주문 컨트롤러 추가
- Buyer / Seller 주문관련 컨트롤러 분리
- Dto 메서드 이름 수정
- 서비스 내 정적 메서드 수정

private List<ItemCategory> itemCategories;

public Item from() {
Copy link
Member

Choose a reason for hiding this comment

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

여기도 이름을 toEntity 정도로 수정해주시면 좋을 것 같습니다

@@ -29,4 +43,25 @@ public void saveItem(Item item) {
public List<Item> salesList(Long accountId) {
return itemRepository.findAllByAccountId(accountId);
Copy link
Member

Choose a reason for hiding this comment

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

여기도 쿼리를 1개만 날리는데 트랜잭션을 걸어주지 않아도 될 것 같습니다

}

@Transactional
public void removeStock(Long itemId, int quantity) {
Copy link
Member

Choose a reason for hiding this comment

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

값을 감소시켜주는거니 decrementStock이란 이름이 좋아보이네요~

* 상품을 주문 or 취소 시 stock 감소 or 증가 로직
*/
@Transactional
public void addStock(Long itemId, int quantity) {
Copy link
Member

Choose a reason for hiding this comment

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

값을 더해주는 개념이므로 incrementStock이 좋아보입니다

jjeda added 15 commits October 16, 2019 00:31
- AccountAdapter가 AccountDto가 아닌 Account를 주입 받도록 재수정
- Redis SerializtionException 으로 인한 토큰스토어 임시변경
- 주문생성 테스트코드 수정
- 상품관련 테스트
[#30] 주문상품에 상품담기
- 정상적인 주문 생성 단위테스트
- 주문관련 도메인 Getter 에 @JsonIgnore 처리
- 기존 단위테스트 수정
- 서비스 레이어 단위테스트 보완
- 정상적으로 주문생성하는 테스트 수정
- AccountController -> AccountService 에서 Entity 가아닌 Dto를 받게 수정
- AccountService 에서 Entity가 아닌 Dto를 리턴하도록 수정
- AccountController에서 Entity 를 받아오는 것이 아닌 Dto를받아오게 수정
- Account <-> AccountDto Circular Dependency 문제해결을 위한 AccountAndDtoAdapter 적용
- Session에 저장할때 Account가 아닌 AccountDto를 저장하도록 수정
- 현재사용자를 받아올때 account를 받아오는것이아닌 AccountDto를 받아오도록 수정
- 테스트코드 수정
- DtoToEntity 메서드 -> dtoToEntity
- 메서드 중복 로직 제거
[#22] 주문관련 비즈니스 로직
@@ -0,0 +1,20 @@
package me.jjeda.mall.exception;

public class NotEnoughStockException extends RuntimeException {
Copy link
Member

Choose a reason for hiding this comment

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

IllegalStateException을 상속받는 것도 좋을 것 같습니다

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

Successfully merging this pull request may close these issues.

2 participants