CodeSOD: Bad Code and Taxes

Here in the US, tax season" is extended into the summer. No one likes dealing with taxes, obviously, but we agree that the social benefits outweigh the costs.

I can't speak to how folks feel in Italy. But Riccardo B was perusing the Italian Revenue Service's (INPS) website, and was having a bad time of it. This website was recently modernized", which Riccardo tells us cost 300M (I wasn't able to track down much on this, and since I don't speak Italian, I'll take Riccardo's word on it), so having a bad time" doesn't seem like it should be part of the process.

Like most of us, when he started having problems, Riccardo pulled up the inspector and started debugging. Which is how he spotted this 300M treat:

function wait(ms) { var start = new Date().getTime(); var end = start; while (end < start + ms) { end = new Date().getTime(); }}

Yes, that's a busy-wait function. Yes, it's constantly checking the date and time. No, you absolutely should never put such a thing in your code.

At the time of this writing, that code is still in production. It's in a lovely 4,000 line file which clearly loves jQuery. Given that the file is named app.js I'll give them the benefit of the doubt and assume it was bundled together from separate source files, but it's also not minified, so who knows? It's good for us anyway, as we get to enjoy" the full ugly.

For example, how is wait used?

if ($(article).hasClass("article--with-sticky-nav")) { $(window).scroll(function () { wait(1000); if ($(window).scrollTop() > article.offset().top) { outline.addClass("article__nav--sticky--fixed"); } else if ($(window).scrollBottom() > article.offset().bottom) { outline.addClass("article__nav--sticky--fixed"); } else { outline.removeClass("article__nav--sticky--fixed"); } });}

Oh, it's called inside of a scroll event handler. That's about as fun as paying taxes. If the user tries to scroll when there's a sticky" article displayed, we'll freeze for a second, then slap some CSS classes around. Bonus points for using an else if where an || would do.

As an aside, while CTRL+Fing for wait, I found waitForFinalEvent, which has other treats:

var waitForFinalEvent = (function () { var timers = {}; return function (callback, ms, uniqueId) { if (!uniqueId) { // C85 uniqueId = "timersv0001"; } if (timers[uniqueId]) { clearTimeout(timers[uniqueId]); } timers[uniqueId] = setTimeout(callback, ms); };})();

Here, we can see that someone understands at least something about using setTimeout, the correct way to wait for a period of time. Other than that, this is a pretty straightforward Immediately Invoked Function Expression", which lets them create a timers variable without that variable being placed in the global scope. That's not a WTF, that's just what you do in JavaScript if you're not working with modules. The WTF is that someone in the shop knew the right tool, but whoever wrote wait didn't.

I assume, in that code, all the //C85 type comments represent changes tied to a ticket tracking system, and no one understands source control well enough to think about why that's not a good practice.

[Advertisement] ProGet can centralize your organization's software applications and components to provide uniform access to developers and servers. Check it out!