Braces Around Blocks
Braces Around BlocksFriendly reminder: ConvertSpacesToTabsNotForCode -- TheMgmt?
The list of BadCodingStandards includes:
which led to the following discussion:
I use {}s after all ifs, fors, and whiles, but that's just because I'm not bright enough to accurately read code without them.
And I use them because they enable easy insertion of extra lines, like debug code ... just try adding a printf when there are no braces -- NissimHadar
A: Indent consistently. (3 or 4 spaces per indent is also good.)
This doesn't help when you have a DanglingElse? ambiguity. Or if you use K&R style indents, since you don't always look for the open brace (I know I don't, since it's always there. Always).
And for my way of coding, I will often put a null block for the then part of an if and write the else first. Going back and inserting braces is more annoying than having them there in the first place. -- AlastairBridgewater
A good programmer's editor (e.g. SlickEdit) will insert braces automatically after constructs that permit them. With such tools, it's more work not to use braces than it is to use them, so you will only be removing them when they really get in the way of readability, and let them stand otherwise. -- LaurentBossavit
Requiring blocks (begin/end, '{'/'}', etc) is better justified when there are comments and a line of code: If someone adds another line of code, they may forget to add the necessary block.
Example:
if (this condition) // There's a good reason here to "do the // right thing. And for some reason, we // feel the need to tell you all about it. // (Blah, blah, blah...) DoTheRightThing();can easily be changed to the following, during maintenance:
if (this condition) // There's a good reason here to "do the // right thing. And for some reason, we // feel the need to tell you all about it. // (Blah, blah, blah...) DoTheRightThing(); AndDoThisToo();As you can see, the indentation does not match the actual meaning and execution of the code. It can be real confusing when you do this accidentally. (This is a reason to block everything. It may not be a good reason, but it's a commonly given reason. ;-) -- JeffGrigg
However, it is still a pretty lame excuse for people not knowing how to do their job right. And, IMO, a pretty lame way to code, putting comments in that actually disrupt the flow of reading the code. -- SomeoneElse
Get an editor that auto-indents assertively enough that you can't easily write that!
Auto-indent isn't as effective as it sounds when every coder in the group uses a different editor. And a slightly different coding/indentation style. I guess this is the motivation for a coding standard. -- BrianStPierre
JeffGrigg, In your example you don't really need a block. You can see that you have no closing brace after DoTheRightThing() with a matching ident level, so you know that there are no opening brace too. -- Anonymous
CodeComprehension is important, but for me the reason to block everything that spans lines is not to make it easier to add code later, but to make it hard to add really ugly, really subtle, run-time bugs later.
My experience with the practice of indenting the single-statement on the next line has been that:
I second the first four of those five bullets. I actually only got about one bug a year of that type before starting to always use the braces, but that was still a day or two of frustrating debugging time per year. In the four or five years since I started using the braces religiously, I haven't had any bugs of that type, at a cost of perhaps an hour or two a year typing extra braces - a good tradeoff.
The braces need not force the code to occupy more lines. I've found the following works just fine:
if (condition)
{return a;}
else
{return b;}
- Warren Dew
A block-structured language can be fairly well defined as being one where:
<statement>::=<block start delimiter><statement>[0 or more]<block end delimiter>Maybe the defrauding is the statements trying to pretend to be blocks.! -- GeneWirchenko
DefensiveProgramming tactics do have a cost - extra code, extra stuff to read. With a good unit test that is likely to catch the bug, it's hard to justify the cost.
A whole 4 characters per if, for, and while. At least it's not as bad as the person who won't use structs for the same reason, but IMNSHO it's still a poor justification. And for some it's easier to read if it has the braces. I'm inclined to leave this to personal preference, and not have this on either BadCodingStandards or GoodCodingStandards? (or if it's on one, it should be on the other). -- AlastairBridgewater
Contrast this:
private int getMidfixGroup (int midfix)
{
boolean isEven = (midfix %2 == 0);
if (isEven)
{
if (midfix >=10)
{
return 2;
}
else
{
return 3;
}
}
else
{
if (midfix >=10 )
{
return 4;
}
else
{
return 1;
}
}
}
with this:
private int getMidfixGroup (int midfix)
{
boolean isEven = (midfix %2 == 0);
if (isEven)
if (midfix >=10)
return 2;
else
return 3;
else
if (midfix >=10 )
return 4;
else
return 1;
}
Which is easier to read?
private int getMidfixGroup (int midfix)
{
boolean isEven = !(midfix & 1);
if (isEven) {
if (midfix >=10) {
return 2;
} else {
return 3;
}
} else {
if (midfix >=10) {
return 4;
} else {
return 1;
}
}
}
And the two if (midfix >= 10) should be the outer if statement (OnceAndOnlyOnce). -- AlastairBridgewater
Or you can do this:
private int getMidfixGroup (int midfix)
{boolean isEven = !(midfix & 1);
if (isEven)
{if (midfix >=10)
{return 2;
}
else
{return 3;
}
}
else
{if (midfix >=10)
{return 4;
}
else
{return 1;
}
}
}
If you like the braces lined up. It's probably best to define a style that the whole team uses although I've actually been on teams whose only standard was that each individual file had to have a consistent style without too much hassle. I think that the only objective case that can be made is that code generators have an easier time if you leave the braces in and that only applies to projects that use such generators. Otherwise its best to just pick one and move on. Those who aren't used to it will get used to it (and benefit from the exercise in being flexible). -- PhilGoodwin
I read somewhere on the NASA software engineering laboratory website http://sel.gsfc.nasa.gov/ [sorry I don't have the URL] that they adopted the C coding standard of lining up braces, after trying various styles and keeping track of bugs related to each style. They do code reviews and keep track of bugs found in code reviews and bugs found in testing. The idea is keep bugs from showing up when their code is running in space-probe.
They found that code written with lined-up braces had fewer bugs overall, and more bugs were found in the code reviews. Code written with braces not lined up had more bugs overall, and fewer of them were found in the code reviews.
This kind of statistical analysis leads me to conclude that lining up braces is more readable for both the writer and the reviewer. It saddens me that Sun adopted the wrong style for Java, and that Java doesn't require braces for conditional and looping blocks. -- KeithRay
I tried to find the exact URL for you ... still haven't found it, but I stumbled across some related pages:)
Personally, I like this style:
private int getMidfixGroup{(int midfix)(boolean isEven=!(midfix&1);
if(isEven){if(midfix>=10){return 2;}else{return 3;}}else{if(midfix
>=10){return 4;}else{return 1;}}}
PointyHairedBoss: "a good source checker will find any bugs and just think how much typing time we'll save"
:-) Sorry, couldn't resist -- jr
Personally, my preference is this one:
private int getMidfixGroup (int midfix)
boolean isEven = !(midfix & 1);
if (isEven)
if (midfix >=10)
return 2;
else
return 3;
else
if (midfix >=10)
return 4;
else
return 1;
but it only works in PythonLanguage, which is much maligned for supporting it. You do have to admit, however, that it makes the whole BracesAroundBlocks problem go away! -- MichaelChermside
I find the extra braces hinder rather than help comprehension. But that's just me. Thanks for the alternative styles (and thanks for the tip about which if should be outer - I missed that!). By the way, I gave my OK to a group standard that requires the silly braces, because having a consistent standard gives us a chance at having ownerless code, which is far more valuable to me than being able to leave out the braces.
You could try
return (midfix >= 10) ? (isEven() ? 2 : 4) : (isEven() ? 3 : 1);Then you don't need the if (aside: I'm never sure what the best way to indent the ?: ternary operator is)! You don't indent them. If you nest them you should turn them into ifs or commit dishonorable suicide.
When I do use if, while, etc., I have a rule that I use braces unless the whole thing is on one line. So I'm happy with
if (foo == null) return false;but not
if (foo == null) return false;-- DaveWhipp
I follow that rule too. Unfortunately it slows me down in Perl, where the braces are mandatory anyway. -- KarlKnechtel
I used to follow that rule until one day's debugging eventually lead to the following code someone else wrote.
if (foo == null) return false; foo = GetDefaultFoo?();The actual line of code was longer, so that you had to look closely to see the return false, obviously someone hadn't. Since that time I use AlwaysUseBracesOnIfThen.
DefensiveProgramming tactics do have a cost - extra code, extra stuff to type. With a good unit test that is likely to catch the bug, it's hard to justify the cost.
I started putting braces everywhere because of things like "makes it easier to add statements", but now I do it just because it's one less nit-picky detail I have to think about when I'd prefer to be thinking about broader issues of code structure and simple design. Extra code? Extra stuff to type? Nah. My fingers do it automatically. I'm much more concerned about extra stuff to think about, especially when the stuff has nothing to do with the problem I'm trying to solve.
When I type the closing parenthesis of an 'if' statement, the body is still a moderately high-level, unified abstraction in my mind. I have no idea yet (in general; obviously some cases are more clear-cut) whether the body will require one statement or more. And when I do start thinking about the individual statements, I want to focus on them, not be distracted by continuing to think about syntactic details of the surrounding control structure.
(And I can't resist pointing out that the title of this page really doesn't make any sense. It should be "Blocks instead of statements" or something like that.) -- GlennVanderburg <glv@pedant.com>
As the perpetrator of that title, I completely agree. It was only when I was half-way through the refactoring that I realized what a dumb name I'd chosen. Actually, I don't like your proposed name much either... Sorry, anyway.
I used to use the braceless form until I learned a few other languages. The turning (breaking?) point was when I worked on a single source file containing C, Perl, and TCL. (The C program used embedded Perl to communicate with a TCL/TK GUI.) Remembering that only C allows braceless if statements didn't seem worthwhile. The only place I still use a braceless style is in empty loops, like:
while (process_input()) ; /* EMPTY */
print $statusmsg if $verbose;...or:
print "Hello\n" unless $silent;...but it's a guilty pleasure. ;-) --CliffordAdams
Personally, I use a braced style for empty loops (like "while (process_input) {}"), so that I won't have to write something like /* EMPTY */ to make my meaning clear. I never write empty if's, and I rarely use postfix-if/postfix-unless in Perl. I'm quite happy with negating my conditions by hand --KarlKnechtel
I'm just learning Perl now and think it is a very cool language. It does seem though that some of the constructs are just there to play tricks on old C coders (Like the do{}while() that does one loop even if the conditional is false to begin with). Do you think the guy that created Perl just had a nerdy sense of humor? :-)
I write empty statements like so:
if (condition1)
{
/* do nothing */;
}
else if ...
It's important that the semi-colon be there, for one important case:
switch (code)
{
...
default:
/* do nothing */;
}
C/C++ requires a semicolon in this case, although I believe Java does not.
I tend to use blocks for cases. So I'd happily write
switch(...)
{
...
default:
{
/* do nothing */
}
}
-- DaveWhipp
Why put in a default: case that does nothing? The language does not require it.
You should really at least assert(false); in the default: case. If you don't have a default: case, you might get a runtime error "Unexpected case" during release. At least this way you'll handle it gracefully. And in debug mode the assert(false) will help catch the error (better than a runtime exception). -- SunirShah
In a case statement with no default where there is no match, C, C++, and Java fall through gently without complaining; Pascal gives a runtime error instead of falling through. In the example given, which isn't Pascal, the empty default is meant as a comment to show that falling through is OK - in that case, if I were doing anything but Pascal, I'd just let it fall through. I think that a case with no default speaks eloquently without a megaphone. I agree that if falling through is bad, you must have a default case with an assert or something.
Geez, why not just emacs and let it handle all this for you? Or use PythonLanguage? -- AnonymousPythonista?
Why is this even in the language? Why can't I switch the *view* in my editor from Classical-C-Mode to Python-Mode to Java-Mode and back, all handled the same internally? -- FalkBruegmann
Yes!!! (but add Pascal-Mode)
For more fun, try:
private int getMidfixGroup(int midfix)
{
return (((midfix & 1) << 1) + (midfix >= 10))
["\x03\x02\x01\x04"];
}
Far too IOCCC, I think, but I couldn't resist. --AlastairBridgewater.
spoiler:
"midfix&1" leaves an evenness detection bit in the lowest bit. "<<1" moves it over to make room for another bit. "midfix>=10" returns a boolean that gets "promoted" to an int because of the addition. So the first part of that mess returns a number between zero and three. The second part relies on an arcane syntax rule in C/C++ that allows the array definition and the index to be switched when you are using square brackets as your dereferencing operator. It's also stashing the integer values for 3, 2, 1 and 4 into a string which is really just an array of bytes. So the whole thing translates the detection criteria into an index into an array of return values.
You're the one who wrote the one-liner that converts time_t to julian date! I knew that if I waited long enough you'd reveal yourself.
While I have never even thought of writing a one-liner to do a date conversion, I did think this one-liner for the example problem given on this page back when the topic first came up. I just couldn't wrap my head around the array for the return values at the time. Now, who wrote this spoiler? It's one of the better descriptions I've seen of how this stuff works. -- AlastairBridgewater [I'd tell you, but PhilGoodwin might kill me. :-]
B-b-b-but that code is full of C/C++-specific hackery, and the private at the start makes it clear that it's actually Java. Bah.
I'm going to go ahead and rub some fur the wrong way. It's late, and I feel bitchy. Quoting (with license) from above, contrast this:
private int getMidfixGroup (int midfix)
{
boolean isEven = (midfix %2 == 0);
if (isEven)
{
if (midfix >=10)
{
return 2;
}
else
{
return 3;
}
}
else
{
if (midfix >=10 )
{
return 4;
}
else
{
return 1;
}
}
}
with this:
private int getMidfixGroup(int midfix)
{
boolean isEven = (midfix %2 == 0);
if (isEven) {
if (midfix >= 10) return (2);
else return (3);
}
else {
if (midfix >= 10) return (4);
else return (1);
}
}
and I think you begin to see why I don't appreciate styles that spread the code out vertically too much. Notice how the second example has all of the code in a space that the eye can grasp at once? Why should I add a bunch of braces and useless, one character lines to code that I can see all at the same time? I even make sure that the numeric values are separated from the operators with a space. I also put return values inside parens so that they are obviously return entities and not operands.
Am I like, you know, totally way cool, or what?!? -- MartySchrader <cough, hack>
Alternatives:
private int getMidfixGroup(int midfix)
{
if(midfix %2==0)return (midfix>=10)?2:3;
return (midfix>=10)?4:1;
}
or
private int getMidfixGroup(int midfix){
if(midfix %2==0)return (midfix>=10)?2:3;
return (midfix>=10)?4:1;}
or
private int getMidfixGroup(int midfix){
return (midfix %2==0)?(midfix>=10)?2:3?(midfix>=10)?4:1;}
or
private int getMidfixGroup(int midfix){return (midfix %2==0)?(midfix>=10)?2:3?(midfix>=10)?4:1;}
I'd take the same theme further. (Actually I might write the version with ?: above, but let's ignore that for now.) What we have here is a result depending on two independent criteria. So arrange the possible results in a 2x2 array, and make it clear that that's what's happening:
private int getMidfixGroup(int midfix) {
boolean isEven = (midfix %2 == 0);
boolean isLarge = (midfix >= 10);
// -------- large -------- ---- small ----
if (isEven) { if (isLarge) return 2; else return 3; } // -- even --
else { if (isLarge) return 4; else return 1; } // -- odd --
}
Now, that's cool. :-) --GarethMcCaughan
In both these versions the indentation fails to show the structure of the code. Using comments is saying verbally something which should be obvious pictorially. I find both hard to read. -- DaveHarris
I don't believe you. And even that's true, it's your private failing, and is quite irrelevant.
Uhh, what? Can you please explain "pictorially," as used in this context? Are you serious about both of the immediately previous examples being hard to read? Does anybody else want to back this up? This is not turf protection for me; I want to know if I'm making people's lives more difficult. -- MartySchrader
I prefer using arrays: in C or C++, I'd use:
private:
int getMidfixGroup(int midfix) {
const bool isEven = (midfix % 2 == 0);
const bool isLarge = (midfix >= 10);
static const int group[][] =
// small large
{{1, 4}, // odd
{3, 2}}; // even
return group[isEven][isLarge];
}
but Java booleans cannot be used as array indices. -- EricJablow
True about Java array index restrictions. However:
private final const int group[][] =
// small large
{{1, 4}, // odd
{3, 2}}; // even
private final int isEven(int midfix) {
return (midfix % 2 == 0) ? 1 : 0;
}
private final int isLarge(int midfix) {
return (midfix >= 10) ? 1 : 0;
}
private int getMidfixGroup(int midfix) {
return group[isEven(midfix)][isLarge(midfix)];
}
Nice and clear. The final keyword allows the compiler to do certain optimizations so the method calls are faster.
-- StevenNewton
Sure, but try making it look "clear" once you have three or more criteria (dimensions in your array).
private int getMidfixGroup (int midfix)
{
if (midfix %2 == 0) // Is even
{
if (midfix >=10)
return 2;
return 3;
}
if (midfix >=10 )
return 4;
return 1;
Less is more. Elses not required if you're returning. Visual cueing as to the scope. -- CHergerThomann
If less is more:
boolean ge10 = (midfix >= 10); return (midfix % 2 == 0)? (ge10? 2 : 3) : (ge10? 4 : 1);But really, why are we having this silly conversation? There are many ways to write this code, but the topic is the stupidity of requiring braces around every block.
''I'm inclined to think this comment is exactly backwards - I suspect that a BraceWar? is strong evidence that the routine in question needs to be refactored. -- DanilSuits
I'm trying to remember how EwDijkstra would have written this. I can't remember the exact syntax. What I found interesting is that he would allow a degree of duplication in order to make symmetry manifest.
if
isEven & midfix >=10 -> 2;
isEven & midfix < 10 -> 3;
isOdd & midfix >=10 -> 4;
isOdd & midfix < 10 -> 1;
fi
This pattern actually exists in LispLanguage:
(cond
((and isEven (>= midfix 10)) 2)
((and isEven (< midfix 10)) 3)
((and isOdd (>= midfix 10)) 4)
((and isOdd (< midfix 10)) 1))
and in the common C idiom of "else if"
if(isEven && midfix >= 10)return 2; else if(isEven && midfix < 10)return 3; else if(isOdd && midfix >= 10)return 4; else if(isOdd && midfix < 10)return 1;or if returning, no else required
if(isEven && midfix >= 10)return 2; if(isEven && midfix < 10)return 3; if(isOdd && midfix >= 10)return 4; if(isOdd && midfix < 10)return 1;
And then there's always the fun ways to do this in Perl.
Starting off with the mundane, yet readable:
sub getMidfixGroup {
my ($midfix) = @_;
return $midfix >=10 ? 2 : 3 unless $midfix & 1;
return $midfix >=10 ? 4 : 1;
}
But since this *is* Perl we're talking about, why not take the extra step and make it a fun mathematical one-liner? No array indexing needed.
sub getMidfixGroup { 1+($b=(($a=shift)>=10)+2)-(($b+$a)&1)*2 }
-- JustinGreer?
DefensiveProgramming tactics do have a cost -- extra code, extra stuff to read. With a good unit test that is likely to catch the bug, it's hard to justify the cost.
(DefensiveProgramming though is often understood to mean things like checking parameters for NULL-ness and muddling on in an effort to make the code more robust. This is counterproductive, since when the program eventually crashes or breaks, the symptom is a long way from the cause. Better to fail at the first opportunity. OffensiveProgramming anyone? -- JamesYoungman)
If we get to qualify statements with "good" then a whole lot of problems can be avoided. Good programmers won't have a problem so there's no problem. Good unit tests are hard to write, so other support mechanisms are needed as well.
The huge (and inconsistent from one person to another) variations in readability are the most interesting thing here. Because code is so much more intricate and fragile syntactically than any natural language, programmers need all the visual cues available -- but there are many different ways in which those visual cues can be arranged, and most people haven't become accustomed to all of them.
Moral: program in a language where a single style of indentation-and-stuff is mandated either by the language itself (e.g., PythonLanguage) or by long-established practice (e.g., CommonLisp). :-)
The other interesting thing (to me) is how surprised I still find myself getting at other people's preferences. I ought to know better. In fact, I do know better, but someone ought to tell my subconscious. -- GarethMcCaughan
If you write well factored code, the brace issues go away. Once you get your methods down to a readable size and get rid of the extraneous comment clutter, following the code with or without braces is equally easy. Reduce the size of your methods to one level of abstraction and let individuals type in or leave out braces as they please.
But that clearly isn't true. The function being discussed above is a readable size, and it is not equally easy to read however it's laid out.
"Short" does not necessarily imply "readable," but "readable" usually implies "short." Hard to read code can be written with or without brackets. Although I prefer to use brackets to punctuate my code, I find that well written code can be understood almost as well without the brackets; if the brackets are necessary, it is usually a good hint that the code structure could be refactored and improved.
For what it's worth, I'd likely write:
private int getMidfixGroup (int midfix) {
if (midfix %2 == 0) {
if (midfix < 10) return 3;
return 2; // else
}
// midfix % 2 != 0
if (midfix < 10 ) return 1;
return 4; // else
}
With the inclusion of any individual comment depending on how I was feeling that day. It's worth pointing out that I prefer the < and > operators to <= and =>, but I'll use the digraphs if the alternative would be an empty if clause.
For Perl I like array-lookup returns, especially for simple tasks (where there is only one "dimension"). My idiom for it is:
sub foo { ['bar', 'baz', 'quux', 'some other hideous return value']->[$_[0]] }
If the processing needs to be significantly more complicated than that, though, I'll take a more conventional approach.
-- KarlKnechtel
Isn't there a serious CodeSmell here? Can anyone give me a reason why I should have a function which returns such a strange combination of MagicNumbers? You shouldn't, but this is only an illustrative example. I don't think any of the points anyone is making would be changed much if the return values were replaced with symbolic constants, or if there were some plausible reason for returning those numbers as they stand.
Uurrrp... (Nothing like a good MountainDew belch to clear the senses) Assuming C/C++:
int getMidfixGroup (int midfix) {
int midfixGroups[4] = {2,3,4,1};
return midfixGroups[((midfix & 1)*2)+(midfix < 10)];
}
(I know it smells. But hey! McCabe = 1 and one point of return.)
Heh. I was going to try to find the next reduction that eliminated the array, but got bored of it. Like:
int getMidfixGroup (int midfix) {
return ((((midfix & 1)*2)+(midfix < 10)+5)&3)+1;
}
There may be more...
Why is everyone so set on additional conditionals, etc? Do it the easy way!
int getMidfixGroup (int midfix) {
boolean isEven = !(midfix & 1);
return midfix >= 10 ? 2 << isEven : 3 >> isEven;
}
-- AdamBerger
"because someone once put a stray semicolon after an if."
Unfortunately, the above statement fails in my workplace due to the once clause.
My justification for preferring x/endx style of block markers:
http://www.geocities.com/tablizer/endx.htm
See CodingStyle
EditText of this page
(last edited March 17, 2008)
or FindPage with title or text search