What Style is Better (Instance Variable vs. Return Value) in Java

Question

I often find myself struggling to decide which of these two ways to use when I require to use common data across some methods in my classes. What would be a better choice?

In this option, I can create an instance variable to avoid the need of having to declare additional variables, and also to avoid defining method parameters, but it may be not so clear where those variables are being instantiated/modified:

public class MyClass {
    private int var1;

    MyClass(){
        doSomething();
        doSomethingElse();
        doMoreStuff();
    }

    private void doSomething(){
        var1 = 2;
    }

    private void doSomethingElse(){
        int var2 = var1 + 1;
    }

    private void doMoreStuff(){
        int var3 = var1 - 1;
    }
}

Or just instantiating local variables and passing them as arguments?

public class MyClass {  
    MyClass(){
        int var1 = doSomething();
        doSomethingElse(var1);
        doMoreStuff(var1);
    }

    private int doSomething(){
        int var = 2;
        return var;
    }

    private void doSomethingElse(int var){
        int var2 = var + 1;
    }

    private void doMoreStuff(int var){
        int var3 = var - 1;
    }
}

If the answer is that they are both correct, which one is seen/used more often? Also, if you can provide additional pros/cons for each option would be very valuable.

Answer 1

I'm surprised this hasn't mentioned yet...

It depends if var1 is actually part of your object's state.

You assume that both of these approaches are correct and that it's just a matter of style. You are wrong.

This is entirely about how to properly model.

Similarly, private instance methods exist to mutate your object's state. If that's not what your method is doing then it should be private static.

Answer 2

I don't know which is more prevalent, but I would always do the latter. It more clearly communicates the data flow and lifetime, and it doesn't bloat every instance of your class with a field whose only relevant lifetime is during initialization. I would say the former is just confusing, and makes code review significantly more difficult, because I have to consider the possibility that any method might modify var1.

Answer 3

You should reduce the scope of your variables as much as possible (and reasonable). Not only in methods, but generally.

For your question that means it depends whether the variable is part of the object's state. If yes, it's OK to use it in that scope, i.e. the whole object. In this case go with the first option. If no, go with the second option as it reduces the visibility of variables and thus overall complexity.

Answer 4

What Style is Better (Instance Variable vs. Return Value) in Java

There's another style - use a context/state.

public static class MyClass {
    // Hold my state from one call to the next.
    public static final class State {
        int var1;
    }

    MyClass() {
        State state = new State();
        doSomething(state);
        doSomethingElse(state);
        doMoreStuff(state);
    }

    private void doSomething(State state) {
        state.var1 = 2;
    }

    private void doSomethingElse(State state) {
        int var2 = state.var1 + 1;
    }

    private void doMoreStuff(State state) {
        int var3 = state.var1 - 1;
    }
}

There are a number of benefits to this approach. The state objects can change independently of the object for example, giving much wiggle room for the future.

This is a pattern that also works well in a distributed/server system where some details must be preserved across calls. You can store user details, database connections, etc. in the state object.

Answer 5

It's about side effects.

Asking whether var1 is part of state misses the point of this question. Sure if var1 must persist, it has to be an instance. Either approach can be made to work whether persistence is needed or not.

The side effect approach

Some instance variables are only used to communicate between private methods from call to call. This kind of instance variable can be refactored out of existence but it doesn't have to be. Sometimes things are clearer with them. But this is not without risk.

You are letting a variable out of its scope because it is used in two different private scopes. Not because it's needed in the scope you're placing it. This can be confusing. The "globals are evil!" level of confusing. This can work but it just won't scale well. It only works in the small. No big objects. No long inheritance chains. Don't cause a yo yo effect.

The functional approach

Now, even if var1 must persist nothing says you have to use if for every transient value it might take on before it reaches the state you want preserved between public calls. That means you can still set a var1 instance using nothing but more functional methods.

So part of state or not, you can still use either approach.

In these examples 'var1' is so encapsulated nothing besides your debugger knows it exists. I'm guessing you did that deliberately because you don't want to bias us. Fortunately I don't care which.

The risk of side effects

That said, I know where your question is coming from. I've worked under miserable yo yo'ing inheritance that mutates an instance variable at multiple levels in multiple methods and gone squirrelly trying to follow it. This is the risk.

