Implement Order Domain Objects - Cart, CartItem, CartItemOption (#1) #2

seongbin9786 · 2022-01-30T07:02:28Z

this closes #1

리뷰 요청 사항

잘못된 코드나 테스트에 대한 말씀 부탁드립니다.
C#이 거의 처음이라 코드 개선에 대한 말씀 부탁드립니다.

기타 구현 결정 사항

장바구니(Cart)가 서버에 저장되는 이유는 회원의 멀티 디바이스 지원 때문입니다(ex: PC, Mobile).

src/AUSG.Eats.Order.Domain/Cart.cs

roeniss · 2022-01-31T21:15:49Z

src/AUSG.Eats.Order.Domain/Cart.cs

+        // Non-readonly property referenced in 'GetHashCode()' ??
+        return this.UserId.GetHashCode();
+    }
+}


소소한 생활상식: 리눅스 표준에서는 파일 마지막에 빈 한줄을 넣는 것이 컨벤션입니다. (SO) 깃헙에서 이렇게 '특별한 표시'를 해주는 것도 같은 맥락일 것 같네요.

(이거 말하는거임)

제가 vscode 쓸 때는 자동적으로 넣어줬는데 Rider를 처음 써봐서 안 들어간 거 같습니다. 수정하겠습니다 =)

일단 저장할 때마다 format하는 설정을 찾았습니다. (Jetbrains Rider 기준)

Rider > Settings > Actions On Save > Reformat and Cleanup Code

Rider 기본 설정으로는 파일의 끝에 빈 라인을 안 넣어줘서 아래 설정을 넣으면 format 시 빈 라인을 강제해줄 수 있습니다.

해당 내용은 wiki에도 따로 정리했습니다.

그런 설정이 있음? 그냥 IDE 우측 아래에서 누르면 되는거 아니예요?

저기 LF 라고 써있는 부분이 CRLF 일텐뎅... 라이다는 다른가 ㄷ

제가 윈도우(에서 개발을 안 해서 설정이 안 되어 있어욥...) + Rider를 거의 처음 써봤는데 WSL에서 풀 받아서 vim에서 :set fileformat으로 확인하면 unix로 뜨는 걸로 봐서 LF가 remote repo 기준인 것 같습니다. (저는 CRLF가 뜨는데 git 혹은 윈도우라 알아서 해주는 건진 모르겠습니다.)

여러분 그냥 .editorconfig 쓰십셔,,

enif-lee

고생하셨습니다.

enif-lee · 2022-02-01T00:29:39Z

src/AUSG.Eats.Order.Domain/Cart.cs

+    public long UserId { get; set; }
+
+    // IList에는 AsReadOnly();가 없다. (why?)
+    private readonly List<CartItem> _items = new List<CartItem>();


