Class fields/variables. Keep single reference point at class level or pass to individual methods?

Question

Often I see a class where a value is injected into a method or even a constructor. That value is then used by several methods within that class and a reference is simply passed through to each method that requires it. To me this seems like a code smell, although I could very well just be a pedant.

Unless a variable is only required once within the scope of a single method, I would usually have a class level field that is initialised by the method that receives the value. Any other methods would then reference this field if needed. This seems to be a lot cleaner to me.

Thoughts?

Answer 1

Fields should store object state. The lifetime of the fields should be the same as the lifetime of the object. Do not use them as temporary storage to avoid passing transient values through a method chain.

There is a code smell, but adding a field to the existing class is not the proper solution. Instead, factor out the related private methods into a separate class. Then you can extract those method arguments into fields of the new class.

Instead of:

class A {
  int m;
  ... // other members;
  def f(x) { u = g(x); v = h(x); m = j(t1, t2); }
}

write:

class F {
  int x;
  F(x) { this.x = x; } // constructor
  int f() { t1 = g(); t2 = h(); return j(t1, t2); }
}

class A {
  int m;
  ...
  f(x) { m = new F(x).f(); }
}

This refactoring has the additional benefit of allowing F to be tested separately from A.

Answer 2

Consider thread safety

In Java, all variables on the thread stack are isolated from other threads. This means that if the variable is passed in as a method parameter, or is scoped within a method, then it is immune from mutation by other threads. (This assumes that shared references are not being modified elsewhere such as would be the case if the variable was immutable like a String or Joda DateTime.)

This is not the case for field variables which may be mutated by other threads sharing the same object instance if appropriate handling is not in place (atomic operations, synchronization etc).

So, a common way to prevent this kind of side-effect is to keep state out of fields and only on the thread stack.

Answer 3

It is a matter of ownership. If the class owns the value being passed in, or it can contain a shared reference to the (largely constant) value then the added coupling of having the class keep its own instance of the value makes sense. Otherwise, I would let the value be simply a parameter passed along. It decreases the coupling between the value and the class as well as being more friendly to unit testing and concurrent access.