CodeSOD: If You're First

Laurie has been supporting an internal application for a few years. The code is a mess, and while she wasn't at the company for the early stages of development, tales are told about it- it was chaotic, it was badly estimated, it was wildly over budget, the latter phases were a crunch where ten developers were shoved onto the project at the last second.

Today, it works- mostly. But it provides plenty of support tickets for Laurie, and demonstrates some WTFs.

One feature is that many forms have a "RESET" button. Push the button, clear out all the values on the form. Here's how that code looks:

for (var i = 0, l = document.getElementsByName("someId").length; i < l; i++) {
  if (i === 0) {
    document.getElementsByName("someId")[i].checked = false;
  } else {
    document.getElementsByName("someId")[i].checked = false;
  }
}
for (var j = 0, l1 = document.getElementsByName("someOtherId").length; j < l1; j++) {
  if (j === 0) {
    document.getElementsByName("someOtherId")[j].checked = false;
  } else {
    document.getElementsByName("someOtherId")[j].checked = false;
  }
}
for (var k = 0, l2 = document.getElementsByName("anotherId").length; k < l2; k++) {
  if (k === 0) {
    document.getElementsByName("anotherId")[k].checked = false;
  } else {
    document.getElementsByName("anotherId")[k].checked = false;
  }
}

Each of these for loops wants to iterate across a set of form fields. Now, document.getElementsByName returns an array of those fields, so we could just iterate across the elements in the array. But no, we have to keep invoking document.getElementsByName every time.

At least they didn't make that mistake about checking the length- they stored that in a variable. But, and it's a minor thing, l, l1, and l2 can look disturbingly like numbers- 12. A good font can help with that, but in a quick skim of the code, it can play a trick on the eyes.

But, of course, the real WTF here are the if statements. If we're on the first element, we uncheck the checkbox. If we're on any other element… we also uncheck the checkbox.

Why is that if statement there? Why is it repeated in every block? We can only guess, but my suspicion is that this is a case of copy/paste programming. In some other part of the application, they wanted to change the state of every box but the first. Since that code was close to what our developer wanted, they copy/pasted and made the minimal changes to make it work, without any attempt to understand the code.

Laurie refactored this block away, but there are many more like it, and there will be plenty of support work.

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!