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
\$\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\$
  • 1
    \$\begingroup\$ +1. (I'd change the else { return null; } to return null;.) \$\endgroup\$ – palacsint Jan 19 '12 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\$ – luketorjussen Jan 19 '12 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\$ – Rohan Singh Jan 19 '12 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\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.