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.
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!
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.
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!
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)
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.
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!!)
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
Quoteif (bSomething){
Do this
}
if (bSomething)
{
Do this
{
if (bSomething)
{
do this
}
if (bSomething)
{
do this
}
Quote from: Thorin on February 22, 2013, 09:28:23 AM
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?"
;D
PS: re-reading the OP, does anyone else here now bugged by an irrational desire for sweet baked pastries with sprinkles? >:(
Mmmmmmmmmuffin
Quote from: Darren Dirt on February 22, 2013, 11:33:36 AM
PS: re-reading the OP, does anyone else here now bugged by an irrational desire for sweet baked pastries with sprinkles? >:(
Quote from: Thorin on February 21, 2013, 02:58:40 PM
Now I want sprinkled donuts, neatly arranged in their container.
Somebody skimmed somebody else's post, methinks :P
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.
Quote from: Thorin on February 22, 2013, 01:25:39 PM
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.
Ctrl+K, Ctrl+D
all you need hah
Quote from: Lazybones on February 22, 2013, 01:33:10 PM
Quote from: Thorin on February 22, 2013, 01:25:39 PM
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 (http://en.wikipedia.org/wiki/Indent_style#Tools)... 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* (http://en.wikipedia.org/wiki/Indent_style#Allman_style) "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 ;)
Quote from: Mr. Analog on February 22, 2013, 01:33:47 PM
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).
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.
That's an excellent suggestion, to keep the code formatting separate from the logic changes. Also, just tell them they have to follow your style because otherwise you'll take their power supplies from their computers.
I'll have to bring the "Board of Eduction" out of retirement...
Quote from: Mr. Analog on February 22, 2013, 02:29:04 PM
"Board of Eduction"
While I know what you meant to type, I now learned that "eduction" is an actual word: http://www.thefreedictionary.com/eduction
It deduces, through physical force, that your coding style is obtuse :D
I prefer the Board of Edumacation.
Quote from: Thorin on February 22, 2013, 01:48:08 PM
Quote from: Mr. Analog on February 22, 2013, 01:33:47 PM
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).
http://www.jetbrains.com/resharper/features/code_formatting.html nice.
Quote from: Mr. Analog on February 22, 2013, 02:29:04 PM
I'll have to bring the "Board of Eduction" out of retirement...
Quote from: Mr. Analog on February 22, 2013, 02:41:24 PM
It deduces, through physical force, that your coding style is obtuse :D
Very i.Ron.ic typo (http://www.thefreedictionary.com/eduction) there bro -- cuz the intimidating Board being wielded = likely "elicits" compliance to your dictated standards!
MAH HAHAHA!
That's twice Darren has said the same thing as me today, in this thread... We seem to be on the same wavelength.
Quote from: Thorin on February 22, 2013, 06:56:16 PM
That's twice Darren has said the same thing as me today, in this thread... We seem to be on the same wavelength.
Be afraid. Be very afraid.
;)