This is the pain that drives me to a more functional approach. A method can document its dependencies and output in its signature. This is a powerful, clear approach. It also lets you change what you pass the private method making it more reusable within the class.

The upside of side effects

It's also limiting. Pure functions have no side effects. That can be a good thing but it's not object oriented. A big part of object orientation is the ability to refer to a context outside of the method. Doing that without leaking globals all over here and gone is OOP's strength. I get the flexibility of a global but it's nicely contained in the class. I can call one method and mutate every instance variable at once if I like. If I do that I'm obligated to at least give the method a name that makes clear what it's up to so people won't be surprised when that happens. Comments can help as well. Sometimes these comments are formalized as "post conditions".

The downside of functional private methods

The functional approach makes some dependencies clear. Unless your in a pure functional language it can't rule out hidden dependencies. You don't know, looking just at a methods signature, that it isn't hiding a side effect from you in the rest of it's code. You just don't.

Post conditionals

If you, and everyone else on the team, reliably documents the side effects (pre/post conditions) in comments, then the gain from the functional approach is much less. Yeah I know, dream on.

Conclusion

Personally I tend towards functional private methods in either case if I can, but honestly it's mostly because those pre/post conditional side effect comments don't cause compiler errors when they're outdated or when methods are called out of order. Unless I really need the flexibility of side effects I'd rather just know that things work.

Answer 6

The first variant looks non-intuitive and potentially dangerous to me (imagine for whatever reason someone makes your private method public).

I would much rather instantiate your variables upon class construction, or pass them in as arguments. The latter gives you the option of using functional idioms and not relying on the state of the containing object.

Answer 7

There already are answers talking about object state and when the second method is preferred. I just want to add one common use case for the first pattern.

The first pattern is perfectly valid when all what your class does is that it encapsulates an algorithm. One use case for this is that if you wrote the algorithm to a single method it would be too large. So you break it down into smaller methods make it a class and make the sub methods private.

Now passing all the state of the algorithm via parameters might get tedious so you use the private fields. It is also in agreement with the rule in the first paragraph because it is basically a state of the instance. You only have to keep in mind and properly document that the algorithm is not reentrant if you use private fields for this. This shouldn't be an issue most of the time but it can possibly bite you.

Answer 8

Let's try an example that does something. Forgive me since this is javascript not java. The point should be the same.

Visit https://blockly-games.appspot.com/pond-duck?lang=en, click the javascript tab, and paste this:

hunt = lock(-90,1), 
heading = -135;

while(true) {
  hunt()  
  heading += 2
  swim(heading,30)
}

function lock(direction, width) {
  var dir = direction
  var wid = width
  var dis = 10000

  //hunt
  return function() {
    //randomize() //Calling this here makes the state of dir meaningless
    scanLock()
    adjustWid()
    if (isSpotted()) {
      if (inRange()) {
        if (wid <= 4) {
          cannon(dir, dis)
        }
      }
    } else {
      if (!left()) {
        right()
      }
    }
  }

  function scanLock() {
    dis = scan(dir, wid);
  }

  function adjustWid() {
    if (inRange()) {
      if (wid > 1)
        wid /= 2;
    } else {
      if (wid < 16) {
        wid *= 2; 
      }
    }
  }

  function isSpotted() {
    return dis < 1000;
  }

  function left() {
    dir += wid;
    scanLock();
    return isSpotted();
  }

  function right() {
    dir -= wid*2;
    scanLock();
    return isSpotted()
  }

  function inRange() {
    return dis < 70;
  }

  function randomize() {
    dir = Math.random() * 360
  }
}

You should notice that dir, wid, and dis are not getting passed around a lot. You should also notice that the code in the function that lock() returns looks a lot like pseudo code. Well it's actual code. Yet it's very easy to read. We could add passing and assigning but that would add clutter you would never see in pseudo code.

If you wish to argue that not doing the assign and passing is fine because those three vars are persistent state then consider a redesign that assigns dir a random value every loop. Not persistent now, is it?

Sure, now we could drop dir down in scope now but not without being forced to clutter our pseudo like code with passing and setting.

So no, state is not why you decide to use or not use side effects rather than passing and returning. Nor do side effects alone mean your code is unreadable. You don't get the benefits of pure functional code. But done well, with good names, they can actually be nice to read.

That isn't to say they can't turn into a spaghetti like nightmare. But then, what can't?

Happy duck hunting.