EventStore / samples

Samples showing practical aspect of EventStoreDB, Event Sourcing
https://developers.eventstore.com/
MIT License
70 stars 24 forks source link

Change local storage for priced items #5

Open natalie-o-perret opened 2 years ago

natalie-o-perret commented 2 years ago

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?

oskardudycz commented 2 years ago

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

natalie-o-perret commented 2 years ago

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

Sure I will