Is it acceptable to assign a class variable within a method?

Question

I've just done a code review where I came across a method like this:

private void FindMinAndMax(double[] values)
{
    // Some algorithm that results in two class vars being assigned:
    _min = ...
    _max = ...
}

(After returning, the calling code utilises these two values in various calculations; the calling code then calls other private methods that utilise these two values).

Personally I don't like this method as you would never know that these vars are being set just from looking at the calling code (in fact the programmer had even put a comment next to the method call to say "This sets _min and _max", reinforcing my opinion!). My recommendation was to either use "out" parameters, or return a tuple containing the two values.

It got me wondering whether there is any guidance/pattern/best practice when it comes to methods assigning or modifying class-level variables?

Answer 1

To answer your topic question: it depends :)

Generally speaking, and in particular since you tagged this with clean code, we prefer readable and maintainable code.

Direct assignments like the one you gave are often resulting in more efficient code, hence, if that is an important factor it might be acceptable. You should have a reason for such an optimization need though. Possible reasons may be developing a hard real-time system or benchmarks clearly indicating the respective bottleneck.

Without such a reason, clearer code is more favorable. This is mostly due to the write-once-read-100-times effect of code. Many more people need to understand the code through its lifetime and I second your feeling that side-effecting methods are problematic in that respect.

Depending on how much your team values clean code, the requested code changes may be more or less dramatic. Here are a few ideas on how such a situation could be addressed leading to more readable and easier maintainable code. It's up to you and your team to decide which is most suitable for your project.

• You could split the method into two separate ones, one for min and one for max. Downside: this may lead to duplication.
• As suggested in the other answer, you may want to introduce an Interval or Range type of some sort to capture min and max as one "thing".
• You may be able to calculate these values once and store them as const/final, opening up ways to inject these values into the object creation.
• You may find that the whole storage of these values in fields is questionable.

And if your team is going really far on code quality, then you can completely ignore the body of your function and address the flaws already inherent in its signature:

• The method name suggest that there is something to be found, but it returns void. Hence, either the name or return type should be changed.
• The find part of the name (just like a search) implies that you may potentially not find anything. How do you deal with that? Even once you return f.ex. an interval type you may still go further and think about Maybe/Optional-kind of types - after all, what is the minimum or maximum of an empty array?

In summary, I'd have a strong tendency to reject this code in a review. However, due to the very local nature of your example, it is next to impossible to offer you a productive criticism. After all, you should not reject code in a review with "I don't like it", but at best you should offer an arguably better variant. Determining that requires more knowledge about the overall design though.

Regarding best practices: I don't believe best practices that work on such a narrow scope make much sense at all. Once you get to a higher level with principles, like single responsibility, tell don't ask, etc., they result in strong implications for these smaller pieces.

Answer 2

Without knowing what the rest of the code looks like, I'd say that this is begging for another object that represents the max and min.

When you have state that comes into existence later, that suggests another object. Otherwise, you've got state that is uninitialized for some time, then later is initialized, so the one object is mixing/conflating state with different lifetimes. That conflation can be removed with another object (class instance or tuple), so each then has consistent state lifetime.

That way you're addressing a number of design principles. Single Responsibility, and also, it makes better abstraction. So, there's two objects, the original with its original purpose yet it can create the min/max object, and then min/max object with it's separate purpose and lifetime.

An object is an abstraction — a grouping of things together having a common purpose and lifetime. If you find a different lifetime, that suggests the code is switching abstractions, which suggests another object.

FWIW, the same issues, conflating state lifetimes, occurs in the other question cited by @user2180613, which in that case seems best resolved by using local variables instead of object fields.

---

It is also worrisome that this is an instance method of an already constructed object that takes the doubles as a parameter. This suggests that the class in question isn't tied to a particular double array, and, that this method could be called again later with a different double array, changing the min and max. Of course, the method is private so maybe this is fine and just internal detail of the constructor; we can't tell from the snippet.

All in all, separating these out into a tuple or other class instance to return would be cleaner and would not beg these questions.

Answer 3

Methods changing the objects state are absolutely OK (unless the object is a DTO or ValueObect).

(After returning, the calling code utilises these two values in various calculations; the calling code then calls other private methods that utilise these two values).

This is the problem but in the opposite direction.

That the "calling code" works on those properties is feature envy and this code most likely belongs into this class.

Answer 4

Broadly speaking, Yes it is ok.

But, there are arguments against it.

• Thread Safety: If the method is called from two threads at the same time, do you have a race condition?

• Global variables: Is it possible to get the wrong result by calling the functions which use min and max in the wrong order?

Functional programming is getting a lot of traction over OOP these days because it avoids problems of internal state.

You could return min and max from this function (as a Tuple?) rather than set state if the values are used immediately