samples icon indicating copy to clipboard operation
samples copied to clipboard

Change local storage for priced items

Open natalie-o-perret opened this issue 3 years ago • 2 comments

I was porting your C# code to F# when I've realized that something didn't add up and most notably how merging quantities is achieved in the current implementation:

https://github.com/EventStore/samples/blob/a552bc5d564cd4121807bc0fba958f5e01e4627e/CQRS_Flow/.NET/Carts/Carts/Carts/Cart.cs#L92-L110

https://github.com/EventStore/samples/blob/a552bc5d564cd4121807bc0fba958f5e01e4627e/CQRS_Flow/.NET/Carts/Carts/Carts/Products/PricedProductItem.cs#L46-L52

https://github.com/EventStore/samples/blob/a552bc5d564cd4121807bc0fba958f5e01e4627e/CQRS_Flow/.NET/Carts/Carts/Carts/Products/ProductItem.cs#L30-L36


Not only there is a redundant check as part of the current implementation:

https://github.com/EventStore/samples/blob/a552bc5d564cd4121807bc0fba958f5e01e4627e/CQRS_Flow/.NET/Carts/Carts/Carts/Products/PricedProductItem.cs#L41-L44

https://github.com/EventStore/samples/blob/a552bc5d564cd4121807bc0fba958f5e01e4627e/CQRS_Flow/.NET/Carts/Carts/Carts/Products/ProductItem.cs#L46-L49


but actually considering how data are stored:

https://github.com/EventStore/samples/blob/a552bc5d564cd4121807bc0fba958f5e01e4627e/CQRS_Flow/.NET/Carts/Carts/Carts/Cart.cs#L21

I think it would be relevant to use a map or a dictionary to store the priced items.

It would make the whole consistency check a lot simpler.

Also why bother throwing exceptions when you can add a product when the same product isn't already part of the collection and then merge quantities when it is.

Wdyt?

natalie-o-perret avatar Jul 31 '21 17:07 natalie-o-perret

@natalie-perret-1986, I think that the suggestion makes sense. Would you be willing to send a PR aligning that? 🙂

oskardudycz avatar Aug 02 '21 13:08 oskardudycz

@natalie-perret-1986, I think that the suggestion makes sense. Would you be willing to send a PR aligning that? 🙂

Sure I will

natalie-o-perret avatar Aug 02 '21 14:08 natalie-o-perret