Consider the following example (very simplified):

public class Basket
{
    private readonly List<BasketItem> _items = new List<BasketItem>();
    public IReadOnlyCollection<BasketItem> Items => _items;

    // _items.Sum(item=>item.TotalAmount)
    public decimal TotalAmount { get; private set; } 
    // ...other properties ...

    public void ChangeItemQuantity(BasketItem item, decimal quantity)
    {
        if (item == null) throw new ArgumentNullException(nameof(item));
        ///////////////////////////////////
        // code duplication
        if (quantity <= 0) throw new ArgumentOutOfRangeException(nameof(quantity));
        ///////////////////////////////////

        item.ChangeQuantity(quantity);

        // recalculates basket totals, items.Sum(item=>item.TotalAmount)
        RecalculateTotals();  
    }
}

public class BasketItem
{
    public decimal Quantity { get; private set; }
    public decimal TotalAmount { get; private set; } // quantity * price
    // ...other properties ...

    public void ChangeQuantity(decimal quantity)
    {
        ///////////////////////////////////
        // code duplication
        if (quantity <= 0) throw new ArgumentOutOfRangeException(nameof(quantity));
        ///////////////////////////////////

        Quantity = quantity;

        RecalculateTotals(); // recalculates item total (price * quantity)
    }
}

As you can see I encapsulate things a lot (I'm really considering moving away from OOP).

The problem I see with this code is that I perform the same validation in 2 places. It's actually the same validation in 3 places because there's additional one at the boundaries (API/UI layer).

Could you tell me please, is the validation in Basket.ChangeItemQuantity redundant?

  • 1
    I'm curious about the using code. Why, if I have a reference to a BasketItem do I tell the Basket to change the quantity rather then just telling the BasketItem directly? How is the Basket helping at all here? – candied_orange Nov 16 at 11:29
  • 2
    If quantity must only be changed from Basket you are failing to encapsulate by letting others have access to BasketItem and BasketItem.ChangeQuantity() – candied_orange Nov 16 at 11:41
  • 2
    Similarly, you might want to just not store totals in the DB but calculate them when needed. Of course once you check out they need to be recorded but at that time quantity is fixed. Now were recording an event that took place at a time. Nothing needs to change so totals stop being volatile. – candied_orange Nov 16 at 12:27
  • 2
    Done this way you don't care who changed what when. You calculate totals when something asks for them. All you need is to be able to traverse whatever your totaling. – candied_orange Nov 16 at 12:29
  • 1
    Or do as you say, and use computed properties, and have some separate database made just for reports (reporting database,data warehouse). Thanks for the help, I must seriously think about some better design/solution to this problem. – Konrad Nov 16 at 12:41
up vote 5 down vote accepted

Could you tell me please, is the validation in Basket.ChangeItemQuantity redundant?

Yes it's redundant. But that's the least of your problems.

Semantically it's weird for an Item to know how many of it are in a basket. It's weird of it to know it's even in a basket. This reads like I could ask a potato to total up my order. Your items simply know to much.

Maybe I'm reading to much into the names and what you really want to know is if having multiple levels of validation of the same thing is always bad. It's not. Provided it's happening at the right places.

When you're crossing significant boundaries validation is a good idea. When you're taking in user data you're at a big one. When building persisting data structures is another. It's OK to put validation of the same thing on the doors through these walls. It maybe redundant but it helps me read your code. I know right away what your expectations are without having to dig around in other places to know what's allowed.

In fact, whenever talking to, or listening to anything that doesn't fully enforce your model on it's own, you should consider validating to impose your model on it.

Maintaining duplicate code is still annoying. Make peace with the fact that identical code, doing a different job, that could change for a different reason, has a perfectly good reason to exist.

That said, it may be you're dealing with a case of primitive obsession. You keep using decimal when what you seem to want is a positive quantity. You could stop spreading decimal and it's validation code around by sticking both of them in a Quantity class. Now all you have to check for is null. Sigh.

  • 2
    "Now all you have to check for is null. Sigh". For a few months more only. C# 8's nullable reference types will neatly address this issue. – David Arno Nov 16 at 11:00
  • "Semantically it's weird for an Item to know how many of it are in a basket." it might be, but as I said before the BasketItem is a separate table in the database. And quantity is a column. So my database is designed this way. I could store the quantity in some kind of Dictionary or something, inside my Basket class but then it wouldn't map to my database table and I would need a separate data models and business models and manage mappings between them. That's why I use the data model as a business model (2 in 1) and just map to my view model (API model). – Konrad Nov 16 at 11:01
  • Because Quantity is a column in BasketItems table in my RDBMS. To apply your suggestion I would have to add another model (layer) on top of my data model or don't use RDBMS at all, or use document db and store serialized basket directly (in form of some JSON or something). – Konrad Nov 16 at 11:08
  • 2
    You seem to be very focused on your DB. So much so that you've created a class named after a join table and expected people to somehow just know what you meant. I didn't. I doubt others will either. – candied_orange Nov 16 at 11:24
  • 1
    it's weird for Item to know how many of it are in a basket what if the object's functionality depends on knowing this? – coolpasta Nov 17 at 9:07

I would say that the redundant code is a bigger problem. Why does your Basket-Class has a method "ChangeItemQuantity" when the item you pass as an argument to it has this exact same method with the same business-logic? To be clear, a method name which occurs multiple times may not be a problem, but business-logic code which occurs multiple times is definitely one. Thats redundant code which has to be maintained. Another question is, where is your method "RecalculateTotals" located? Is it redundant, too?

But that was not the question, maybe your code all in all makes more sense than this excerpt.

Im terms of validation, to be pedantic you have to validate a specific value / input at every place where you can't guarantee that it has been validated before. In your example, the method "ChangeQuantity" of your BasketItem could be called directly from client-code without going through the method "ChangeQuantity" of your Basket. So you have to assume that the input parameters of the BasketItem-method are not validated yet.

If BasketItem would be a private class inside the public Basket class and the only execution-path to BasketItem.ChangeQuantity() gous through Basket.ChangeQuantity() where the data is validated, I would say it's not necessary to validate it again, because you can be sure that it was validated before. But evan in that case one could argue that maybe in a later refactoring, the private class may be moved outside and become public.

To be clear, thats a really pedantic view which can't be followed the whole time. In addition, all this validation-code is pretty bloating and makes the actual porpuse of the method more unreadable. All I want to say is, assumptions about other modules / classes from the current module / class are often a bad thing in object oriented software and should be made whisely, because you can never be sure that the assumption turns out wrong (now or maybe in the future).

  • "I would say that the redundant code is a bigger problem. Why does your Basket-Class has a method "ChangeItemQuantity" when the item you pass as an argument to it has this exact same method with the same business-logic?" - Because changing quantity of some item in the basket changes total amount in the basket, so it invalidates state of the basket – Konrad Nov 16 at 12:08
  • 1
    @Konrad That reasoning is correct, but this also means that a BasketItem should not have a publicly available ChangeQuantity() method – someone could call that directly and therefore invalidate the Basket state. – amon Nov 16 at 18:32

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.