Is it simpler to group tests or to keep them separate?

Question

I just had a discussion with a colleague where we disagreed about which of the following snippets was simpler:

public boolean foo(int x, int y) {
    if (x < 0)
        return false;
    if (y < 0)
        return false;
    // more stuff below
}

OR

public boolean foo(int x, int y) {
    if (x < 0 || y < 0)
        return false;
    // more stuff below
}

It is obvious which is shorter; it is also obvious that their cyclomatic complexity is identical (so for that definition of "simple", of course, they are the same).

What is your feeling, and why? Which is more readable; which is easier to debug?

Answer 1

I'd go for the second one, with one change:

public boolean foo(int x, int y) {
    if ((x < 0) || (y < 0))
        return false;
    // more stuff below
}

The reason being that it follows the way a human would word it in a natural language (we'd say "if any of the two is negative, then the answer is 'no'" rather than "if the first one is negative, the answer is 'no'; if the second one is negative, the answer is also 'no'").

Answer 2

I think the styles suggest different things:

if (x < 0)
    return false;
if (y < 0)
    return false;

says that something is done if x < 0 and another thing is done if y < 0.

Whereas

if (x < 0 || y < 0)
    return false;

implies that one thing is done if either one of the conditions is true.

So it depends on the actual code. Obviously, you would not split the statement if both expressions should do the same because this would lead to duplicate code and would just feel wrong.

Answer 3

In the completely generic case you describe, I would choose the second one because I hate to repeat code.

But in real life, I would make that decision based on whether the two tests are related.

For example

if (user.isDisabled() || user.isSuspended())
    return false;

both tests are about whether the user can do something.

but

if (user.isDisabled())
    return false;
if (catalog.isClosedForOrders()) 
    return false;

one test is about the user, one is about the system; I'd separate those for ease of maintenance when one changes later independently of the other.

Answer 4

Well, largely it would depend on my mood at the time.

I think the most objective consideration would be how closely related are x & y. Checking one variable for a high & low range -- one if. Check a string for null and a int for range -- two ifs.

Answer 5

Better names, then it is truly clear...

public boolean foo(int x, int y) {
    var hasInvalidArgument = (x < 0 || y < 0);

    if (hasInvalidArgument)
        return false;

    // more stuff below
}

You don't need to look at the expression... the variable name tell you what the test is