5

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?

Steve Moser
  • 287
  • 1
  • 6
  • 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 03:11
  • 4
    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 03:15
  • 2
    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 07: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 07:43
  • @Zalomon: So instead of a comment you are bloating up your code by creating a completely pointless method? So you create complicated extra code to make clear what you're trying to achieve so you don't have to write some very simple code to make clear what you're trying to achieve? Are you being paid by the line of code you write? – gnasher729 Apr 28 '16 at 07:49
  • 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 07:51
  • @gnasher729, no I'm paid for making code that is readable and executable and avoids repetition. Going a little beyond the scope of the question It is likely, for instance, that he needs to check if the url is a separator in some other place. If so is easier to create a single line method that says exactly what is that you're trying to do and test in every if statement. So yes, I stand by what I said as best practice. – Zalomon Apr 28 '16 at 07:56
  • @Zalomon, how is removing the comment-only-else more readable, more executable, or less repetitious? I would argue that it's less readable, "more executable" is not meaningful, and it seems neither the OP's example nor any of the suggestions are any more or less repetitious. Adding a function does seem more verbose, though, and [less code is better](http://blog.codinghorror.com/the-best-code-is-no-code-at-all/) – alexanderbird Apr 28 '16 at 08:51
  • Voting to close this, as the answers are entirely opinion-based. – David Arno Apr 28 '16 at 08:53
  • @alexanderbird there's no such think as more executable, code is either executable or not; comments are not executable. And if you think is less readable is fine with me, wouldn't a comment like '//if it's not a separator add an icon' before the if more readable than else just for a comment? – Zalomon Apr 28 '16 at 09:00
  • @Zalomon yes, I was joking with that one ;) As for which is more readable, I can't think of an objective way of determining that. Obviously you and I disagree with each other about what is more readable, but our opinions alone aren't worth much in this discussion. I think we could all agree that the performance hit is almost certainly insignificant, that you could measure the performance hit if you cared, that you should choose whichever option is most readable, and that we don't know how to measure readability. – alexanderbird Apr 28 '16 at 09:07
  • Try to change name `urlStr` to something that would show that it is not separator. Maybe `nodeUrl` or `imageUrl`. I cannot suggest a definite name, because I don't know code around this part. Also you could just add a flag `bool isSeparator = urlStr == null` and use it inside `if`. My main point - try to use less comment, but more good names in code. – user2216 Apr 28 '16 at 10:36

6 Answers6

8

Edit: my answer got some criticism in the comments, and I'm pretty sure the commentators are right and my answer is probably mostly nonsense.

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

Your performance argument makes no sense, as you are compiling without optimizations. Skipping the else block is a trivial optimization for any compiler -- MartinHaTh

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.

alexanderbird
  • 554
  • 3
  • 12
  • 7
    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 04:16
  • 7
    @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 04:34
  • 1
    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 00:02
  • 2
    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 08:56
4

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.

Andy
  • 10,238
  • 4
  • 25
  • 50
  • 3
    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 07:53
  • 2
    @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 08:40
  • I agree with making the function. Maybe it just looks odd to @gnasher729 because of the string argument. It should probably be an instance method with no arguments and the same definition. – Jeremy Hunt Mar 01 '22 at 02:12
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;
}
DesignerAnalyst
  • 449
  • 2
  • 6
  • 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
}
Philipp
  • 23,166
  • 6
  • 61
  • 67
  • 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;
}
Murphy
  • 821
  • 6
  • 15
-3

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
Kevin Krumwiede
  • 2,586
  • 1
  • 15
  • 19
  • 5
    That comment could seem to be about the next line below it. – RemcoGerlich Apr 28 '16 at 07:16
  • 6
    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 07: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
  • In that case lint is quite misguided. My compiler warns me if I write “if(condition);” or “for(I=0;I – gnasher729 Feb 28 '22 at 19:08
  • @gnasher729 My point is that there's no reason to write an empty else block to contain a comment when you can just write a comment. I'm not afraid the compiler is going to generate anything. Lint is misguided if it *doesn't* issue a warning about the OP's empty else block, because even if the compiler elides it, its presence suggests that someone forgot to write more than a comment in it. And I would hope that lint would behave exactly the same for `if(condition);` and `if(condition) ;` as well. – Kevin Krumwiede Feb 28 '22 at 23:32