Mutable list structure가 필요한 경우 되도록이면 ICollection과 같은 추상에 의존하는게 좋습니다. 물론 private field라서 노출되지 않아 구체적인 대상을 지정해도 나쁘지는 않긴 하지만 그래도 추상타입에 의존하는게 조금더 좋아보여요. (구현체를 변경하거나 등등의 이점이 있음.

아마 AsReadOnly() 메소드가 지원되지 않았어서 List라는 구체 클래스로 선언했던 것 같습니다. List를 공개하지 않는다면 해당 컬렉션도 ICollection 혹은 IEnumerable로 변경하겠습니다.

AsReadOnly()가 없어도 new ReadonlyList(items) 형태로 이미 충분히 대체는 가능할 것 같습니닷

src/AUSG.Eats.Order.Domain/Cart.cs

enif-lee · 2022-02-01T00:34:25Z

src/AUSG.Eats.Order.Domain/Cart.cs

+
+    public override bool Equals(object? obj)
+    {
+        if ((obj == null) || !(this.GetType() == obj.GetType()))


if (object == null) return false; if (object is not Cart other) return false; return UserId == other.Id

이런 코드도 괜찮을까요?

public long? Id { get; } public override bool Equals(object? obj) { // use null propagation if (obj is not EqualsImplEx other) return false; return Id == other.Id; }

사실
return object is EqualsImplEx other && Id == other.id 로 해도 되겠네요.

enif-lee · 2022-02-01T00:37:52Z

src/AUSG.Eats.Order.Domain/CartItem.cs

+
+public class CartItem
+{
+    public long Id { get; set; }


요거는 불변이 아니라서 문제가 될 것 같네요. 외부에서 Cart에 입력 후 변경이 가능할 법합니다.

public record CartItem(long Id, ICollection Options, int Quantity)

정도면 깔끔하지 않을까 싶니다.

근대 여기서 Id는 어떤걸 의미하나요?

CartItem은 상태가 변경될 수 있는 객체로 Entity로 정의되어서 식별자가 필요해서 정의했습니다.

Auto_Increment로 Id를 할당하는 경우, CartItem에 Id setter가 없을 때 EF에서 지정할 수 있는지 궁금합니다 =)

EF를 고려하지 않는다가 제일 중요한 것 같네요. 사실 long type보단 id로는 string과 같은 난수 기반의 생성시점에 즉시 생성할 수 있는 타입을 쓰는게 좋을 것 같습니다.

EF를 완전히 고려하지 않으려면 도메인 모델과 영속화 모델을 나눌 때만 가능하지 않을까요?
(ex) JPA 사용시 빈 생성자 필요

src/AUSG.Eats.Order.Domain/CartItemOption.cs

src/AUSG.Eats.Order.Test/OrderTests.cs

enif-lee · 2022-02-01T00:47:33Z

src/AUSG.Eats.Order.Test/OrderTests.cs

+        const int newQuantity = 2;
+        oldItem.Options = newOptions;
+        oldItem.Quantity = newQuantity;
+        cart.AlterCartItem(oldItem);


이미 위에서 변경한 뒤에 삭제후 다시 추가하는 테스트는 이 줄을 삭제하셔도 테스트에 통과할 것 같아요. 같은 인스턴스를 쓸 것 같은데여?

맞습니다. 의도를 명확히 하려면 새 객체를 사용하는 게 좋을 것 같습니다. 다른 의도가 있어서 oldItem을 재활용한 것은 아니고 새 객체를 생성하고 동일 Id를 할당하는 것이 장황해서 이렇게 짰습니다.

MakeNewCartItem 에서 id 값을 파라미터로 받으면 안되나요? 자주 그런 용도로 쓰이는 것 같아서요. @seongbin9786

그런데 그보다 저는 "지금 테스트의 주축이 되는 기반 객체 (base object)의 '일부분'이 factory method 로 extract 된게 조금 불편하다"는 입장이예요. MakeCart면 안되는 이유가 무엇인지? 혹은 given 절에서 일일이 만들면 안되는 이유가 무엇인지? (물론 좀 verbose 하겠네요 ㅋㅋ)

MakeNewCartItem이 Id를 받지 않는 이유는 제가 귀찮아서 정의하지 않았습니다. 저도 필요성에 동의합니다...

MakeCart가 아닌 이유는 Cart만 테스트하는 게 아니어서입니다. 다만 이 부분은 지금 이 Issue/PR의 전체적인 문제점인데요, 영속화될 상태를 고려하지 않고 도메인 요구사항으로만 출발했다면 Cart를 생성하는 팩토리 함수 위주로 사용될 것 같고, Cart 애그리거트의 내부 상태를 자세히 테스트하는 경우는 잘 없었을 것 같습니다.

영속화는 고려하지 않는다 가 좋을 것 같아요. 이게 맞는 접근인지 모르겠는데, 스프링 식으로 따지면 "JPA는 (이미 검증된 라이브러리라서) 테스트하지 않는다" 따라서 "Persistence 레이어는 테스트할 필요가 없다" 전개라고 할 수 있고요.

지금 중요한 것은 (@enif-lee 검수 부탁드립니당) 도메인의 state, action 이기 때문에 그 외의 것들에 관심을 주지 않아도 좋지 않을까용? @seongbin9786

스프링 식으로 따지면 "JPA는 (이미 검증된 라이브러리라서) 테스트하지 않는다" 따라서 "Persistence 레이어는 테스트할 필요가 없다" 전개라고 할 수 있고요.

음.. 🤔 같은 접근은 아닌 것 같아요. 신뢰 가능하기 떄문에 테스트하지 거랑 애초에 고려해서는 안되는 거랑 다른 것 같습니다. 저는 조금 더 Domain을 엄격하게 생각하고 있어요. 결과적으로 이후의 의견에는 백번 동의합니다.

JPA를 신뢰하지 않으면 사용하지 않았을 것이라는 전제도 제 말에 숨어있네요 ㅋㅋ; ㅇㅋㅇㅋ

enif-lee

잘못눌렀네요 ㅋㅋ

seongbin9786 · 2022-02-01T12:32:11Z

따로 답글을 달지 않은 개선점들은 모두 동의하는 부분이고 개선된 코드를 이 PR 혹은 새 PR에 반영하겠습니다~~ =) 의견 주셔서 너무 감사합니당 ㅎㅅㅎ

…o be nullable.

…ecord struct.

seongbin9786 · 2022-02-05T03:12:49Z

잘못된 코드나 개선해야 되는 코드는 반영했습니다. 도메인 요구사항으로 접근하지 않은 TC를 제거하는 부분은 다른 Issue/PR에서 처리해보겠습니다.

roeniss · 2022-02-05T05:13:50Z

룰루랄라 남의 코드가 제일 재밌구먼

seongbin9786 · 2022-02-06T10:13:01Z

원본 이슈 (#1)에 테스트 명세의 변경 사항을 추가하고 이에 따라 코드도 변경하였습니다.

테스트 코드에도 변경이 있는데요, 아래와 같이 변경되었습니다.

구현 상세를 테스트하는 불필요한 TC 제거
- 당장 필요하지 않은 CartItem의 내부 상태에 대한 테스트가 제거되었고, 이에 따라 CartItem의 내부 구현도 제거하였습니다. 현재는 Id 필드만 존재합니다.
Id를 직접 구해와 비교하는 TC를 직접 객체의 Equals 호출을 활용하도록 변경.
- 따라서 현재의 구현에서는 Id를 노출하지 않습니다.
식별의 기준을 Equals와 HashCode 모두 확인하는 것으로 통일

enif-lee · 2022-02-07T04:51:45Z

src/AUSG.Eats.Order.Domain/Cart.cs

+        _items.Remove(itemToRemove);
+    }
+
+    public void AlterCartItem(CartItem itemToChange)


public UpdateCartItem(CartItem.Id id, Action<CartItem> action) { action(_items.First(item => item.Id == id) }

사실 요것도 좀 걸리긴하지만 이전보단 낫지 않을까요?

도메인적인 요구사항을 외부로 노출시키는 용도라면 UpdateQuantity(Id id, int num)이 더 나아보이긴 해요.

AlterCartItem의 구현은 best practice를 참고했거나 이론적 근거가 있지는 않았습니다.

올려주신 코드처럼 CartItem => void도 상태 갱신의 유연성 측면에선 가장 좋을 것 같습니다.

지금처럼 특정 상태 갱신에 대한 애그리거트 수준의 불변식 요구사항이 없을 때는 가장 좋은 선택일 것 같습니다.

한 가지 의견이 있다면

CartItem#Id보다 CartItem만 참조해도 비교가 가능해서 엔터티로 받아도 충분할 것 같은데 그렇게 해도 될까요?

아래는 말씀하신 UpdateQuantity를 보고 든 생각인데요, Cart 애그리거트 차원에서 불변식을 만족시켜야 할 때는 Update~~~가 유의미할 것 같습니다.

다만 현재의 도메인 분석/요구 사항에는 해당 내부 엔터티(CartItem)의 상태를 넘어서는 불변식이 아직 없어서, 지금처럼 상태를 바꾸는 정도로 충분하지 않을까요? (ex: 아이템 수량은 모든 장바구니 아이템들을 통틀어 최대 10개까지만 담기 가능)

애그리거트 차원에서 불변식 강제가 필요 없는 상황에서 내부 엔터티의 상태 변경에 대응되는 상태 갱신 메소드를 만든다면 애그리거트 <=> 내부 엔터티간 결합은 커지는데 이익은 모호한 것 같습니다.

그래서 제 생각은 (위에서처럼) 도메인 요구사항이 딱히 없는 경우 상태 갱신은 replace 혹은 콜백 형태로 구현하되, 일부 불변식 조건이 필요한 경우 애그리거트 차원에서 불변식 강제를 위해 애그리거트에서 public API를 제공하는 게 좋을 것 같습니다.

(그리고 지금은 CartItem 엔터티의 내부 필드가 없어서 지금 updateQuantity를 구현할 순 없을 것 같습니다...)

만약 콜백 함수 방식으로 처리한다면, 상태 갱신을 하는 메소드를 노출해야 하는지 궁금합니다.

enif-lee · 2022-02-07T04:52:37Z

src/AUSG.Eats.Order.Domain/CartItem.cs

+
+public class CartItem
+{
+    private long? _id;


Id VO 따로 만들어주시면 좋을 것 같습니다.

Id VO가 어떤 것을 말씀하신 건지 잘 모르겠습니다 =(

enif-lee · 2022-02-07T04:55:55Z

src/AUSG.Eats.Order.Test/OrderTests.cs

+        Assert.Equal(alteredItem, oldItem);
+    }
+
+    [Fact(DisplayName = "Cart 객체는 CartItem을 변경할 때 해당 Item이 이미 List에 없다면 예외를 발생시킨다.")]


"Cart에 담기지 않은 CartItem을 수정하려고 할 수 없다." 요로케 하면 좋지 않을까요?

네. 예외는 기술적인 명세다 보니 말씀하신 이름이 도메인적으로 더 좋은 네이밍일 것 같습니다.

Implement Order Domain Objects - Cart, CartItem, CartItemOption (#1)

5407cc4

seongbin9786 requested review from enif-lee and p4rksh January 30, 2022 07:02

roeniss reviewed Jan 31, 2022

View reviewed changes

src/AUSG.Eats.Order.Domain/Cart.cs Outdated Show resolved Hide resolved

roeniss reviewed Jan 31, 2022

View reviewed changes

enif-lee approved these changes Feb 1, 2022

View reviewed changes

enif-lee requested changes Feb 1, 2022

View reviewed changes

seongbin9786 added 6 commits February 5, 2022 11:38

test: Added test checking default values of int and nullable int

78540dd

refactor: Removed redundant usage of 'this' and changed Id property t…

c9702b0

…o be nullable.

refactor: Changed implementation of a value object from hand ame to r…

254f920

…ecord struct.

refactor: Implemented Equals method as suggested (@enif-lee).

6d4ed0f

style: Reformatted codes related to this PR using Rider.

7eedfae

refactor: Added few Factory methods for CartItem

dd04be7

refactor: Removed tests asserting implementation details.

85b7261

enif-lee requested changes Feb 7, 2022

View reviewed changes

enif-lee approved these changes Feb 22, 2022

View reviewed changes

Implement Order Domain Objects - Cart, CartItem, CartItemOption (#1) #2

Are you sure you want to change the base?

Implement Order Domain Objects - Cart, CartItem, CartItemOption (#1) #2

Conversation

seongbin9786 commented Jan 30, 2022 • edited Loading

리뷰 요청 사항

기타 구현 결정 사항

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongbin9786 Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongbin9786 Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enif-lee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enif-lee left a comment

Choose a reason for hiding this comment

seongbin9786 commented Feb 1, 2022

seongbin9786 commented Feb 5, 2022

roeniss commented Feb 5, 2022

seongbin9786 commented Feb 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seongbin9786 commented Jan 30, 2022 •

edited

Loading

seongbin9786 Feb 5, 2022 •

edited

Loading

seongbin9786 Feb 5, 2022 •

edited

Loading

seongbin9786 commented Feb 6, 2022 •

edited

Loading