6

I have an if/else if structure that on some cases does nothing. From the code I have seen in my career, the "empty" case is normally left out. But when I wrote my code, it just doesn't feel right to leave this case out, but to show it as a real case that simply does nothing. For example, in a case where something is done when a number is below 5 and above 10, but in between nothing is done:

int a = 4
if(a < 5) {
  do something
} else if(a >=5 && a <= 10) {
  // do nothing
} else if(a > 10) {
  do something else
}

The reason I thought this is a better option is because:

  1. The is how I though about the problem in my mind.
  2. This shows the reader of the code that I thought of all the possibilities and didn't forget one by mistake.

So I was wondering if this convention is either accepted by the programming community or it is shunned upon.

gnat
  • 21,442
  • 29
  • 112
  • 288
vainolo
  • 1,321
  • 3
  • 13
  • 22
  • possible duplicate of [Elegant ways to handle if(if else) else](http://programmers.stackexchange.com/questions/122485/elegant-ways-to-handle-ifif-else-else) and of [Are too many if-else statements for validation bad?](http://programmers.stackexchange.com/questions/209822/are-too-many-if-else-statements-for-validation-bad) – gnat Oct 04 '13 at 11:27
  • In your example it's ok, but in other cases where a is not an int but for instance a property, the mere checking of a multiple times can have sides effects. – Pieter B Oct 04 '13 at 11:29
  • What about if a is less than or equal to zero? Is this supposed to only handle positive numbers? – eidsonator Oct 04 '13 at 15:44
  • @gnat: Your 'duplicate detector' seems to have malfunctioned here. The only relation between this Q and the two you mentioned as possible duplicates is that they concern if-statements. – Bart van Ingen Schenau Oct 08 '13 at 09:49
  • @BartvanIngenSchenau I believe these are dupes. And it's interesting how "conveniently" you omit else-statements in your analysis of the questions – gnat Oct 08 '13 at 10:23
  • @gnat: Fair enough that you believe them to be duplicates. I don't, but lets agree to disagree on that one. With regard to not mentioning "else-statements", for me the `else` is part and parcel of the `if`. An if-statement may or may not contain an else part (or even an elif part), but I don't consider those to be separate statements. – Bart van Ingen Schenau Oct 08 '13 at 11:23
  • I usually "cheat" in this case by putting in a log statement: 'else if (a >=5 && a <= 10) {LOG.debug("Nothing to do, because ...")}'. Keeps the compiler and checkstyle happy *AND* provides a hint to future maintainers that you actually thought about it. – firtydank Feb 15 '16 at 09:24

2 Answers2

11

I would avoid to define the do nothing case explicit by giving the intervall. I would use an else-branch for that

int a = 4
if(a < 5) {
  do something
} else if(a > 10) {
  do something else
} else {
     //This should mean: (a >=5 && a <= 10)
     // do nothing

     // Maybe log something here 
     // or even add an assertion while development to ensure that the 
     // branch is only reached in expected cases:
     assert (a >=5 && a <= 10) : "I expected 'a' to be 5<=a<=10, but a is:"+a;
}
MrSmith42
  • 1,041
  • 7
  • 12
11

You are not simply checking the value of a for the sake of it. It probably means something.

Thus I would prefer to see:

Boolean sell = a < 5;
Boolean buy = a > 10;
Boolean error = a >= 5 & a <= 10;
// or even: 
Boolean error = !sell & !buy;
/* ^ (that's single source principle: if 5 changes to 6 one day, 
 * you don't have to remember to change it in two places) */

And then:

if (sell) {

} else if (buy) {

} else if (error) {

}

Isn't it clearer?

Furthermore, could a be both less than 5 and greater than 10??

Rather not, so those elses are clearly redundant. (Unless you change the value of a within one of your if blocks, mind you, If you only evaluate a once, however - as in my example - you don't have to worry about it).

Hence:

Boolean sell = a < 5;
Boolean buy = a > 10;
Boolean error = !sell & !buy;

if (sell) {

} 
if (buy) {

} 
if (error) {
    // handle error
}

This is way clearer and more flexible in my opinion - if some conditions are no longer mutually exclusive, as it can happen, you won't need to do much refactoring at all.

Konrad Morawski
  • 9,721
  • 4
  • 37
  • 58
  • Very interesting idea. Only thing I don't like about it is that it makes the code longer, which makes it "harder" to read. But otherwise it is very elegant. – vainolo Oct 04 '13 at 12:09
  • 3
    @vainolo yes it's longer by three lines, but I think it actually improves the readability (since your conditions are given meaningful names) and so the trade off is worth it. Of course ultimately it comes down to taste – Konrad Morawski Oct 04 '13 at 12:17
  • 4
    When the difference in efficiency is negligible as in this case, readability is most important. Simplifying conditionals by extracting and naming their components is a great way to improve readability. – Mike Partridge Oct 04 '13 at 12:29
  • 1
    I like a series of if-else-if-else and virtually always have an else at the end. Similarly for a switch-statement, I virtually always have a default case. I think that it's just a good idea to always consider every case, and if a case is "impossible" then have an assert statement there (if not throw an exception), and to be sure to turn on assertions for debugging and for testing. – Kaydell Oct 05 '13 at 04:46