Parameterizing vs property assignment

Nope. Your way is better.

Here’s why: the variables are properly confined to only the scope in which they are used. While your colleague’s way will work perfectly fine, it will add cognitive dissonance (even if it’s only a small amount) to the programmer coming after him, who will read the code and have to figure out why there are what amounts to “global variables” within the class. Are they used anywhere else? What will happen if I change one of them? And so forth.

Your colleague is wrong

My colleague believes that class properties may also be useful to increase readability by decreasing the parameter count of internal private functions.

Decreasing the number of parameters does not necessarily make code more readable.

When working with a function or method, it is critical to know what its inputs and outputs are. Your colleague’s approach makes it difficult to tell and impossible to tell from the function’s prototype.

Also, if your code executes on more than one thread, member-scoped variables can be a problem because object instances are held in the same heap that is used by all threads. This differs from local variables which are held on a thread-specific stack. Heap variables run the risk of contamination from other threads of execution, while stack variables are much safer.

OK, maybe he’s right, but only sometimes

That being said, there may be some cases where very complicated functions with many working variables/logical parts/recursion/repetition where you find you are passing around a ridiculous amount of stuff. In those cases there are a couple patterns that may assist, and these could use member variables as your colleague prefers. Here are two I can think of:

Method Object

Turn the method into its own object so that all the local variables become fields on that object. You can then decompose the method into other methods on the same object.

E.g. let’s say you need to do a Diffie-Helman key exchange. This algorithm has a lot of working variables and maybe it’s a pain to pass them all around, so you want them declared at a member level.

var o = new DiffieHelmanExchanger();
o.Execute();
var result = o.SessionKey;
o.Dispose();

Notes:

• The class name will often end with “-er” because it is associated with an action, not an item.

• All the member variables are created just for the task and then erased when the task is done.

• There is only one public method.

• You don’t keep it around very long. Create it, get what you want, dispose of it.

Preserve whole object

Problem: You get several values from an object and then pass them as parameters to a method. Instead, try passing the whole object.

With this pattern, you manage a large set of parameters by removing them from the argument list and replacing them with a single “parameter object.” The object has one property for each of the parameters. If the function needs to pass parameters to yet another function, it can pass along the whole object instead of passing the parameters one by one.

This is sort of what your colleague is advocating, but instead of storing the working variables in the same object, they’re stored in an object dedicated to handling the state of the parameters.

Some benefits:

• Since objects are always passed by reference (in a sense), changes made to the parameter object’s members will be available to any subfunctions that receive the parameter object as an argument.

• With this pattern it is still somewhat clear what the inputs and outputs are

• This pattern provides a way to distinguish between working variables (which are stored in the parameter object being passed) and bone fide state variables (which are stored in the object that contains the function that is being executed).

• This pattern is more resilient to multithreading concerns, because each thread can have its own copy of the parameter object.

I can tell you from first hand experience that your coworker’s method becomes an unmaintainable mess. Any time you have a non-obvious call order dependency it’s going to eventually cause a bug.

Let’s say someone later needs to modify doSomething() to use another value:

class Example {

    private $valueFromConstructor;
    private $value1;
    private $value2;
    private $value3;
    private $value4;
    private $value5;

    public function __construct($valueFromConstructor) {
        $this->valueFromConstructor = $valueFromConstructor;
    }

    public function doSomething() {
        $this->computeValue1();
        $this->computeValue2();
        $this->computeValue3WithValue1AndValue2();
        $this->computeValue5WithValue3AndValue4();
        $this->computeValue4WithValue1AndValue3();
        $this->doSomethingWithValue4();

        return $this->value5;
    }
}

Is the bug obvious?

With your method it would be much clearer:

class Example {

    private $valueFromConstructor;

    public function __construct($valueFromConstructor) {
        $this->valueFromConstructor = $valueFromConstructor;
    }

    public function doSomething() {
        $value1 = $this->computeValue1();
        $value2 = $this->computeValue2();
        $value3 = $this->computeValue3WithValue1AndValue2($value1, $value2);
        $value5 = $this->computeValue5WithValue3AndValue4($value3, $value4);
        $value4 = $this->computeValue4WithValue1AndValue3($value1, $value3);

        return $this->doSomethingWithValue4($value4);
    }
}

I’m not familiar with PHP, but in many programming languages this would generate a warning or error due to using $value4 before declaring it.

Or, even worse – what if someone did this:

public function computeValue4WithValue1AndValue3()
{
    $value4 = $value1 * $value3;
    $value2 = $value4 / $value1;
}

Now the code becomes incredibly hard to read and understand. You start getting very different behavior based on the call order of multiple methods.