Like obscenity, or good-smelly cheese, or how that one network sitcom I swear, totally subverts the genre, bad code is hard to define.
It's not about bugs (except if they're deliberately ignored) since bugs happen in all software. Code that works, at least for the moment, can still be bad code — and code that doesn't work yet can still be good in terms of its approach to a problem.
Antipatterns like poor variable naming, magic numbers, overlong functions, and DRY violations are found in bad code, but those mistakes can be sprinkled around good software without turning the whole thing bad.
Even the gruesome stuff turned out by total newbies isn't necessarily bad code. Humility hopefully stops that code from big-time production. And besides, it may not even run without throwing syntax errors or giving obviously wrong results, so it isn't fair to call it bad (at least in my definition).
Truly bad code has a certain Tu ne sais code. In the mind of its author, it works. But it seems to taunt anyone else with its incompetence, as if every single line is purposely filled with at least one minor mistake, and every wider scope with major mistakes. Such code is always way too long for the task, but it feels like that's so the author can show off more bad decisions. Yes, it runs, but if it happens to work as intended it's dumb luck.
We're going to look a block of bad code below, and I'm going to show you numerous ways in which it earns the title.
Look, except for flashes of brilliance, I don't write world-class code.
But I strive to write business-class code and to always get better. I'd be ashamed of myself otherwise, and get second-hand embarrassment at junk like you see below. The fact that someone got paid to do this is bad enough, but it's worse that their client is trying to get some JS skills and they'll think this is the way things are professionally done.
I'm going to show you 56 lines of JavaScript from a custom LP template. Remember, this code was written by a brand-name agency for money. They should've either put a good developer on the task or not tried at all. (Notably, the webpage layout problem it “solves” could've been solved with just CSS, and that's where it should've stayed.)
$(document).ready(function(){
setTimeout(function() {
var liMaxHeight = -1;
$(".myrow .span6.left-icon a img").each(function(index) {
if ($(this).outerHeight() > liMaxHeight) {
liMaxHeight = $(this).outerHeight();
}
});
$(".myrow .span6.left-icon a").css("height", liMaxHeight + "px");
console.log("max height is" + liMaxHeight);
}, 4000);
setTimeout(function() {
var liMaxHeight2 = -1;
$(".myrow2 .span6.left-icon a img").each(function(index) {
if ($(this).outerHeight() > liMaxHeight2) {
liMaxHeight2 = $(this).outerHeight();
}
});
$(".myrow2 .span6.left-icon a").css("height", liMaxHeight2 + "px");
console.log("max height is" + liMaxHeight2);
}, 4000);
setTimeout(function() {
var liMaxHeight3 = -1;
$(".myrow3 .span6.left-icon a img").each(function(index) {
if ($(this).outerHeight() > liMaxHeight3) {
liMaxHeight3 = $(this).outerHeight();
}
});
$(".myrow3 .span6.left-icon a").css("height", liMaxHeight3 + "px");
console.log("max height is" + liMaxHeight3);
}, 4000);
setTimeout(function() {
var liMaxHeight4 = -1;
$(".myrow4 .span6.left-icon a img").each(function(index) {
if ($(this).outerHeight() > liMaxHeight4) {
liMaxHeight4 = $(this).outerHeight();
}
});
$(".myrow4 .span6.left-icon a").css("height", liMaxHeight4 + "px");
console.log("max height is" + liMaxHeight4);
}, 4000);
setTimeout(function() {
var liMaxHeight5 = -1;
$(".myrow5 .span6.left-icon a img").each(function(index) {
if ($(this).outerHeight() > liMaxHeight5) {
liMaxHeight5 = $(this).outerHeight();
}
});
$(".myrow5 .span6.left-icon a").css("height", liMaxHeight5 + "px");
console.log("max height is" + liMaxHeight5);
}, 4000);
});
Where do I start?
You may know from before that I detest jQuery, though I'm trying to not hold that choice against the developer.[1]
So let me kick off with this: copy-and-paste-and-change is not a coding style. It's a non-coding style, and though I get that non-coders use it millions of times per day, there's no excuse for it in a professional environment. (Remember, this code was written for money by a third party!) As all code changes over time, structure like this guarantees you will make a mistake. As you'll see below, there was no reason for this code to be 5 near-copies of the same logic.
Let me run down some more Bad Things.
Arbitrary limits
Anytime you've got variable names like <text><incrementing number> (liMaxHeight2, liMaxHeight3, etc.) that's a code smell: it signals that you have a Zero-One-Infinity (ZOI) violation. ZOI dictates if you can't limit the members of a collection to exactly 0 or 1, you should not hard-code a limit and should treat the limit as infinite[2]
Here, the code tops out at liMaxHeight5, so this is currently the max number of HTML rows the code can process, for no good reason. To process more, you'd need to copy-paste-change another block of code.
Why 5? Was that a restriction the coder was deliberately placing on the web designer, saying “I refuse to process any more than 5 rows of this table”? Nope, it's that the coder couldn't handle making delicate changes in more than 5 places. So instead of changing their approach to ease later changes, they gambled that the live page wouldn't overrun the limit.
Individual vars instead of arrays
Turning those variables into an autogrowing array is the move. In JS, arrays are your go-to to avoid ZOI mistakes, since they don't have a fixed length.
Arrays aren't scary. They let you create shorter, clearer, and more maintainable code. (Shorter isn't always better — sometimes, more LoC are clearer — but here it's part of the win-win-win.)
Compare this clunker:
var value1 = 1;
var value2 = 10;
var value3 = 99;
var value4 = 445;
var result1 = doSomethingWith(value1);
var result2 = doSomethingWith(value2);
var result3 = doSomethingWith(value3);
var result4 = doSomethingWith(value4);
With this:
var values = [1,10,99,445];
var results = values.map(doSomethingWith);
Consider the gain in maintainability.
To add a new value in the second version, just add an integer into the values array. To do that in the original version, you have to add at least 2 full lines of code, and if you need to reorder the <incrementing number>, you may need to change every line of code!
Note I'm using raw JS map: no framework like jQuery, Underscore, Ramda, etc. is necessary. Another reason I — sorry, have to do this — hate people's reliance on jQuery is that people will use its methods, but never learn that the browser has faster and more powerful methods that are, quite reasonably, not copied into the jQuery namespace.
Discarding return values
This snippet wraps the element this in jQuery twice and gets its outerHeight twice.
if ($(this).outerHeight() > liMaxHeight) {
liMaxHeight = $(this).outerHeight();
}
Don't Repeat Yourself, people. If you need to reuse the result, make one call:
var outerHeight = $(this).outerHeight();
if (outerHeight > liMaxHeight) {
liMaxHeight = outerHeight;
}
Or, if you want to go super-slim, no intermediate var and no repetition:
liMaxHeight = Math.max(liMaxHeight, $(this).outerHeight());
We all re-call functions sometimes. In the context of other repetitive code, though, it's more glaring: as I said above, almost a taunt to those developers who might happen upon the page source. Also, when you're thinking about ZOI, consider what happens if you call this function not 5 but 5000 times. outerHeight() is not a particularly fast operation.
Initialize vars to valid values, or leave them undefined
var liMaxHeight3 = -1;
This line deliberately initializes the variable that will later be used as a CSS height to a value that cannot be a CSS height.
Don't do this. You'll never be able to set { height: -1; } so explicitly making it the default is a head-spinner when reading the code. If you want the default to be auto or 0, use one of those. If you'll check for validity later, leave it undefined. Or if you want something that may not be a valid CSS unit but at least expresses “no number can be less than this in a < comparison” more clearly, use -Infinity.
-1 seems like the worst choice as it suggests there's some special difference between -1 and -2 and undefined, but there isn't.
Redundant DOM selectors
$(".myrow4 .span6.left-icon a").css( ... );
$(".myrow4 .span6.left-icon a img").each( ... );
Each of these 2 calls entails searching the entire document for elements that match this common selector:
.myrow4 .span6.left-icon a
In a large and complex document, this is can be costly, but even in a lightweight template, why be redundant?
This modification only needs to find the relevant <A> elements once, drilling down from there to the <IMG>s that are children of each <A>:
var links = $(".myrow4 .span6.left-icon a");
links.css( ... );
links.find("img").each( ... )
While the performance diff may be in practice nonexistent, the principle of DRY applies. Match a common selector once, then use it as your new root element.
Calling setTimeout() 5 times
If you have multiple copy-and-pasted DOM functions that you want to run right after each other 4 seconds from now, you don't run them all separately with setTimeout(). You wrap them in a single setTimeout(). That change alone would've at least disguised how bad the rest of the code is.
Calling setTimeout() at all
The reason why setTimeout() is being used at all is because of a fundamental misunderstanding of the way browsers and jQuery's ready event work.
ready (akin to the native browser DOMContentLoaded) fires when the HTML scaffolding of the page is ready for further updates, but before remote assets like images have necessarily been fetched into their spots on the page.
But the goal of the bad code is to resize an <A> container to the natural height of its largest <IMG>. As such, you need to have the images actually present in the page!
Waiting 4 seconds for images to load is completely arbitrary. Most of the time, it'll be more than enough time, so you'll be making the user see the wrong layout for longer than necessary. The rest of the time, it'll be not enough time as images may not be finished loading. It's not a trigger, it's a guess, and it's always wrong to some degree.
You don't use setTimeout() to guess how long other things might take, you use it to inject a deliberate pause. It's like Marketo Wait steps, which can be similarly misused. Use a Wait Step if you want to send someone an email a week from now. Don't use a Wait Step because you think it'll take less than 5 minutes for a data value to change: use a Data Value Changes trigger for that.
And there's indeed a trigger you can use: the window load event, which has existed in browsers for 20+ years and is guaranteed to fire when all images and other remote assets are complete. Maybe it's too old-fashioned? More likely, the developer didn't understand you could use something other than ready, so they trapped themselves into the world of guesswork.
The refactor
We can rewrite that 56-line monstrosity in 10 lines that are clearer, more maintainable, and even (since we eliminate the delay) noticeably faster:
window.addEventListener("load", function(e) {
// find all rows
$(".myrow").each(function(rowIdx, row) {
var links = $(row).find(".product a");
// get heights of all child IMGs
var imgHeights = links.find("img").map(function(imgIdx, img) {
return $(img).outerHeight();
});
// set parent A height to tallest IMG
var maxHeight = Math.max.apply(null, imgHeights);
links.css("height", maxHeight + "px");
});
});
So go out there, write better code, and don't make me angry! :)
Notes
[1] But seriously, is element.css("height","20px") actually easier to use/read than element.style.height="20px"?
[2] This actually means as high/low as allowed by the language, OS, and runtime engine, not truly infinite. JS arrays can have 4 billion members. You don't need to test anywhere near that size, and you don't need to claim infinitely scaleable performance. Anyway, the typical ZOI problem starts way before you can claim it's about performance: we're usually talking about arbitrary limits < 100 or even < 10.