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.

  • 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 4 '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 4 '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 8 '13 at 9: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 8 '13 at 10:23
10

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.

  • 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 4 '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 4 '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 4 '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 5 '13 at 4:46
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;
}

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.