Home > Custom Development, Extreme Programming > The Danger of Single Responsibility in Programming Continued

The Danger of Single Responsibility in Programming Continued

The Danger of Single Responsibility in Programming Cont.

October 16th 2009 | David Cooksey

The Dangers of Single Responsibility in Programming Continued

The Dangers of Single Responsibility, Cont.

Doug Rohrer responded to my initial post on this topic with a good refactoring of the classes involved in a manner similar to the Strategy pattern. I agree with many of his points—the hypothetical developer certainly chose the wrong responsibilities; misunderstood the Single Responsibility Principle; and generally made the code a mess. That said, I believe that SRP is most definitely dangerous, not because of what happens when it is used correctly, but because of how easy it is to get it wrong. Misapplying the SRP can result in code that makes God objects look easy to maintain.

For clarity’s sake, I’ll go one step further—it is easy to misunderstand the sentence “A class should have only one reason to change” as a literal commandment to be applied at the line or method level. This results in disaster. One common example of how the SRP is misunderstood can be seen in this thread, where the poster asks if the SRP means that each class can have only one method. Luckily the poster received a good informative answer, but that is not the case for all developers learning about the SRP.

Here is an example of code modifications I have seen motivated by a desire to apply the SRP.

BEFORE

public class Check
    {
        private readonly IDataProvider _dataProvider;

        public Check(IDataProvider dataProvider)
        {
            _dataProvider = dataProvider;
        }

        public bool Run()
        {
            IBusinessObject data = _dataProvider.Get();

            if (data.Condition1 && data.Condition2)
            {
                string message = string.Format("Check failed, {0} {1}", data.Property1, data.Property2);
                throw new Exception(message);
            }
            return data.Property3 != data.Property4;
        }
    }

AFTER

public class Class
    {
        private readonly IDataProvider _dataProvider;
        private readonly ICheckErrorMessageProvider _checkErrorMessageProvider;

        public Check(IDataProvider dataProvider, ICheckErrorMessageProvider checkErrorMessageProvider)
        {
            _dataProvider = dataProvider;
            _checkErrorMessageProvider = checkErrorMessageProvider;
        }

        public bool Run()
        {
            IBusinessObject data = _dataProvider.Get();

            if (data.Condition1 && data.Condition2)
            {
                string message = _checkErrorMessageProvider.GetErrorMessage(data);
                throw new Exception(message);
            }
            return data.Property3 != data.Property4;
        }
    }
public class CheckErrorMessageProvider : ICheckErrorMessageProvider
    {
        public string GetErrorMessage(IBusinessObject data)
        {
            return string.Format("Check failed, {0} {1}", data.Property1, data.Property2);
        }
    }

Here, the developer asked the SRP question “Does this class have only one reason to change?” and got the answer “No, it could change because the formatted text could change, or because the logic could change”, and refactored the String.format out into its own provider. While harmless on the surface, this artificial separation of concerns does not add any value. The new class is so specific that it cannot be used anywhere else. In addition, the developer is likely to forget the CheckErrorMessageProvider name almost immediately, so if a text change is required he will most likely go to the Check class first, and then go the extra level down into the string provider in order to make the text change. In other words, the complexity of the code was increased for no benefit.

I believe that after correctness, simplicity is the most important programming principle. Simpler code is easier to understand when first read; easier to remember; easier to test; easier to refactor; and easier to add features to. Anything that adds complexity makes all of these tasks harder, especially on larger projects with many non-trivial sub-systems. Applying single responsibility at the line or method level diffuses business logic into a cloud of tiny classes that do next-to-nothing individually, and thoroughly obscure the logic they represent.

In conclusion, yes, the SRP is not dangerous when applied correctly. But then, most things are dangerous because of what happens when they are misused, and the Single Responsibility Principle is no exception. Handle with care!

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 19, 2009 at 1:06 pm

    David, I totally agree, check out my post on SRP, too.

  2. Michael L
    November 20, 2009 at 11:54 am

    David, I almost agree with you. Any design concept can be applied either incorrectly or inappropriately. In the example that you’ve given, having a provider to create the message in that form is stretching SRP, IMO. OTOH, if the provider had to go to a message store (ex. database) to retrieve an appropriate message, or there was a need to engage a translation service (to name a couple), the partitioning of functionality that you’ve given is appropriate, IMO.

    There are many things that we deal with throughout life that are dangerous – knives, chainsaws, orbital space lasers for example. Experience gives us guidance.

  3. Oleg
    November 20, 2009 at 1:38 pm

    While I understand your position it seems kind of artificial to me.

    You are kind of replacing SRP intention to separate concerns by absouletely different question – “What is going to happen if I am not sure which are my concerns?”

    Well, bad will happen for sure! But SRP was not intended to be used when you are not sure if Error Message provision is a separate concern.

  4. November 30, 2009 at 10:03 am

    Michael, I agree that if the message was pulled from the database or if there were a good number of message formats to choose from that a separate provider would make sense.

    My problem with this implementation is that there is no such complexity in message format.

    Oleg, you are absolutely right that applying SRP when you are not sure what the responsibilities are is a bad idea. In this case, my dispute is with an overly literal interpretation of SRP that fragments code to no benefit. The code sample here is not artificial. It, and the ‘SRP’ justification of it, are real (I just changed the class names and functionality slightly, the provider wrapping a string.format is real). My intention is not to scare people away from the SRP, that would be silly. My intention is to warn people that a simple literal reading of SRP is not sufficient, critical thinking is (as always in programming) still required.

  1. November 20, 2009 at 2:02 pm
  2. November 20, 2009 at 10:28 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: