4

I came across the following in some sample code:

if (urlStr)
{
    NSImage *iconImage = [[NSWorkspace sharedWorkspace] iconForFile:urlStr];
    node.nodeIcon = iconImage;
}
else
{
    // it's a separator, don't bother with the icon
}

Is this style of using an else statement only for a comment good practice for production or it is only useful for code used as an example?

  • 1
    I would be terribly surprised if anyone can come up with an answer that's backed by some concrete data or scenario. This appears to be entirely opinion based to me. – RubberDuck Apr 28 '16 at 3:11
  • 3
    It is useful for explaining why something was, or in this case wasn't, done. I do try to make my code as readable as possible, but that doesn't always convey the reasoning behind an implementation. – Adam Zuckerman Apr 28 '16 at 3:15
  • 1
    As a general rule code is supposed to be self explainatory, if it needs commenting that code ain't as clear as it is supposed to be. The answer that @David Packer provided makes the most sense to me. Just create a method that is called IsSeparator and put that into the if statement, this way it is perfectly clear that if it's not separator it needs an icon and you don't need to have extra code just to make clear what you're trying to achieve. – Zalomon Apr 28 '16 at 7:33
  • 1
    @Zalomon, what if you're implementing a complicated but well documented and established math formula and put a comment with a link to the theory behind the magic formula? Or, more to the point of this example, what's the reasoning for doing nothing in the else block? It might make sense to even have one or two lines of comment explaining the weird corner case and why it's different than you would expect. To say "all comments can be replaced with well-named variables" seems overly simplistic. – alexanderbird Apr 28 '16 at 7:43
  • 1
    As I said, it is a general rule not a law and I think it applies to this specific question. I think that you should get rid of the else statement, it does nothing so there's no point on keeping it; if you want to add a comment to clarify what you're trying to do with a comment you can just make it the other way around: add a comment explaining why when the condition in the if is met you do what you do. Like //if it's not a separator add an icon. – Zalomon Apr 28 '16 at 7:51
6

For the programmer reading it, I think it makes it quite clear. I am used to reading "if condition do stuff else do other stuff", so I can parse that pretty easily. The else block seems like a really logical spot for a comment explaining why we don't want to do anything in that condition.

But, will it make a significant impact if we have the empty block there? Is improved clarity for humans worth the cost? I think that's a language-specific (or rather, compiler/interpreter specific) question.

The only time I can think that this could have some negative impact is if it's in a bottleneck that needs to be ultra-optimized.

Even in those cases, it might make no difference. Here's an example of how I would check, for C.

sometimes it makes no difference to the compiled code

short.c

int main() {
  if(1) {
    printf("Hello World");
  }
}

long.c

int main() {
  if(1) {
    printf("Hello World");
  } else {
    // not possible
  }
}

Then I compiled both with the -S option to compile without assembling. I compared the assembly files and they were identical.

sometimes it does make a difference to the compiled code

I then updated the files and replaced if(1) with int foo = 1; if(foo) and repeated the process. (I know, the block is still not reachable - so if the compiler still treats it as unreachable I have proved nothing, but if the compiler treats it as reachable then I know I have successfully tricked the compiler and I have proven my point.) In this case, the compiled files were not identical - the compiler did not ignore the empty else block when it thought it was reachable.

short.s

  ## call to printf, etc. (i.e. the if block)...
LBB0_2:
  ## rest of code...

long.s

  ## call to printf, etc. (i.e. the if block)...
  jmp LBB0_3 ## ! there is always a jump statement
LBB0_2:
  jmp LBB0_3
LBB0_3:
  ## rest of code...

So in this almost reachable version, when the empty else block is included, there is an extra jump statement at the end of the if block. So if performance is a key concern here, then adding the empty else block is not a good idea.

conclusion

Based on the language, the else block could change the compiled code and affect performance. You would have to check for whichever language you're using, and then if it does have a performance hit in your language, decide if performance is more important than readability in this section of code.

  • 6
    Unless this code is called billions of times inside a loop of a real-time video game, one extra instruction is completely meaningless for performance. -1 – user949300 Apr 28 '16 at 4:16
  • 6
    @user949300, that's exactly my point - there is some real but probably insignificant performance hit. So when you ask yourself "can I put an empty else here as documentation?" the answer is "Yes, unless this is a bottleneck where every instruction counts. So probably yes." I'm not advocating for premature optimization, just being aware that there might actually be some small cost. – alexanderbird Apr 28 '16 at 4:34
  • Almost all of your answer is about performance, which we all agree is not an issue. The correct answer should address what is more clear, more readable, etc. – user949300 Apr 29 '16 at 0:02
  • Your performance argument makes no sense, as you are compiling without optimizations. Skipping the else block is a trivial optimization for any compiler. – MartinHaTh May 28 '16 at 8:56
3

No, having an else clause just to make a comment is generally not a good idea, it polutes the code with unnecessary lines and slows the programmer reading it.

When a programmer reads the code his thought proces may be: Look, there's an else clause, so when the urlStr is not set, something else mig... oh, nevermind, it's just a placeholder.

By changing the code to:

bool isSeparator(string urlStr)
{
    return urlStr != null;
}

if (!isSeparator(urlStr))
{
    NSImage *iconImage = [[NSWorkspace sharedWorkspace] iconForFile:urlStr];
    node.nodeIcon = iconImage;
}

You achieve the very same thing without needing the else block and the comment.

  • 2
    Yeah, saved an else block, added a function. The function itself is very confusing: Why would be a null url string be a separator? That's nonsense. It's very specific to the situation and in general null url strings are not separators. So that needs commenting. – gnasher729 Apr 28 '16 at 7:53
  • 1
    @gnasher729 The body of the method is not inportant, in this specific case that was the rule presented by op. – Andy Apr 28 '16 at 8:40
3

No, it is very bad idea. It makes your code more complex without any benefit. If you want to explicitly state the else condition do it with a comment above the if :

// When the urlStr is false then it is a separator and no action is needed on the icon
if (urlStr)
{
    NSImage *iconImage = [[NSWorkspace sharedWorkspace] iconForFile:urlStr];
    node.nodeIcon = iconImage;
}
  • 1
    You're commenting on the negative case which is misleading. Comment on the code if you need to but don't comment code that isn't there... – Robbie Dee Apr 28 '16 at 11:06
  • 1
    @RobbieDee you can phrase the comment differently i.e. // Do action on icon ONLY if it is not a separator – DesignerAnalyst Apr 28 '16 at 12:56
0

It can sometimes make sense when you want to document that you thought about there being an else-case but intentionally want to point out that it is fine to do nothing in it.

if (file.open()) {
   readStuffFrom(file);
} else {
   // file not found - but we can ignore this for now because REASONS
}
  • 1
    Code should be self-documenting. Without your else branch, there is no code there, so it cannot be self documenting. I wouldn't have a clue whether it's Ok that the file couldn't be open, or whether you were just too lazy to handle that case. The comment makes it clear and is needed because without it, there is no code and it's not self-documenting. Thanks for the moment. – gnasher729 Apr 28 '16 at 10:56
-1

Though I wouldn't vote against the else case in someone else's code, personally I would prefer the following form with the rationale to avoid the additional no-op code:

if (urlStr)
{
    // it's not a separator, so we set an icon
    NSImage *iconImage = [[NSWorkspace sharedWorkspace] iconForFile:urlStr];
    node.nodeIcon = iconImage;
}
-4

Counter-question that won't work well as a comment: is there a compelling reason why you can't write this?

if (urlStr)
{
    NSImage *iconImage = [[NSWorkspace sharedWorkspace] iconForFile:urlStr];
    node.nodeIcon = iconImage;
}
// else it's a separator, don't bother with the icon
  • 5
    That comment could seem to be about the next line below it. – RemcoGerlich Apr 28 '16 at 7:16
  • 5
    Is there a compelling reason why this would be better? Are you afraid that the compiler will create code for the else? If that is a case, 1. stop micro optimisations. 2. get a new compiler. Or 3. Check that your fears are unjustified and learn something new. – gnasher729 Apr 28 '16 at 7:54
  • @gnasher729 This avoids the need to suppress the lint warning about an empty else statement. Disabling that warning may be undesirable if you're concerned about accidentally empty else statements. – Kevin Krumwiede Apr 28 '16 at 18:32

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.