2

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
  • 2
    You have to wonder if that method is even meant to be public and therefore subject to unit tests. If it is public and is always supposed to receive a specific member as the argument, how do you enforce that on external callers? For that matter, should the callers even be able to access that member? If the method is not public, then why do you test it? Should it be factored out of the class entirely? – Sebastian Redl Dec 18 '19 at 9:23
  • 4
    Please don't take Clean Code literally. Use it not as a rulebook, but as a collection of ideas that you can use if you need it. – T. Sar Dec 18 '19 at 10:55
  • 1
    @AndresF. In other words - just because Martin says this is a good guideline doesn't mean he is right. I love Clean Code, but that book is by no means perfect and has quite a few dangerous generalizations that can seriously damage a newbie coder's perspective of things. More so, that book is seriously outdated - languages are far larger and broader than they were when it was written back then. – T. Sar Dec 18 '19 at 15:49
  • 1
    Absolutely agree with your comments. I've added a code example to clarify my situation. I also agree that Clean Code shouldn't be taken as gospel, and neither should any of the literature. Still, I feel that if I were to go against advice, I should be able to back it up with reasoning. – Lagerbaer Dec 18 '19 at 16:37
  • 1
    @AndresF. You have a point! That said, it is sad how many times we have to remember people that stuff is in fact just a guideline, and not gospel. I'm glad this isn't the case of the OP, tho. – T. Sar Dec 18 '19 at 18:51
3

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.

| improve this answer | |
  • You got it exactly right. That's the structure. And you also provide the reasoning I'd have for saying to drop the parameter foo from AddFoo and use the member attribute _foo instead: Don't test implementation details such as private methods. – Lagerbaer Dec 18 '19 at 16:26
5

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.

| improve this answer | |
  • Oh the member attribute doesn't get set prior to a function call. It gets set once in the constructor and then stays the same for the lifespan of the object. – Lagerbaer Dec 18 '19 at 16:31
4

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.

| improve this answer | |
1

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.

| improve this answer | |
  • I am totally talking about a member property and not a class attribute. Apologies for the naming confusion. – Lagerbaer Dec 18 '19 at 16:27
1

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.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.