In response to "The Dangers of Single Responsibility in Programming"

David Cooksey writes an interesting article titled "The Dangers of Single Responsibility in Programming" in which he proposes that there is a certain level of granularity below which SRP is not appropriate.  Although I understand where he's coming from, I tend to disagree with his conclusion, and I think it's because his hypothetical programmer didn't actually find the right responsibilities, not because the method was already granular enough.  I'd propose a slightly different breakdown of the responsibilities, leveraging Inversion of Control and creating several Pricing Calculators that each handle a different kind of discount (In his scenario, there are Sales prices and Gold Level Discounts).  I would see each of these items as their own calculator, and a separate "Pricer" class that would use each of these.  Note that in a real application I would probably leverage something like StructureMap to find all classes that implement IPriceCalculator and have some way to control the orderring of these (probably a Priority field on the IPriceCalculator interface to sort by) but to keep things simple I'm hard-coding the calculators list in this example.  So, something like this:

   public interface IProduct

    {

        decimal BasePrice { get; }

        decimal SalesPrice { get; }

        decimal GoldLevelDiscount { get; }

        bool IsOnSale { get; }

    }

 

    public interface ICustomer

    {

        bool HasFixedDiscountAgreement { get; }

        decimal FixedDiscount { get; }

        bool IsGoldLevelCustomer { get; }

    }

 

    public class Pricer

    {

        private List<IPriceCalculator> _calculators = new List<IPriceCalculator>

          {

              new GoldLevelPriceCalculator(),

              new SalePriceCalculator()

          };

        public decimal GetPrice(IProduct product, ICustomer customer)

        {

            decimal price = product.BasePrice;

            foreach (IPriceCalculator calculator in _calculators)

            {

                price = calculator.CalculatePrice(price, customer, product);

            }

            return price;

        }

    }

 

    public interface IPriceCalculator

    {

        decimal CalculatePrice(decimal price, ICustomer customer, IProduct product);

    }

 

    public class GoldLevelPriceCalculator: IPriceCalculator

    {

        public decimal CalculatePrice(decimal price, ICustomer customer, IProduct product)

        {

            if (customer.IsGoldLevelCustomer)

            {

                price = price*(1 - product.GoldLevelDiscount);

            }

            return price;

        }

    }

 

    public class SalePriceCalculator: IPriceCalculator

    {

        public decimal CalculatePrice(decimal price, ICustomer customer, IProduct product)

        {

            if (product.IsOnSale && !customer.HasFixedDiscountAgreement)

            {

                price = product.SalesPrice < price ? product.SalesPrice : price;

            }

            return price;

        }

    }

Notice that now, each type of discount is contained in its own class which takes the customer, product, and current price and applies its business rules to the price.  In this way, the GetPrice method isn't going to turn into a Monster Method as new business rules are added. Also notice that it's easy to add a new pricing calculator.   So, for grins, let's add the "FixedDiscount" version alluded to in David's example.  First, create a new price calculator:

public class FixedDiscountCalculator: IPriceCalculator

    {

        public decimal CalculatePrice(decimal price, ICustomer customer, IProduct product)

        {

            if (customer.HasFixedDiscountAgreement)

            {

                price = price*(1 - customer.FixedDiscount);

            }

            return price;

        }

    }

Now, we simply add this to the collection of price calculator used by our pricer (again, this would be handled by our DI container of choice in the real world):

private List<IPriceCalculator> _calculators = new List<IPriceCalculator>

                                                          {

                                                              new GoldLevelPriceCalculator(),

                                                              new SalePriceCalculator(),

                                                              new FixedDiscountCalculator()

                                                          };

And we're done.  Now, we've got a well-factored system which follows SRP and is easily extendable with new pricing rules (which, I would assume, could become significantly more complex than the examples given).

Well, we're not really done - if the theoretical developer had actually modified this original functionality in a Test-driven manner, you'd have a large number of unit tests to prove that you didn't break anything when you added the new FixedDiscountCalculator.  Note that I'm a fan of RhinoMocks, and leveraged it so I don't actually care about where my customer or product information comes from (there's actually no implementation of these interfaces at all - they should be loaded using your favorite repository-pattern-ORM-leveraging-code).  For the sake of keeping this post short, I've uploaded the complete source with unit tests as PricingCalculator.txt - rename to .cs, add NUnit and RhinoMocks references, and you're good to go.

In conclusion, Is SRP dangerous?  IMHO, not when properly applied.  But that's true for most SOLID principles - you need to know how to apply them appropriately.

 

8 Comments

  • Great post!! I've been trying to get my head around real work implementations of the SOLID principles.
    To make the code adhere to the open closed principle you could extend the Pricer class to take in a list of calculators, therefore none of your core code classes would have to change.

    private readonly List _calculators;
    public Pricer(List calculators)
    {
    _calculators = calculators;
    }

    Thanks


  • sifarrell: Yea - as I said in the original post, I would probably use some DI/IOC container (StructureMap is my personal favoriate right now) to load the calculators - which would handle the open/closed principle issue, but lacking some sort of DI container I would do something like you suggest and pass in the calculators as a constructor parameter.

  • I am a little unsure of this function:

    public decimal GetPrice(IProduct product, ICustomer customer)

    {
    decimal price = product.BasePrice;
    foreach (IPriceCalculator calculator in _calculators)

    {
    //Right here needs to find the correct //calculator, right?
    price = calculator.CalculatePrice(price, customer, product);

    }

    return price;

    }

    I think you want to use your IoC container to get the correct calculator. This code will just overwrite price and return whatever the last calculator added calculates. A bit confusing for those unfamiliar with DI.

    Thanks

  • Phil - Actually, if you look at the initial problem David put together, there are several types of pricing rules that can apply. Some of them override prices, and some of them modify the previous price. Therefore, the way I implemented it was to pass the "current" price in the loop to each calculator, and let them figure out how to update/modify the price. More specifically, the "Gold Member" price is calculated based on the base price, but if the item is on sale and the sales price is lower than the currently calculated price, the sales price is returned. In this way, you allow all pricing calculators a chance at processing the price. Perhaps there should be some sort of out param that would indicate that price calculations should stop after a certain calculator runs (allowing it to override all other calculator values) but that didn't seem to be part of the business requirements at the time. Does this help you understand it a bit better?

  • Nice explanation...

  • Doug,

    You make a number of good points, but given the number of arguments I've been in regarding the SRP, I feel that 'got it wrong' is insufficiently descriptive. I don't believe that we are in disagreement about SRP's application, but I do feel that the frequency with which the SRP is misunderstood is a good indication of how dangerous it is. I have a more detailed response posted.

  • When I see things like ICustomer, I can't help but think you messed up. The chances that you need to expand on the Customer class is high. This means breaking the ICustomer interface, which is bad, or creating ICustomer2, ICustomer3, etc, which is worse.

  • Started working on a system where SRP was applied almost vigorously, and everything is tested. Only problem is in readability. The source code for a 2 form application and 5 functions has more than 50 source code files, and supporting code to make everything stick together. It is next to impossible to read and almost impossible to stick anything together mentally. Even the original creator took 5 hours to implement a "close" button.

    It is difficult to get the balance right, so you don't over create a big application.

    I agree with this post though, wondered how to change that function and how to test it, and it makes sense, but it also makes sense for it to be all in one source code file, then you can actually read and follow it.

Comments have been disabled for this content.