extract boolean checks to local variables

Question

Sometimes i extract boolean checks into local variables to achief better readability.

What do you think?

Any disadvantages?

Does the compiler a line-in or something if the variable isn't used anywhere else? I also thought about reducing the scope with an additional block "{}".

if (person.getAge() > MINIMUM_AGE && person.getTall() > MAXIMUM_SIZE && person.getWeight < MAXIMUM_WEIGHT) {
 // do something
}

---

final boolean isOldEnough = person.getAge() > MINIMUM_AGE;
final boolean isTallEnough = person.getTall() > MAXIMUM_SIZE;
final boolean isNotToHeavy = person.getWeight < MAXIMUM_WEIGHT;

if (isOldEnough && isTallEnough && isNotToHeavy) {
 // do something
}

Answer 1

I do this all the time. The code is much more readable that way. The only reason for not doing this is that it inhibits the runtime from doing shortcut optimisation, although a smart VM might figure that out.

Answer 2

I think the checks probably belong in the person class. You could pass in the Min/Max values, but calling person.IsEligable() would be a better solution in my opinion.

Answer 3

You could go one step further and create subtypes of the Person:

Teenager extends Person
ThirdAgePerson extends Person
Kid extends Person

Subclasses will be overriding Person's methods in their own way.

Answer 4

One advantage to the latter case is that you will have the isOldEnough, isTallEnough, and isNotToHeavy (sic) variables available for reuse later in the code. It is also more easily readable.

You might want to consider abstracting those boolean checks into their own methods, or combining the check into a method. For example a person.isOldEnough() method which would return the value of the boolean check. You could even give it an integer parameter that would be your minimum age, to give it more flexible functionality.

Answer 5

I think this is a matter of personal taste. I find your refactoring quite readable.

In this particualr case I might refactor the whole test into a

isThisPersonSuitable()

method.

If there were much such code I might even create a PersonInterpreter (maybe inner) class which holds a person and answers questions about their eligibility.

Generally I would tend to favour readability over any minor performance considerations.

Answer 6

The only possible negative is that you lose the benefits of the AND being short-circuited. But in reality this is only really of any significance if any of your checks is largely more expensive than the others, for example if person.getWeight() was a significant operation and not just an accessor.

Answer 7

The real risk in this approach is that it loses responsiveness to changing values.

Yes, people's age, weight, and height don't change very often, relative to the runtime of most programs, but they do change, and if, for example, age changes while the object from which your snippet is still alive, your final isOldEnough could now yield a wrong answer.

And yet I don't believe putting isEligible into Person is appropriate either, since the knowledge of what constitutes eligibility seems to be of a larger scope. One must ask: eligible for what?

All in all, in a code review, I'd probably recommend that you add methods in Person instead.

boolean isOldEnough (int minimumAge)  { return (this.getAge() > minimumAge); }

And so on.

Answer 8

I have nothing against your construct, but it seems to me that in this case the readability gain could be achieved by simply putting in line breaks, i.e.

if (person.getAge() > MINIMUM_AGE
  && person.getTall() > MAXIMUM_SIZE
  && person.getWeight < MAXIMUM_WEIGHT)
{
  // do something
}

The bigger issue that other answers brought up is whether this belongs inside the Person object. I think the simple answer to that is: If there are several places where you do the same test, it belongs in Person. If there are places where you do similar but different tests, then they belong in the calling class.

Like, if this is a system for a site that sells alcohol and you have many places where you must test if the person is of legal drinking age, then it makes sense to have a Person.isLegalDrinkingAge() function. If the only factor is age, then having a MINIMUM_DRINKING_AGE constant would accomplish the same result, I guess, but once there's other logic involved, like different legal drinking ages in different legal jurisdictions or there are special cases or exceptions, then it really should be a member function.

On the other hand, if you have one place where you check if someone is over 18 and somewhere else where you check if he's over 12 and somewhere else where you check if he's over 65 etc etc, then there's little to be gained by pushing this function into Person.

Answer 9

Your two blocks of code are inequivalent.

There are many cases that could be used to show this but I will use one. Suppose that person.getAge() > MINIMUM_AGE were true and person.getTall() threw an exception.

In the first case, the expression will execute the if code block, while the second case will throw an exception. In computability theory, when an exception is thrown, then this is called 'the bottom element. It has been shown that a program when evaluated using eager evaluation semantics (as in your second example), that if it terminates (does not resolve to bottom), then it is guaranteed that an evaluation strategy of laziness (your first example) is guaranteed to terminate. This is an important tenet of programming. Notice that you cannot write Java's && function yourself.

While it is unlikely that your getTall() method will throw an exception, you cannot apply your reasoning to the general case.