Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change local storage for priced items #5

Open
natalie-o-perret opened this issue Jul 31, 2021 · 2 comments
Open

Change local storage for priced items #5

natalie-o-perret opened this issue Jul 31, 2021 · 2 comments
Labels
enhancement New feature or request

Comments

@natalie-o-perret
Copy link

natalie-o-perret commented Jul 31, 2021

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:

private void Apply(ProductAdded @event)
{
Version++;
var newProductItem = @event.ProductItem;
var existingProductItem = FindProductItemMatchingWith(newProductItem);
if (existingProductItem is null)
{
ProductItems.Add(newProductItem);
return;
}
ProductItems.Replace(
existingProductItem,
existingProductItem.MergeWith(newProductItem)
);
}

public PricedProductItem MergeWith(PricedProductItem pricedProductItem)
{
if (!MatchesProductAndPrice(pricedProductItem))
throw new ArgumentException("Product or price does not match.");
return new PricedProductItem(ProductItem.MergeWith(pricedProductItem.ProductItem), UnitPrice);
}

public ProductItem MergeWith(ProductItem productItem)
{
if (!MatchesProduct(productItem))
throw new ArgumentException("Product does not match.");
return Create(ProductId, Quantity + productItem.Quantity);
}


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

public bool MatchesProductAndPrice(PricedProductItem pricedProductItem)
{
return ProductId == pricedProductItem.ProductId && UnitPrice == pricedProductItem.UnitPrice;
}

public bool MatchesProduct(ProductItem productItem)
{
return ProductId == productItem.ProductId;
}


but actually considering how data are stored:

public IList<PricedProductItem> ProductItems { get; private set; } = default!;

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
Copy link
Contributor

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

@oskardudycz oskardudycz added the enhancement New feature or request label Aug 2, 2021
@natalie-o-perret
Copy link
Author

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

Sure I will

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants