Inspection suggestion

I was wondering whether it would be worth submitting a Jira
issue for this:

Given the code:

if (condA) {
doA();
} else if (condB) {
doB();
} if (condC) {
doC();
} else {
doD();
}

warn that the third 'if' should probably be an 'else if".

What do you think?

Vince.


12 comments
Comment actions Permalink


The only reason to think it should be an "else if" is formatting, yes? So far I've resisted inspections that depend on formatting, because there are so many ideosyncratic styles and IDEA makes it so easy to change them. That said, this is a pretty good one. Yes, please submit a JIRA item.

--Dave Griffith

0
Comment actions Permalink

Vincent

>Given the code:
>

if (condA) {
doA();
} else if (condB) {
doB();
} if (condC) {
doC();
} else {
doD();
}

>

>

Problem: condB and condC are true, at the same time => replacing the 2nd
if by an else if would change the behaviour.

Alain

0
Comment actions Permalink

typo correction:

Problem: condB and condC are true, at the same time => replacing the
2nd if by an else if would change the behaviour.

>
Should read:

Problem: if condB and condC are true at the same time, then replacing
the 2nd if by an else if would change the behaviour.


Alain

0
Comment actions Permalink

Dave

>The only reason to think it should be an "else if" is formatting, yes?
>


If your next step is to replace a if-elseif sequence by a switch, you'd
better have the longest sequence first.


Alain

0
Comment actions Permalink

http://www.jetbrains.net/jira/browse/IDEA-2222

It could rely on formatting (I know, that's a good way to create
false positives) perhaps looking at the configuration for
"special else if treatment".

It could also look at the construct:
1. if/elseif/.../elseif without final else
2. immediately followed by if/elseif/else or if/else.

Vince.


0
Comment actions Permalink

That's the idea :) Change the original buggy behavior with
the probably-intended one.

I see it kind of like the fall-through-switch inspection.
switch()

Adding a break at end of case 2 will change the behavior. But
most likely the current behavior was not intended and having the
inspection helps tremendously in finding these problems quickly.

Vince.


0
Comment actions Permalink

Vincent

>That's the idea :) Change the original buggy behavior with
>the probably-intended one.
>

>

I fear - no stats to backup the feeling, though - that it would find a
lot of false positives, like :


if (doubleInitNeeded())
doubleInit();
else
simpleInit();
if (doubleBodyNeeded())
doubleBody();
else
simpleBody();



Alain

0
Comment actions Permalink

This wouldn't be flagged. This is a regular if-else followed by
if-else

We're talking if-elseif followed by if-elseif or if-else

if (simpleInitNeeded())
simpleInit();
else if (doubleInitNeeded())
doubleInit();
if (tripleInitNeeded())
tripleInit();
else
quadrupleInit();

(Man do I miss those braces! :) )

Vince.


0
Comment actions Permalink


What about if/else if followed by a simple if (without an else)?

Wierdly, I think this may be less prone to false positives if I consider formatting. That's not a good sign.

--Dave Griffith

0
Comment actions Permalink

There are a few simple 100% safe cases, like:

if (cond1) {
...
return 1;
}
if (cond2) {
return 2;
}
else if (cond3) {
throw new MyException();
}
if (cond4) {
System.exit();
}

0
Comment actions Permalink

Interesting. I would see that one as already covered
by the 'redundant else' inspection and would use the quickfix
to 'drop the redundant else' :)

Vince.


0
Comment actions Permalink

Vincent Mallet wrote:

>Interesting. I would see that one as already covered
>by the 'redundant else' inspection and would use the quickfix
>to 'drop the redundant else' :)
>

>

Well, no: the 3 ifs can be merged into one big if-else fi sequence, in
one shot.

Alain

0

Please sign in to leave a comment.