2
\$\begingroup\$

I have inherited the Java function below, and it works the way it should, but you have to look at it for a minute to figure out exactly what is going on. Is there a more succinct or elegant way to encode this logic?

private Float applyFactors(Float originalValue, Float localFactor, Float globalFactor){
    if (globalFactor == null || globalFactor == 0){
        if (localFactor == null || localFactor == 0){
            return null;
        } else {
            return localFactor * originalValue;
        }
    } else {
        if (localFactor == null || localFactor == 0){
            return globalFactor * originalValue;
        } else {
            return localFactor * originalValue * globalFactor;
        }
    }
}
\$\endgroup\$

2 Answers 2

Reset to default
2
\$\begingroup\$

What I like to do in this sort of complicated conditional situation is to do the comparisons upfront and give them names. This makes it easier to reason about the code, and doesn't have any drawback if the conditional expressions are cheap.

I then go ahead and replace the conditional expressions with the named booleans, and it usually becomes more obvious how to simplify the code. Here's what I ended up with:

private Float applyFactors(Float originalValue, Float localFactor, Float globalFactor) {
    Float result = null;

    boolean hasGlobalFactor = ((globalFactor != null) && (globalFactor != 0));
    boolean hasLocalFactor =  ((localFactor != null) && (localFactor != 0));

    if (hasGlobalFactor && hasLocalFactor) {
        result = localFactor * globalFactor * originalValue;
    }
    else if (hasGlobalFactor) {
        result = globalFactor * originalValue;
    }
    else if (hasLocalFactor) {
        result = localFactor * originalValue;
    }

    return result;
}

This approach also makes the code more readable as well.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ +1. (I'd change the else { return null; } to return null;.) \$\endgroup\$ Commented Jan 19, 2012 at 21:03
  • \$\begingroup\$ Is it safe to use != 0 when you are using Float objects? There may be a different Float object representing 0, so you should be using the equals method \$\endgroup\$ Commented Jan 19, 2012 at 23:05
  • \$\begingroup\$ @luketorjussen That is a good point. I transposed the original code, but I don't have enough familiarity with Java to be able to answer that definitively. However, equals does seem like it would be the safer bet. \$\endgroup\$ Commented Jan 19, 2012 at 23:13
1
\$\begingroup\$

I prefer to break up the logic into pieces, removing the repeated arithmetic. This is how I'd have done it:

private Boolean nullOrZero(Float value) {
    return value == null || value.equals(0);
}

private Float oneIfInvalid(Float value) {
    return nullOrZero(value) ? new Float(1) : value;
}

private Float applyFactors(Float originalValue, Float localFactor, Float globalFactor) {
    if(nullOrZero(globalFactor) && nullOrZero(localFactor)) {
        return null;
    }
    return oneIfInvalid(localFactor) * oneIfInvalid(globalFactor) * originalValue;
}

There are more method calls, and nullOrZero() will be repeated once for each value when the first condition fails, but I believe improved readability is more important than premature micro optimization.

Another thing becomes noticeable in this code, which in my opinion was not obvious in the other versions; originalValue is never checked for null or zero. (Whether or not this was your intent is another matter.)

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.