Home > Custom Development, Extreme Programming, General Software Development, Refactoring > The Dangers of Single Responsibility in Programming

The Dangers of Single Responsibility in Programming

The Danger of Single Responsibility in Programming

October 16th 2009 | David Cooksey

The Dangers of Single Responsibility in Programming

The principle of single responsibility is an important and well-known object-oriented guideline. Systems designed without any consideration for this principle often result in the God-object anti-pattern, with predictably unfortunate results. However, taking single responsibility too far also results in code that is difficult to read and maintain.

Let’s take a look at a possible system built for a company that sells a wide variety of products. This system was originally built around a Product object, and over the years the product object has continued to grow and acquire responsibility as new features were added to the system. The IProduct interface now looks like this:

  public interface IProduct
  {
    int Id { get; set; }
    string Name { get; set; }
    int TypeId { get; set; }
    DateTime AddedDate { get; set; }
    decimal Cost { get; set; }
    decimal BasePrice { get; set; }
    decimal SalesPrice();
    decimal Price(int customerId);
    decimal Discount(int customerId);
    decimal GetShippingCharge(int customerId, string stateCode);
    int GetMinQuantity(int customerId);
    DateTime GetNumAvailable(int customerId);
  }

This company has built its business model on special services for frequent customers, including special discounts, shipping rates, lower base prices, etc. Some customers receive lower prices on specific products in return for promises to order at least a certain quantity each time. The net result is that the full cost depends on who and where the customer is as much as it depends on the product itself.

Now imagine that a developer on this project has read about the fascinating new concept of single responsibility and decides that IProduct is responsible for too much. In fact, it’s responsible for everything. So he creates a PriceProvider that contains a GetPrice method as shown below, moving the logic of the method directly from the Product class to the PriceProvider.

    public decimal GetPrice(IProduct product, int customerId)
    {
      decimal price = product.BasePrice;
      ICustomer customer = GetCustomer(customerId);
      if (customer.GoldLevelCustomer)
      {
        price = price * (1 - GetGoldLevelDiscount());
      }
      if (ProductIsOnSale() && !FixedDiscountAgreementExists(customer, product))
      {
        decimal salePrice = product.SalesPrice();
        if (salePrice < price)
        {
          price = salePrice;
        }
      }

      return price;
    }

So far, so good. The logic is adequately complex and involves enough dependencies that it should probably exist in its own class. Initially, our developer is happy. However, as he continues to look at this method, he decides that it is doing a lot more than it should. After all, any number of business logic changes could make this class change, and a class should have only one reason to change, right? So he rolls up his sleeves and gets to work, eventually producing the following:

    public decimal GetPrice(IProduct product, int customerId)
    {
      decimal price = product.BasePrice;
      ICustomer customer = GetCustomer(customerId);
      if (goldLevelDeterminator.IsGoldLevelCustomer(customer))
      {
        price = price * (1 - goldLevelDiscountProvider(product));
      }
      if (saleProvider.IsOnSale(product) && !fixedDiscountAgreementProvider.AgreementExists(customer, product))
      {
        decimal salePrice = product.SalesPrice();
        if (useSalesPriceInsteadOfPriceDeterminator.UseSalesPrice(price, salePrice))
        {
          price = salePrice;
        }
      }
      return price;
    }

The goldLevelDiscountProvider, saleProvider, and fixedDiscountAgreementProvider probably refer to their own tables, given the code structure shown, so it makes sense to split them out. However, the goldLevelDeterminator is literally calling the GoldLevelCustomer property on customer and returning it, and the useSalesPriceInsteadOfPriceDeterminator is simply comparing the sales price to the price.

These latter two changes are examples of implementing the principle of single responsibility at a level of granularity below that of the business requirements. It is possible that the company’s needs will change in such a way that these classes will become necessary, but they do not need their own class unless and until their complexity warrants it. The creation of two determinator classes here implies that significant logic is involved in determining whether a customer is a gold level customer, or whether the sales price should be used.

Unnecessary classes like the two mentioned above cause a number of problems. Firstly, they increase the complexity of the code. A developer reading through this class for the first time will need to open both determinators up and mentally piece their contents back into the main function instead of simply reading them. Secondly, their existence as independent entities implies that they are reusable. However, their creation was solely based on a desire to separate method calls into their own class, not a thorough investigation of how the class meshes with the rest of the classes in the project.. Quite often, classes like these are not reused and in fact their functionality is duplicated in other tiny classes used in other places.

In short, when you’re designing or refactoring systems, plan your class structure around business needs, not logical decision points. A method with two if statements should not automatically be considered as having two reasons to change. After all, those two if statements may represent a single business concept.

David Cooksey is a Senior .NET Consultant at Thycotic Software, an agile software services and product development company based in Washington DC. Secret Server is our flagship password management software product.

  1. November 10, 2009 at 9:14 pm

    After reading your article, I think your hypothetical developer just got SRP wrong. I’ve revisited your example with a better breakdown of functionality on my blog (see above) – would be interested in your reaction to a different refactoring.

  1. November 12, 2009 at 10:48 am
  2. November 20, 2009 at 1:58 pm

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: