Braces are for Other Programmers (aka take your style and shove it)

[Mr. Analog]

I found a rather nasty bug today that made me want to invent the device for punching people through the internet.

Now remember, because this is an example you'll be paying attention, but think about passing this logic over at a glance and you'll see why it sucks balls.

Code Select Expand


if (muffinData == null) return;
if (typeof (muffinData) == "string")
muffinData = JSON.parse(muffinData);
if (Array.isArray(muffinData) && muffinData.length > 0)
muffinData = muffinData[0];
{
for (var propertyName in muffinData) {
if (propertyName == "SelectedSprinklesArray") {
var existingSprinkles = jQuery.makeArray(muffinData[propertyName]);
for (var i = 0; i < $(existingSprinkles).size() ; i++) {
object.addSprinkle(existingSprinkles[i].ID, existingSprinkles[i].Description);
}
}
else {
if (object.hasOwnProperty(propertyName)) {
if (existingData[propertyName] != undefined)
object[propertyName](existingData[propertyName]);
}
}
}
}


Now imagine all this encapsulated in a try/catch that eats exceptions, now imagine running this code in IE7 where Array.IsArray isn't supported.

Get your Bad Code Bingo Card out!
-Spaghetti Code
-Poor formatting
-Syntactically invalid braces
-Not tested on supported platform(s)

Once I isolated the main problem (ECMAScript 5th Edition features don't work in IE7 or older!) I took a look at the logic being attempted here and it hurt my brain, it actually took a few moments to parse what was being attempted here and for the love of god it's not complex but it sure as hell reads that way!!

Generally I ascribe to the rule of thumb that says only omit braces when you have something really short and sweet, otherwise use braces! The inclusion of the redundant braces made this even harder to read.

And I shouldn't even have to mention this but there is a VERY special place in hell for programmers who use a try/catch with no handler in JavaScript.

The kicker? NONE OF THIS TYPE CHECKING WAS NEEDED (the method is hard-coded to a KNOWN DATA BINDING)

Raaaaaaaaaaaaaaaaaaaaaageeeeeeeeeeeeee!

[Thorin]

Nice, it's the unnecessary braces that really @%&# with the programmers perception of how this works...  Took me three tries of reading to realize the stuff in the braces after the if statement will always be run.

I wouldn't mind the early return statement if it was whitespaced so that it was noticeable.  An' yeah, braces for every if statement, that should just be a rule.

Now I want sprinkled donuts, neatly arranged in their container.

[Mr. Analog]

Haha exactly.

At first I wasn't all that upset I mean how many people know without testing that you can't use Array.IsArray in IE7 or older? The real problem I had was after I fixed that and started looking at what the code was actually trying to do. Yikes!

I popped a vessel after having had to re-read it a few times to fully understand and appreciate how truly insidious this could be, I mean imagine hitting a browser that can't handle the superfluous braces in the "array" logic? Remembering that all this would be swallowed in a try/catch and never reported.

The mind boggles.

I won't even tell you what the function is called, it's long, misleading AND ambiguously generic!

[Darren Dirt]

I personally add the { } right after I start typing my IF or FOR or WHILE or whatever, just in CASE the code to execute is more than a single line and/or if there's a non-zero chance that an ELSE (or similar) branch will eventually find its way into the mix.

so...
"
if (Array.isArray(muffinData) && muffinData.length > 0)
   muffinData = muffinData[0];
...
"

woulda been instantly the longer-but-less-likely-to-cause-heartburn...
"
if (Array.isArray(muffinData) && muffinData.length > 0)
{
   muffinData = muffinData[0];
}
...
"

Makes lining up multiple levels of indentation a snap too... especially when in the future a good part of the logic needs to be wrapped inside a new extra outer loop, OR if I want to move a chunk of the logic into its own function (for re-use, or easier maintenance, or w/e)

[Thorin]

Ah but Darren, you've got experience on your side.  Some people write code fresh out of school and just haven't been burned by someone else's mess of code that they have to maintain yet.

[Mr. Analog]

Spot on Darren, in fact I just did a "tip o' the week" thing with my team and basically illustrated what you wrote there

(among other things, like using try/catch with no handlers!!)

[Lazybones]

Well beyond just doing it wrong there seems to be a lot of variation in how they are formatted between languages and programers and even mixed styles in the same code can be confusing

if (bSomething){
   Do this
}

if (bSomething)
{
   Do this
{

if (bSomething)
   {
      do this
   }
   
if (bSomething)
   {
   do this
   }

[Darren Dirt]

Ah but Darren, you've got experience on your side.  Some people write code fresh out of school and just haven't been burned by someone else's mess of code that they have to maintain yet.

PFFF! what do you mean "burned by someone else's mess of code?"




PS: re-reading the OP, does anyone else here now bugged by an irrational desire for sweet baked pastries with sprinkles?   

[Mr. Analog]

Mmmmmmmmmuffin

[Thorin]

PS: re-reading the OP, does anyone else here now bugged by an irrational desire for sweet baked pastries with sprinkles?   

Now I want sprinkled donuts, neatly arranged in their container.

Somebody skimmed somebody else's post, methinks

Lazy: That's why we end up with "coding standards" documents that tell you exactly where to place the braces and to use tabs instead of spaces for indenting and all that fun stuff.

[Lazybones]

Lazy: That's why we end up with "coding standards" documents that tell you exactly where to place the braces and to use tabs instead of spaces for indenting and all that fun stuff.

Well from all the discussions it appears that is not a very common practice or the standards are not always followed.

[Mr. Analog]

Ctrl+K, Ctrl+D

all you need hah

[Darren Dirt]

Lazy: That's why we end up with "coding standards" documents that tell you exactly where to place the braces and to use tabs instead of spaces for indenting and all that fun stuff.

Well from all the discussions it appears that is not a very common practice or the standards are not always followed.

Or use an IDE (or separate tool) that allows you to "re-format" the way you want... I would hate to have a dictate dictate that specific a standard i.e. what if you have trained yourself to prefer over a decade+ of coding with a particular style (which is a never-ending debate -- see Wikipedia *cough* "Each of the enclosing braces occupies an entire line by itself without adding any actual code. Whether this makes the code easier or harder to read is debated." ) -- with that function in your IDE you can just make it appear the way your brain likes, then when you are finished working on that particular challenging chunk of code you just gotta remember to revert the formatting back to the team's standards

[Thorin]

Ctrl+K, Ctrl+D

all you need hah

Pretty sure that's Resharper-specific (and thus Visual Studio specific).  Works nice, though.  Except when two programmers set up their Resharper code layout preferences differently, then every commit can affect the entire file (switching tabs for spaces for indenting, for instance).

[Mr. Analog]

Then it becomes a battle of will! (j/k)

What I recommended this morning was that we set the same options but if we don't then it's ok, they can format one way they want and roll it back.

I just recommended that if they need to format code they do it and check in with a comment before they make changes so when you do a file compare later without losing your mind.