Clean code vs ease of testing: Make method params explicit when they are class attributes just so unit testing is easier?

Question

In Clean Code, we're advised to reduce, wherever possible, the number of arguments in a method's signature. This can often be achieved by using class attributes in lieu of parameters.

Now in a code review I pointed out this exact thing, because a particular method had a parameter but really this parameter was only used to pass always the same class attribute into it. So why not drop the parameter and reference the class attribute instead?

The author of the code argued that, by keeping the parameter explicit, it was easier to write unit tests for the method, because the test data could be directly passed in.

I feel like that's a bit of a code smell or design smell. I'd concede that it's easier to do testing if you can pass stuff directly to the method. But would that be so much harder than creating the object, then setting the internal fields to what you need them to be? (Or maybe there's even better techniques?)

I appreciate that there is some amount of subjectivity to this question, but there still should be an answer to the question whether or not it would be considered "best practice".

EDIT: Of course I mixed up terminology. I don't mean a class attribute. I mean a member attribute. The example would look like this

class C:
    def __init__(self, some_field):
        self.some_field = some_field

    def do_work(self):
        _helper_function(self.some_field)

    def _helper_function(self, param_field):
        #doing work using `param_field`

The method _helper_function isn't part of the public interface, and from within C only ever gets called with self.some_field. I suggested changing that to:

class C:
    def __init__(self, some_field):
        self.some_field = some_field

    def do_work(self):
        _helper_function()

    def _helper_function(self):
        #do work using self.some_field

Answer 1

because a particular method had a parameter but really this parameter was only used to pass always the same class attribute into it.

So the class looks like this?

 class C
 {
   private int _foo;

   public C(int foo) { _foo=foo;}

   public int AddFoo(int bar, int foo){return foo+bar;}

   public void DoSomeCalculation()
   {
      // ...
      int result1 = AddFoo(bar1,_foo);
      // ...
      int result2 = AddFoo(bar2,_foo);
      // ...
   }
}

Now it should be obvious if any call to AddFoo passes _foo as a second parameter, then any call to AddFoo is necessarily inside C. That means, AddFoo should be private, which leads to the question why your colleague wants to write unit tests for it in first place!

So either it is not possible to make the method private (because it should stay callable from outside for another reason than just testing, and with the option of passing something different than _foo), then your colleague is right. Or it can be made private, which means it should be - and your colleague should not try to test implementation details.

Answer 2

Change the object state when you want to change the object state, not to pass parameters. It’s not good to have many parameters to a method, but changing object state to reduce the number of parameters just hides the problem and makes it worse.

Among other things, this makes it impossible for the method to be thread safe because two threads would be changing the state. Another problem would be state that stays between method calls, if you forget to set some property the call uses the property value of the previous call, which can lead to obscure failures.

Answer 3

Well, yes, it's easier to test a pure method. You call it with some test parameters and check its return value; that's it. To test the method your way, you would have to stand up a mock object, and if the state you're testing for is private, things get complicated.

So which way is "best practice?" As always, it depends on what your goals are. Classes exist for a reason, and if the reason your class exists is so that you can write methods that mutate the object's state, then no, you don't need parameters on those methods except to pass in new values.

But if your goal is to provide methods that can act independently of any object state, then yes, you need to pass the method whatever it needs to do its work and return a value. The benefit is that such methods are easier to reason about; you don't have to wonder what state the object is currently in, or who might have modified it previously.

Answer 4

I think you are confused about 'class attribute'. You're talking about a member property of the class that contains the function. In other words, the same function can do different things with the same parameter values, depending on the state of it's parent. To me, that's the code smell.

In C# (not sure about other languages) an attributes belongs to the class that is passed as the parameter.

So if your function was:

void DoStuff(IAnimal animal){...}

You'd pass it one of these:

[AnimalNoise("Woof")]
class Dog : IAnimal {…}

Now your function could work out that Dogs are supposed to Woof without being told, which is better than a second parameter:

void DoStuff(IAnimal animal, string noise){...}

And much better than using state:

myObj.Noise = "woof";
myobj.DoStuff(new Dog());

If you've just got a choice between the last two, parameters are better than using the object state.

Answer 5

An answer depends on the specifics.

The difference between:

C obj = new C();
obj.f(p, q);
obj.f(p, r);
obj.f(p, s);

and

C obj = new C(p);
obj.f(q);
obj.f(r);
obj.f(s);

is minor.

The antipattern your colleague fears is:

class C {
    private static final P p = new P();

    void f(Q q) { ... }
}

This is not easily unit tested. If P is an enum or int, a trustworthy entity, then there is no problem. If p is a (singleton) object shared at many places it would not be good to hold it in more than one class as static field. In the new java date time API system Clock is such a "global variable," but unit testable.

Best would be if the obj is immutable:

class C {
    final P p;
    C(P p) {
        this.p = p;
    }
}

If C is an event type class, the method maybe to be used once:

class C {
    private P p;

    f(P p, Q q) {
        this.p = p;
        ...
    }

    private void g() {
        p;
    }
}

Then it seems indifferent when to pass the parameter p.

The problem arises when the parameter is always the same, a bean/service, in general a singleton, needing some creation.

In my experience one would make it injectable in the constructor, and have a method fWithP(Q q) without the p parameter.

Unittests should still be possible in either case.