[java] ConfusingTernary should treat != null as positive condition

[sebbASF]

Rule Set: ConfusingTernary

Description:

ConfusingTernary should treat negative comparisons with null as a positive condtion.

The condition "!= null" is often used to mean "the item is present" rather than "the item is not absent".

Rewriting using "== null" would make the code much harder to follow.

Code Sample demonstrating the issue:

if (parser.getArgumentById(VERSION_OPT) != null) {
...
} else if (parser.getArgumentById(HELP_OPT) != null) {
...
} else if (parser.getArgumentById(OPTIONS_OPT) != null) {
...
} else if (parser.getArgumentById(SERVER_OPT) != null) {
...

Running PMD through: Ant

[jsotuyod]

@sebbASF thanks for the report!

I think there are a bunch of things to think about on this most interesting report.

Firstly, regardless of all else, the rule is certainly just introducing noise. The rule attempts to avoid "inverted" conditionals such as "if NOT A then B else C", but on this scenario, all checks are for different things altogether. The rule should not apply to this block.

Right now, there is a ignoreElseIf property for the rule, to have it ignore all else if. This would remove this warning, but will certainly be broader than expected.

Then it's the point of considering "!=" a positive. That would allow to write code such as:

if (x != null) {
  return x.foo();
} else {
  return 0;
}

which is the very kind of code this rule attempts to prevent.

What I guess should happen is:

• the rule should be smart enough to check if else if is concerning the same value, and only apply if so
• the ignoreElseIf is probably redundant in such scenario, analyze removing it

[sebbASF]

My example happened to use the same variable, but there are other cases such as a hierarchy of defaults where the conditions need to check for the first non-null variable.

My point is that null is special here, because the natural check for non-null uses inequality.

[SUPERCILEX]

I'm having the same issue:

if (deepLink.getQueryParameter(TEAM_QUERY_KEY) != null) { // PMD shouldn't report this as an error
    launchTeam(getTeam(deepLink).get(0));
} else if (deepLink.toString().equals(ADD_SCOUT_INTENT)) {
    NewTeamDialog.show(mActivity.getSupportFragmentManager());
}

[jsotuyod]

@SUPERCILEX yeap, similar case, you are analyzing different values on the ifs (all related to deepLink, but different all the same), the rule should be smart enough to know that and assume therefore order is important and not report as violation.

[ThrawnCA]

I think that any usage of else if should be enough to disable the rule. As soon as it's more complex than the binary case, there may be good reasons for writing it with the negative first.

In other words, I think ignoreElseIf should default to true.

[oowekyala]

@jsotuyod

Then it's the point of considering "!=" a positive.

I think the point is only to consider != null as a positive. The rule will still catch confusing conditionals like

if (x != 2) {
  //...
} else {
  //...
}

But null checks will be ignored because it's (arguably) as easy to read if (x != null) as to read if (x == null).

[sebbASF]

Yes; see the title and initial comment of this issue.

[alsingh287]

Any updates on this ?

[sabberworm]

I would also agree that a null check is a special case that needs to be treated differently.

The following structure:

if something is null then
  do the fallback
else
  do the thing with something

is much more confusing to read than

if something isn’t null then
  do the thing with something
else
  do the fallback

Currently I’m using the following workaround:

if (Objects.nonNull(something)) {
  // do the thing with something
} else {
  // do the fallback
}

However, this might have some runtime overhead (I’m not sure if the compiler inlines it, though).

[JoyChopra1298]

Is this being considered? Any update?