#366714 - 18/05/2016 18:13
C# if else if without braces
|
carpal tunnel
Registered: 20/12/1999
Posts: 31597
Loc: Seattle, WA
|
I have to follow the coding standards at my company, and they've got one which I disagree with, but which I must follow anyway. The standard is: 'If' statements which are followed by only one line of code: Don't use the curly braces. Yes, I know this is bad practice, and I know why, but the company has standardized on it and my code reviews get corrections if I don't do it their way. My question is what happens with nested if-else-if statements in situations like that. I tried googling on this, but the discussions I've found only ever talk about the basic case, and the philosophical reasons why this is a bad practice. I have yet to see an example that addresses this one particular case: Original, with braces, to show desired code execution flow:
// Report whether MSI uninstall was successful or not, based on the exit code of MSI;
// if we're not reporting errors, then don't say it's an error if the exit code is nonzero
if (0 == installResult)
{
Console.WriteLine("MSI Uninstalled.");
}
else
{
if (reportErrors)
{
Console.WriteLine("Error: Uninstallation failed. MSI Error code: {0}", installResult);
}
else
{
Console.WriteLine("MSI Exit Code: {0}", installResult);
}
}
With braces removed, to meet coding standards:
// Report whether MSI uninstall was successful or not, based on the exit code of MSI;
// if we're not reporting errors, then don't say it's an error if the exit code is nonzero
if (0 == installResult)
Console.WriteLine("MSI Uninstalled.");
else
if (reportErrors)
Console.WriteLine("Error: Uninstallation failed. MSI Error code: {0}", installResult);
else
Console.WriteLine("MSI Exit Code: {0}", installResult);
Will the second example function exactly like the first example? My concern is the last ELSE statement. When I google this, every discussion says things like "it goes up to the first semicolon it finds" and stuff like that, so it makes me wonder if that last ELSE is handled as expected. I'm pretty sure it will be handled correctly as expected. The syntax checker and compiler like it. Just wondering if anyone knows for sure. It probably would have taken me less time to just write a separate test program than to write this post on the BBS. Oh well....
|
|
Top
|
|
|
|
|
#366719 - 18/05/2016 19:58
Re: C# if else if without braces
[Re: tfabris]
|
carpal tunnel
Registered: 13/02/2002
Posts: 3212
Loc: Portland, OR
|
My question is what happens with nested if-else-if statements in situations like that. [...] Will the second example function exactly like the first example? My answer to that is "it's no longer a *trivial* single line of code, and should use braces." That nested if-else-if may have single *statements*, but it's not a single line of code, IMHO. Because C# uses "else if", as opposed to "elsif" or something like that, this is clearly in the territory of ambiguous code. My expectation is that unless the the parser considers whitespace a-la python, it will chunk the tokens as (if) (else if) (else), as opposed to (if) (else) { (if) (else) }. That said, they're logically equivalent. Cribbing a bit of symbolic logic symbols (it's been too long since I've done any formal logic for the following to really be proper, but whatever):
if (X) { } else if (Y) { } else { }
X v ¬X ^ Y v ¬X ^ ¬Y
vs.
if (X) { } else { if (Y) { } else { } }
X v ¬X ^ ( Y v ¬Y )
Using the distributive property, the latter, of course, simplifies to: which is identical to the other statement. edit: fixed symbols.
Edited by canuckInOR (18/05/2016 20:01)
|
|
Top
|
|
|
|
|
#366728 - 18/05/2016 22:46
Re: C# if else if without braces
[Re: canuckInOR]
|
carpal tunnel
Registered: 20/12/1999
Posts: 31597
Loc: Seattle, WA
|
Awesome! Thanks! I expected that, but wasn't sure.
|
|
Top
|
|
|
|
|
#366740 - 19/05/2016 07:33
Re: C# if else if without braces
[Re: tfabris]
|
carpal tunnel
Registered: 18/01/2000
Posts: 5683
Loc: London, UK
|
Yes, I know this is bad practice No it's not.
_________________________
-- roger
|
|
Top
|
|
|
|
|
#366741 - 19/05/2016 08:23
Re: C# if else if without braces
[Re: Roger]
|
carpal tunnel
Registered: 20/12/1999
Posts: 31597
Loc: Seattle, WA
|
I'm interested in hearing any reasons in favor of the practice, maybe it will help me feel less twitchy when I have to follow it.
I feel like it's bad practice for the same reason as everyone else on the internet says it is: it opens up a hole for a careless mistake if you are code refactoring or simply adding a second statement that you want to occur inside the "if". I've been bitten by this issue more than once, enough to get annoyed at the practice.
But I certainly would like to hear some positive benefits of it, other than that it's fewer lines of code and it's slightly cleaner to the eye.
|
|
Top
|
|
|
|
|
#366743 - 19/05/2016 09:58
Re: C# if else if without braces
[Re: tfabris]
|
carpal tunnel
Registered: 18/01/2000
Posts: 5683
Loc: London, UK
|
it opens up a hole for a careless mistake if you are code refactoring or simply adding a second statement that you want to occur inside the "if". Yeah, so don't do that. Add the extra braces when you need them. I've been bitten by this issue more than once I can honestly say I've never been bitten by this, and I wonder what's wrong with people who have (no offense!). it's fewer lines of code and it's slightly cleaner to the eye. That is the positive benefit. Don't dismiss it. Fewer lines of code means less cognitive load, and that's a good thing. That said, your second code sample would not have agreed with my personal coding style, for the reasons that canuckInOR gives: that last else-clause is no longer simple. I also have the rule that if the 'else' needs the braces, then so does the 'if'. This looks unbalanced, for example:
if (0 == installResult)
Console.WriteLine("MSI Uninstalled.");
else
{
if (reportErrors)
Console.WriteLine("Error: Uninstallation failed. MSI Error code: {0}", installResult);
else
Console.WriteLine("MSI Exit Code: {0}", installResult);
}
On the other hand, I have no problem with this:
if (installResult != 0)
{
if (reportErrors)
Console.WriteLine("Error: Uninstallation failed. MSI Error code: {0}", installResult);
else
Console.WriteLine("MSI Exit Code: {0}", installResult);
}
else
Console.WriteLine("MSI Uninstalled.");
I do have a problem with this: Just ick. Get a better compiler and write it naturally:
_________________________
-- roger
|
|
Top
|
|
|
|
|
#366744 - 19/05/2016 10:05
Re: C# if else if without braces
[Re: Roger]
|
carpal tunnel
Registered: 18/01/2000
Posts: 5683
Loc: London, UK
|
Oh, but all that said, I might write your example as this:
if (installResult == 0)
Console.WriteLine("MSI Uninstalled.");
else if (reportErrors)
Console.WriteLine("Error: Uninstallation failed. MSI Error code: {0}", installResult);
else
Console.WriteLine("MSI Exit Code: {0}", installResult);
that installResult and reportErrors are logically unrelated, so I wouldn't. This, on the other hand, is fine:
if (count == 0)
Console.WriteLine("You have no bananas");
else if (count == 1)
Console.WriteLine("You have one banana");
else
Console.WriteLine("You have {0} bananas", count);
...because the clauses are logically related.
_________________________
-- roger
|
|
Top
|
|
|
|
|
#366746 - 19/05/2016 11:52
Re: C# if else if without braces
[Re: Roger]
|
veteran
Registered: 25/04/2000
Posts: 1525
Loc: Arizona
|
[quote=tfabris] I've been bitten by this issue more than once I can honestly say I've never been bitten by this, and I wonder what's wrong with people who have (no offense!). It has bitten me more than once. Mainly because I was not formally trained in any languages that used curly brackets (FORTRAN and ADA). If an IF statement doesn't have curly braces, it looks natural and just add more to it. If an IF statement does have curly braces, they are there for a reason and add stuff between the braces. Of course, I am probably not the kind of person that would ever get hired with Tony's employer.
|
|
Top
|
|
|
|
|
#366750 - 19/05/2016 16:44
Re: C# if else if without braces
[Re: Roger]
|
carpal tunnel
Registered: 20/12/1999
Posts: 31597
Loc: Seattle, WA
|
I do have a problem with this: Just ick. Get a better compiler and write it naturally: I know what you mean. It feels weird and looks wrong. But that's another one of our coding standards that I must follow. I happen to agree with that one. Ironically, it's there to prevent a careless mistake (using a single equals sign accidentally, turning the comparison into an assignment). Thanks for the insights.
|
|
Top
|
|
|
|
|
#366754 - 19/05/2016 18:20
Re: C# if else if without braces
[Re: tfabris]
|
carpal tunnel
Registered: 18/01/2000
Posts: 5683
Loc: London, UK
|
using a single equals sign accidentally, turning the comparison into an assignment ...which can't happen with a modern compiler. A modern C++ compiler will require some extra parentheses to suppress the warning. The C# compiler won't allow it at all. Your coding standard has a large portion of cargo cult in it.
_________________________
-- roger
|
|
Top
|
|
|
|
|
#366755 - 19/05/2016 18:45
Re: C# if else if without braces
[Re: Roger]
|
carpal tunnel
Registered: 20/12/1999
Posts: 31597
Loc: Seattle, WA
|
...which can't happen with a modern compiler. A modern C++ compiler will require some extra parentheses to suppress the warning. The C# compiler won't allow it at all.
Your coding standard has a large portion of cargo cult in it. I don't deny it.  However I didn't realize that recent compilers prevented the mistake. That's cool! Is that only true of recent versions of Visual Studio? Because I could swear I had made that mistake myself within the last 5 years or so, and had adopted the reversed positioning ever since then, to prevent the problem. Maybe I'm remembering the mistake from a different language other than C/++/#, perhaps a scripting language where the mistake isn't protected against. I suppose that alone could be a good reason to adopt the practice: Some languages/compilers protect you against it, some don't. I have one other reason that the reversed positioning makes sense to me, which is related to writing unit tests. If I have an NUnit test and I have an assert like this in it: Assert.AreEqual(0, returnValue, "Installation failed."); Then the expected value is almost always the first parameter in NUnit assertions, which is usually a constant of some kind (in this case 0 meaning successful return value), and the tested value is almost always the second parameter. I've seen some unit tests where the test author got them backwards, and then the email that reports the test failure is backwards, saying something like "Installation failed. Expected: 1603, Actual: 0." So I'm always double-checking that in those kind of comparisons, the constant is first and the tested value is second. This positioning happens to map directly to the "backwards 'if' statement" style in my company's cargo-cult coding standards, so it helps my brain to be always thinking that way. Another "Cargo Cult" thing our company does is the thing with empty strings... Bad: string myString = ""; Good: string myString = string.Empty; There is some historical thing saying that there are technical reasons why the latter is better or more efficient for the compiler. But a while back I googled it, and the results seemed to tell me that the historical thing is no longer true and so this convention is no longer needed. But it's still in our coding standards and I still get corrected on it.
|
|
Top
|
|
|
|
|
#366756 - 19/05/2016 19:40
Re: C# if else if without braces
[Re: tfabris]
|
addict
Registered: 24/07/2002
Posts: 618
Loc: South London
|
I have a massive problem with K&R style bracing, I find it incredibly difficult to quickly read or look at the code, especially if you're playing around with nested levels, I prefer stuff lining up because when you remove indents you can clearly see what matches with what.
The "if (0==dataValue)" drives me potty looking at code as well, I understand the historical reasons for it, but, urgh, just icky.
While on the subject of irrational things, I cannot write a return like:
return myResult;
I always bracket it
return (myResult);
And switch statements I always put the cases in brackets....
Yeah, I'm weird.
|
|
Top
|
|
|
|
|
#366763 - 21/05/2016 20:04
Re: C# if else if without braces
[Re: tfabris]
|
carpal tunnel
Registered: 08/03/2000
Posts: 12338
Loc: Sterling, VA
|
I find it interesting that something in this thread prevents automatically skipping to the new posts...
_________________________
Matt
|
|
Top
|
|
|
|
|
#366764 - 21/05/2016 20:52
Re: C# if else if without braces
[Re: Dignan]
|
carpal tunnel
Registered: 10/06/1999
Posts: 5916
Loc: Wivenhoe, Essex, UK
|
Thought that was just happening to me 
_________________________
Remind me to change my signature to something more interesting someday
|
|
Top
|
|
|
|
|
#366766 - 22/05/2016 01:31
Re: C# if else if without braces
[Re: Dignan]
|
carpal tunnel
Registered: 20/12/1999
Posts: 31597
Loc: Seattle, WA
|
I find it interesting that something in this thread prevents automatically skipping to the new posts... I wonder if it's the octothorpe in the title?
|
|
Top
|
|
|
|
|
#366767 - 22/05/2016 09:06
Re: C# if else if without braces
[Re: tfabris]
|
carpal tunnel
Registered: 10/06/1999
Posts: 5916
Loc: Wivenhoe, Essex, UK
|
Well spotted, someone missed an escape/encode.
_________________________
Remind me to change my signature to something more interesting someday
|
|
Top
|
|
|
|
|
#366768 - 22/05/2016 15:08
Re: C# if else if without braces
[Re: tfabris]
|
old hand
Registered: 27/02/2003
Posts: 777
Loc: Washington, DC metro
|
I, too, am having trouble with new post.
Did someone delete a post?
-jk
|
|
Top
|
|
|
|
|
#366769 - 22/05/2016 18:01
Re: C# if else if without braces
[Re: jmwking]
|
carpal tunnel
Registered: 10/06/1999
Posts: 5916
Loc: Wivenhoe, Essex, UK
|
It is just this thread that is a problem. There is a # in the thread title. The # symbol is used in URLs to indicate where within a page to link to.
The BBS uses that to link to the first new posts within threads when viewing in flat mode. The BBS clearly doesn't do quite the right thing when constructing this links to the new posts. It should do a thing called encoding, that replaces special characters like # that have special meanings.
Because it doesn't do it correctly the link to the first new post for this thread doesn't work.
_________________________
Remind me to change my signature to something more interesting someday
|
|
Top
|
|
|
|
|
#366771 - 22/05/2016 19:32
Re: C# if else if without braces
[Re: tfabris]
|
carpal tunnel
Registered: 10/06/1999
Posts: 5916
Loc: Wivenhoe, Essex, UK
|
Someone should do many things, like cut the hedges for the start...
_________________________
Remind me to change my signature to something more interesting someday
|
|
Top
|
|
|
|
|
|
Previous Topic
Index