Single Responsibility Principle: do all public methods in a class have to use all class dependencies?

Question

Say I have a class that looks like the following:

internal class SomeClass
{
    IDependency _someDependency;

    ...


    internal string SomeFunctionality_MakesUseofIDependency()
    {
    ...
    }
}

And then I want to add functionality that is related but makes use of a different dependency to achieve its purpose. Perhaps something like the following:

internal class SomeClass
{
    IDependency _someDependency;

    IDependency2 _someDependency2;

    ...


    internal string SomeFunctionality_MakesUseofIDependency()
    {
    ...
    }

    internal string OtherFunctionality_MakesUseOfIDependency2()
    {
    ...
    }
}

When I write unit tests for this new functionality (or update the unit tests that I have for the existing functionality), I find myself creating a new instance of SomeClass (the SUT) whilst passing in null for the dependency that I don't need for the particular bit of functionality that I'm looking to test.

This seems like a bad smell to me but the very reason why I find myself going down this path is because I found myself creating new classes for each piece of new functionality that I was introducing. This seemed like a bad thing as well and so I started attempting to group similar functionality together.

My question: should all dependencies of a class be consumed by all its functionality i.e. if different bits of functionality use different dependencies, it is a clue that these should probably live in separate classes?

Answer 1

I usually group methods into classes if they use a shared piece of state that can be encapsulated in the class. Having dependencies that aren't used by all methods in a class can be a code smell but not a very strong one. I usually only split up methods from classes when the class gets too big, the class has too many dependencies or the methods don't have shared state.

Answer 2

My question: should all dependencies of a class be consumed by all its functionality i.e. if different bits of functionality use different dependencies, it is a clue that these should probably live in separate classes?

It is a hint, indicating that your class may be a little incoherent ("doing more than just one thing"), but like you say, if you take this too far, you end up with a new class for every piece of new functionality. So you would want to introduce facade objects to pull them together again (it seems that a facade object is exactly the opposite of this particular design rule).

You have to find a good balance that works for you (and the rest of your team).

Answer 3

Looks like overloading to me. You're trying to do something and there's two ways to do it, one way or another. At the SomeClass level, I'd have one dependency to do the work, then have that single dependent class support the two (or more) ways to do the same thing, most likely with mutually exclusive input parameters. In other words, I'd have the same code you have for SomeClass, but define it as SomeWork instead, and not include any other unrelated code.

HTH

Answer 4

Does SomeClass maintain an internal state, or is it just "assembling" various pieces of functionality ? Can you rewrite it that way:

internal class SomeClass
{
    ...


    internal string SomeFunctionality(IDependency _someDependency)
    {
    ...
    }

    internal string OtherFunctionality(IDependency2 _someDependency2)
    {
    ...
    }
}

In this case, you may not break SRP if SomeFunctionality and OtherFunctionality are somehow (functionally) related which is not apparent using placeholders. And you have the added value of being able to select the dependency to use from the client, not at creation/DI time. Maybe some tests defining use cases for those methods would help clarifying the situation: If you can write a meaningful test case where both methods are called on same object, then you don't break SRP.

As for the Facade pattern, I have seen it too many times gone wild to like it, you know, when you end up with a 50+ methods class... Question is: Why do you need it ? For efficiency reasons à la old-timer EJB ?

Answer 5

A Facade is used when you want to hide complexity (like an interface to a legacy system) or you want to consolidate functionality while being backwards compatible from an interface perspective.

The key in your case is why you have the two different methods in the same class. Is the intent to have a class which groups together similar types of behavior even if it is implemented through unrelated code, as in aggregation. Or, are you attempting to support the same behavior but have alternative implementations depending on the specifics, which would be a hint for a inheritance/overloading type of solution.

The problem will be whether this class will continue to grow and in what direction. Two methods won't make a difference but if this repeats with more than 3, you will need to decide whether you want to declare it as a facade/adapter or that you need to create child classes for the variations.

Your suspicions are correct but the smell is just the wisp of smoke from a burning ember. You need to keep an eye on it in case it flares up and then you need to make a decision as how you want to quench the fire before it burns out of control.

Answer 6

When every instance method touches every instance variable then the class is maximally cohesive. When no instance method shares an instance variable with any other, the class is minimally cohesive. While it is true that we like cohesion to be high, it's also true that the 80-20 rule applies. Getting that last little increase in cohesion may require a mamoth effort.

In general if you have methods that don't use some variables, it is a smell. But a small odor is not sufficient to completely refactor the class. It's something to be concerned about, and to keep an eye on, but I don't recommend immediate action.