When should a method depend on a data source and NOT have it declared as a parameter?

Question

I was assigned a code review to one of my colleagues. I posed the following, which I wanted to share here in order to hear whether I am right or wrong.

Consider the following code snippet:

public void DoSomethingWithList()
{
   var list = AnotherClass.GetList();
   var itemsToRemove = list.Where(...);
   list.Remove(itemsToRemove);
   .
   .
   .
}

I argue that the method above should have list declared as a dependency (in other words, declared as a parameter in the method's signature). Why ? To follow the Single Responsibility principle, and/or for testing purposes. (I think this is the case for an external class call, but even if GetList was a private method inside the same class, I would still claim the same - even if the sole motif would be code clarity (call it "purpose clarity").

My colleague says this is not so. Having to call this function every time always passing the same parameter is cumbersome.

I obviously understand that as a developer, but I insisted that the "software engineering principles aimed at code maintainability" should prevail over "developer laziness" (myself included !)

I do understand there are no bulletproof rules, but nevertheless that they should only be broken for a very good reason.

I wonder whether I am wrong. And if am I not, which other aspects I did not mention should have been.

Answer 1

Between being right and wrong, there is a third possibility, that you're both right.

You are correct at one level, and your colleague is correct that we should bundle things together so that we don't have to deal with two variables when one would do.

Sometimes when there is this kind of tension there is a missing abstraction or entity.

I'd suggest to do it your way with parameter — and also provide another entity for your colleagues to use in their parameterless way.  This other entity will bind, in construction, to what was otherwise the parameter (e.g. the list).  It is a new entity that represents the binding of the two items, so that callers have only one item to deal with instead of dealing with a pair of items that are conceptually (but only implicitly and informally) bundled.

Answer 2

This method is somewhat difficult to test because it doesn't take anything in nor does it return anything back. So, to test you would need to know something about the internals of the method, that is what is being modified internally.

Testing it involves calling that method and inspecting the results of the AnotherClasses.GetList() method. It also might involve some setup to initially create the list so you can see the results of DoSomethingWithList().

That's not ideal but doable.

Passing in the list makes the function more explicit.

DoSomethingWithList(List theList)

If I casually look at that I can assume the list may be modified in some way. Straight forward to write a test.

DoSomethingWithList() does not have that clarity.

As Erik suggests, try it both ways, and find out which is easier to read, understand, and test. At the end of the day, that it what matters.

Answer 3

Based on DDD pricinpal, I think the answer depends on is it a Domain Service method or an Application layer method.

1. If this is a Domain Service method, I think you are right. The original design violate the Single Responsibility principal. The main issue here is this code is not test friendly, the reponsiblity of retrieving the list leaked into your domain service logic. It makes the unit test impossible without introducing some kind of mocking framework (e.g. Mockito). And it makes the integration test even harder.

   Think about the case that your domain service actually works perfectly but the AnotherClass.GetList() has some bug won't return the right list. Then if the code failed, you are the first person got called to blame and you are the one forced to investigate the bug.

   But if we refactor the code to:

   public void DoSomethingWithList(List<T> theList) {
       if(!isValid(theList)) {
           throw new IllegalArgumentException("The list is not valid for processing");
       }
       // Normal logic below
       ... ...
   }
   

   We can obtain the benefits:

   • We can create comprehensive Test cases to cover as many scanarios as possbile.
   • We can create test cases to iterate illegal argument problems at development and compile time.
   • We can create bean validator to guard against the input argument at runtime without letting garbage input directly penerate into our domain logic.
   • We can use the specific runtime exception to let's know where the exact problem is. Is it an argument issue or is it the domain logic issue? A clear answer helps us to reduce the maintenance cost.

2. If it is application layer code, you may not have to enforce the design principals. But based the developer's argument, this logic is reused in different places, it is obviously not a good design to put it in application layer and the better option is to extract the domain logic from the application layer to make it a Domain Service which will make your whole application more testable, flexible and robust.

Answer 4

The problem with your friend's approach is that it is impossible to stub out the call to GetList() for unit testing, since it is a static method. You will be unable to test DoSomethingWithList without also testing GetList. This could be a serious problem for your automated tests if the backing store for GetList is not available on the test bed environment.

The easy path forward is to convert AnotherClass to an instanced class and inject it. You then register it with your IoC container as a singleton so everyone shares the same copy.

interface IAnotherClass 
{
    List<Foo> GetList();
}

class AnotherClass : IAnotherClass
{
    private IExpensiveDatabaseConnection _singleInstance; 

    public List<Foo> GetList()
    {
        return _singleInstance.GetList();
    }
}



class Bar
{
    protected readonly IAnotherClass _anotherClass;

    public Bar(IAnotherClass anotherClass)
    {
        _anotherClass = anotherClass;
    }

    public void DoSomethingWithList()
    {
        var list = _anotherClass.GetList();
        var itemsToRemove = list.Where(...);
        list.Remove(itemsToRemove);
    }
}

Of course, if you're going to do this, you may need to refactor other code to match it. If this is a legacy code base with a lot of static method calls, it might be more cost effective to continue using the old pattern.