if(x!=y) vs if(x==y)

Question

I have run the PMD plugin in Eclipse against my code and I'm getting a high priority warning for code similar to the one shown below:

 if(singleRequest !=null){
   // do my work
 }else{
   // do my other work
 }

PMD says `Avoid if (x != y) ..; else ..;

And the description of the error looks like this:

In an "if" expression with an "else" clause, avoid negation in
the test.  For example, rephrase:
if (x != y) diff(); else same();
as:
if (x == y) same(); else diff();
Most "if (x != y)" cases without an "else" are often return

but I still can't understand the impact on my code. If someone could guide me with an example, I would appreciate it.

Answer 1

A number of PMD rules are more style opinions than correctness alerts. If you don't agree with this rule or the rule doesn't match your project's coding standards, you could consider suppressing warnings or even configuring PMD to enforce only the rules you like

Answer 2

PMD is a tool. PMD works based on heuristics. Someone decided upon this heuristic; that negative conditionals with else statements are not "good style".

However, in this case, as I have argued in my comments, the code posted is how I would write it. (In particular with x != null, but not exclusively to this construct.)

This is because I don't look at the conditional (excepting as it can be simplified; e.g. removing double-negatives as shown by Jim Kin) but rather I look at the logic of the branches or "flow".

That is, I place the positive branch first. In this case I contend that

if (x != null) {
  doValid         // positive branch
} else {
  doFallback
}

is semantically equivalent to

if (isValid(x)) { // it looks like a "positive conditional" now
  doValid         // still positive branch
} else {
  doFallback
}

and is thus positive branch first.

Of course, not all situations have such a "clear" positive flow, and some expressions might be expressed much easier in a negative manner. In these cases I will "invert" the branches - similar to what PMD is suggesting - usually with a comment stating the action at the top of the block if the positive branch/flow was reversed.

Another factor that may influence the conditional choice used is "immediate scope exiting" branches like:

if (x == null) {
  // return, break, or
  throw new Exception("oops!");
} else {
  // But in this case, the else is silly
  // and should be removed for clarity (IMOHO) which,
  // if done, avoids the PMD warning entirely
} 

This is how I consistently (a few occasional exceptions aside) write my code: if (x != null) { .. }. Use the tools available; and make them work for you. See Steven's answer for how PMD can be configured to a more suitable "taste" here.

Answer 3

It's a readability issue. Consider

if ( x != y ) 
{
}
else  // "if x doesn't not equal y"
{
}

vs.

if ( x == y )
{
}
else  // "if x doesn't equal y"
{
}

The latter example is more immediately identifiable. Mind you, I see nothing wrong with using negatives... it can make a lot more sense, consider

if ( x != null )...

Answer 4

The only reason I would avoid using the negative-case is if it resulted in double-negatives, which might be confusing.

e.g.

if (!checkbox.disabled) {
    // checkbox is enabled
} 
else {
    // checkbox is disabled
}

Answer 5

Who reads your code? You do. The compiler does. Or maybe the assistant of the lecturer. A co-worker, who can't make difference between == and != ? Hope not.

I can only think negatives being bad in complex expressions. (Context being: at least for me. I know I've frustrated in debugging in my head while(!expr && !expr2 || expr3) { })

ch=getch(); if (ch!='a') is a pattern that is easily extended to
if (ch!='a' || ch!='b') which is always true, while sounding semantically correct.

From performance standpoint, it's best to sort the probabilities.

if (more_probable) {
      ....
      unconditional_jump_to_end_of_block;
} else {
   ...
}

This choice should lead to better performance, as the there is no mis-prediction penalty in the more probable branch.

if (p && p->next) evaluated from performance standpoint gives poor results.

Answer 6

You have to avoid having "not equals" in the if condition. This is because when someone else looks at your code, there is a real possibility that the person might ignore the != and might jump to wrong conclusion about the logic of your program.

For your case, you may have to interchange the if logic with else logic and change != to ==

Answer 7

It's a balancing case of code readability vs. code organization. The warning is basically suggesting that it's confusing for people reading the code to navigate the negation of a negative.

My personal rule of thumb is, whatever you expect to be the "normal" case is what you should test for in the if. Consider:

 if (x != y) {
   // do work here...
 } else {
   throw new IllegalArgumentException();
 }

In this situation I'd say that the important work is being done in the x != y case, so that's what you should test for. This is because I like to organize code so that the important work comes first, followed by handling for exceptional cases.

Answer 8

It's because "good style" says that if possible tests should be "positive", so:

if (singleRequest == null){
   // do my other work
} else {
   // do my work
}

Is easier to read because the test is "positive" (ie "equals" not "not equals"), and ultimately better readability leads to less bugs.

Edited

This is particularly the case with test like:

if (!str.equals("foo")) {

you can easily miss the ! at the front, but if you make the test positive, it's a lot cleaner.

The only time you should have a negative test is when there's no else block - then a negative test is unavoidable unless you have an empty true block, which itself is considered a style problem.

Answer 9

Not really an answer, but you can minimise the overall complexity and improve readability by returning or failing early and then continuing without indentation:

if (something == null) {
    throw new IllegalArgumentException("something must not be null");
}

// continue here