3
\$\begingroup\$

As part of a code review I came across this isDummy(a,b)-Method and the question, whether it is more readable as a conjunction, or a disjunction:

Original snippet:

public static boolean isDummy(TriState a, TriState b) {
    return (a.is(ZERO) || b.is(ZERO)) && (a.is(NULL) || b.is(NULL));
}

Suggested change:

public static boolean isDummy(TriState a, TriState b) {
    return a.is(ZERO) && b.is(NULL) || a.is(NULL) && b.is(ZERO);
}

Both of the above return true if one TriState is ZERO and the other is NULL.

Both do exactly the same; however, the latter imho clearly states, that just two possible combinations are a dummy.
The first one could - without knowing that ZERO is not a VALUE and TriState#is(TriState) does an equality check - possibly suggest, that four possible combinations are dummy values.

As you can see in TriState's constructor the author's intention was to implement a helper mechanism to check Doubles for its "valueness". Infinity and NaN aren't taken into account:

enum TriState {
    NULL, ZERO, VALUE;

    public static TriState of(Double number) {
        if (number == null) {
            return TriState.NULL;
        } else if (number == 0d) {
            return TriState.ZERO;
        } else {
            return VALUE;
        }
    }

    private boolean is(TriState other) {
        return this == other;
    }

    /**
    * Tests if this object is {@link #VALUE} and the given TriState is not {@link #VALUE}.
    *
    * @param other the TriState to test against
    * @return true if this object is {@link #VALUE} AND the given TriState is not {@link #VALUE}. False otherwise.
    */
    public boolean andNot(TriState other) {
        return this.is(VALUE) && !other.is(VALUE);
    }

    /**
    * Compares two TriState objects to find out if exactly one is ZERO and exactly one is NULL. This method is commutative.
    *
    * @param a a TriState instance to check
    * @param b a TriState instance to check
    * @return true, if one of the given TriState objects is {@link #ZERO} while the other is {@link #NULL}. False otherwise.
    */
    public static boolean isDummy(TriState a, TriState b) {
        return (a.is(ZERO) || b.is(ZERO)) && (a.is(NULL) || b.is(NULL));
    }
}

While the mere existence of that Enum instead of just using Double instances directly is a whole different matter, I would be interested if there are any known best practices regarding that boolean expression.

Which of the both is easier to understand?

\$\endgroup\$
  • \$\begingroup\$ This is my first question on CR, I tried hard to find a good title and the right tags. If there is anything I can improve, please enlighten me. \$\endgroup\$ – Clijsters Feb 6 at 13:36
  • \$\begingroup\$ Can you confirm that the code you've posted for review is your own code (either written by you, or now maintained by you), so that it's certainly on-topic? \$\endgroup\$ – Toby Speight Feb 6 at 17:37
  • 1
    \$\begingroup\$ Yes this code is maintained by me and a colleague and we are discussing on how to improve its readability. The original author of that specific method told me that his (original) version is a well known best practice so we decided to ask the community. \$\endgroup\$ – Clijsters Feb 6 at 21:03
  • \$\begingroup\$ Thanks for the clarification! \$\endgroup\$ – Toby Speight Feb 7 at 9:28
3
\$\begingroup\$

The readability can be improved by using explanatory variables, (of course changing the variable names as they make sense to you).

public static boolean isDummy(TriState a, TriState b) {
    boolean oneOfTheInputsIsZero = a.is(ZERO) || b.is(ZERO);
    boolean oneOfTheInputsIsNull = a.is(NULL) || b.is(NULL);
    return oneOfTheInputsIsZero && oneOfTheInputsIsNull;
}

First one makes it obvious that the function is commutative, in the second one you have to check the LHS and RHS are indeed the mirror image of each other.

Although the two functions may evaluate the same thing they don't say the same thing. First one says "There is at least one zero and there is at least one null." Second one says "There is one zero and the other one is null." (which matches the javadoc)

But the comments sometimes also lie. To check the veracity of the comment consider how would you write the function if it took more parameters.

They still say what they say before but now they don't evaluate the same coincidentally.

public static boolean isDummy(TriState a, TriState b, TriState c) {
    return (a.is(ZERO) || b.is(ZERO) || c.is(ZERO))
        && (a.is(NULL) || b.is(NULL) || c.is(NULL));
}


public static boolean isDummy(TriState a, TriState b, TriState c) {
    return a.is(ZERO) && b.is(NULL) && c.is(NULL) 
        || a.is(NULL) && b.is(ZERO) && c.is(NULL) 
        || a.is(NULL) && b.is(NULL) && c.is(ZERO);
}
\$\endgroup\$
  • \$\begingroup\$ This one together with removing is() seems the best option imo. Thanks! Also your answer seems to best illustrate the different "kind of expressing". \$\endgroup\$ – Clijsters Feb 7 at 16:30
1
\$\begingroup\$

This question is on the edge of being closed as off topic:

Authorship of code: Since Code Review is a community where programmers improve their skills through peer review, we require that the code be posted by an author or maintainer of the code, that the code be embedded directly, and that the poster know why the code is written the way it is.

I'll assume you are (now) the maintainer of the code, since you are doing a code review of it, and considering changing it. If you have inherited this code, you probably have access to the original author of the code, and should ask them why they wrote it the way they did. If they are no longer around, and this is now your code to maintain, and you are considering changing it for clarity ...

I would drop the .is() altogether, and use:

return a==ZERO && b==NULL   ||   b==ZERO && a==NULL;

The .is() method is private, and can't be used outside of the class. The implementation of the .is() method is not doing anything other than a == test, so it is adding visual noise (extra characters), and mental noise (another function to examine to understand the code), and possible inefficiency (extra function calls) for no apparent gain.

\$\endgroup\$
  • 2
    \$\begingroup\$ You've might have already read answers to Short answers and code-only answers - could you explain to the OP and others why removing the .is() calls is better? e.g. fewer function calls, fewer characters, etc. \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Feb 6 at 16:52
  • \$\begingroup\$ Thanks for your answer. As I told in a comment on the question I asked the original author and his answer didn't really had a real why that's why I'm asking whether this might be a best practice I don't know or the other way is more logical. Don't think it is off topic. \$\endgroup\$ – Clijsters Feb 6 at 21:07
  • \$\begingroup\$ Removing the is () is honestly something I didn't really consider... It looked so... familiar \$\endgroup\$ – Clijsters Feb 6 at 21:09
0
\$\begingroup\$

With given definition of what this method do:

return true if one TriState is ZERO and the other is NULL

changed method looks better.

Also, as @AJNeufeld proposed, it's better to get rid of is() (because why do you even need this? :) )

\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

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