12

I noticed that even when paying respect to the single responsibility principle of OOD, sometimes classes still grow large. Sometimes accessing member variables directly in methods feels like having global state, and a lot of stuff exists in the current scope. Just by looking at the method currently working in, it is not possible anymore to determine where invidiual variables accessible in the current scope come from.

When working together with a friend lately, I realized I write much more verbose code than him, because I pass member variables still as parameters into every single method.

Is this bad practice?

edit: example:

class AddNumbers {
public:
    int a, b;
    // ...
    int addNumbers {
        // I could have called this without arguments like this:
        // return internalAlgorithmAddNumbers();
        // because the data needed to compute the result is in members.
        return internalAlgorithmAddNumbers(a,b);
    }

private:
    int internalAlgorithmAddNumbers(int sum1, int sum2) { return sum1+sum2; }
};
Tom
  • 3,115
  • 6
  • 33
  • 38
  • 1
    If your classes are too big split them up. If you have a member variable use it. If you're not using member variables in a method it should probably be `static`, but doing that lots sounds odd. – Flexo Aug 04 '12 at 17:29
  • 1
    Excuse me, I seem to be having a slight problem with your English. You pass member variables as parameters? Public member variables? Or new values for these member variables? I am a little bit confused. – ATaylor Aug 04 '12 at 17:29
  • Sorry that I wasnt clear on that. When implementing an algorithm, I usually re-iterate the parameters it takes in the method's signature, although the data the algorithm requires could actually be pulled directly from the member variables. Lets assume you have a class, that adds two numbers, and has the 2 numbers a and b as member variables. Then instead of having a private add method with zero parameters, I would still define a function taking 2 int arguments. Like this I could later reuse the algorithm also outside of a class. – Tom Aug 04 '12 at 17:32
  • hmm.. if you want to reuse it, then don't put it into the class... or, if it really belongs to the class, then as Flexo mentioned it, create static methods – Karoly Horvath Aug 04 '12 at 17:34
  • I added an example. The algorithm definity belongs to the class at the moment where the code was written. And it definitly requires the shared state in the context of this class. But as life shows, often later on you catch yourself that you need to tackle a different problem, and you want to re-use code, but you only need certain parts of your old code, in the worst case copy paste recycling of code. If the algorithm then already states its required parameters in the signature, its easier to re-use. – Tom Aug 04 '12 at 17:40

3 Answers3

6

If a class has member variables, use them. If you want to pass parameters explicitly, make it a free function. Passing member variables around not only makes the code more verbose, it also violates people's expectations and makes the code hard to understand.

The entire purpose of a class is to create a set of functions with an implicitly passed shared state. If this isn't what you want to do, don't use a class.

Antimony
  • 37,781
  • 10
  • 100
  • 107
3

Yes, definetely a bad practice. Passing a member variable to a member function has no sense at all, from my point of view. It has several disadvantages:

  1. Decrease code readability
  2. Cost in term of performances to copy the parameter on the stack

Eventually converting the method to a simple function, may have sense. In fact, from a performance point of view, call to non-member function are actually faster (doesn't need to dereference this pointer).

EDIT:

Answer to your comment. If the function can perform its job only using a few parameters passed explicitely, and doesn't need any internal state, than probably there is no reason to declare it has a member function. Use a simple C-style function call and pass the parameters to it.

Heisenbug
  • 38,762
  • 28
  • 132
  • 190
  • 1
    That's exactly my point in the question. Because I feel that it actually _increases_ code readability, that no magic external names pop up in your local scope. A long time ago I was taught global variables are very bad style, and accessing a member variable sometimes feels like using global variables. You're accessing something that is neither in your local scope, nor passed in from the outside as a parameter. – Tom Aug 04 '12 at 17:43
  • 1
    @Tom - class memebers aren't global - they're deliberately explicitly local to the instance of the class you're working with. Your class is an encapsulation of some small, related state. If they're not then you have bigger design problems that can't be patched up like this. In the example you gave I'd be tempted to make your instances immutable even. – Flexo Aug 04 '12 at 17:50
2

I understand the problem, having had to maintain large classes in code I didn't originally author. In C++ we have the keyword const to help identify methods that don't change the state:

void methodA() const;

Use of this helps maintainability because we can see if a method may change the state of an object.

In other languages that don't have this concept I prefer to be clear about whether I'm changing the state of the instance variable by either having it passed in by reference or returning the change

this->mMemberVariable = this->someMethod();

Rather than

void someMethod()
{
   this->mMemberVariable = 1; // change object state but do so in non transparent way
}

I have found over the years that this makes for easier to maintain code.

RickF
  • 39
  • 2