CodeSOD: The Address Shuffle

The best thing about being a consultant, in Ashleigh's opinion, was that you got to leave at the end of a job- meaning you never had to live with the mistakes your pointy-haired supervisor forced on you.

This client, an online retailer, just needed an update to their Web.release.config file to resolve their session-management issue. Ashleigh had been hired for a two-week contract, and on Wednesday of week two, the fix went live. Of course, she wouldn't get paid if she didn't manage to look busy, so Thursday morning, she was scrambling for a quick fix to add to the pile.

That's when Betsy, the manager, popped by the temp cube. "Hey, I just got some new code from the offshore team. Mind taking a look at it?"

Normally, Ashleigh would decline such a dubious honor; today, however, it seemed easier than digging at the growing pile of minor defects, so she agreed. The method in question was meant to eliminate blank lines in the middle of the address on an invoice, because it "looked weird" to the accounting department.

Sure, whatever, Ashleigh thought. Dubious cost-savings were hardly unusual in a company that offshored their main development team and paid Ashleigh's exorbitant fees to clean up after them.

private string FormatAddress(Address address){ if (address.Line1 == null or address.Line1 == "") { address.Line1 = address.Line2; address.Line2 = address.Line3; address.Line3 = address.City; address.City = address.PostalCode; address.PostalCode = address.Country; address.Country = null; } if (address.Line2 == null or address.Line2 == "") { address.Line2 = address.Line3; address.Line3 = address.City; address.City = address.PostalCode; address.PostalCode = address.Country; address.Country = null; } if (address.Line3 == null or address.Line3 == "") { address.Line3 = address.City; address.City = address.PostalCode; address.PostalCode = address.Country; address.Country = null; } if (address.City == null or address.City == "") { address.City = address.PostalCode; address.PostalCode = address.Country; address.Country = null; } if (address.PostalCode == null or address.PostalCode == "") { address.PostalCode = address.Country; address.Country = null; } return address.Line1 + "<br />" + address.Line2 + "<br />" + address.Line3 + "<br />" + address.City + "<br />" + address.PostalCode + "<br />" + address.Country;}

The best part, Ashleigh mused as she skimmed over the code for a third time, is that it fails miserably at its goal. It always returns the same number of lines. And if both lines 2 and 3 are empty- probably the majority case- it still leaves a gap in between.

A moment's browsing of the rest of the codebase revealed an even better part: it modified the fields on the address, leading to odd validation bugs later on when the city (which might contain the postcode now, or even the country) or postcode were used.

"My recommendation?" Ashleigh typed into an email she knew she wouldn't send. "Fire the offshore team altogether. Or at least stop paying them by the line."

[Advertisement] Manage IT infrastructure as code across all environments with Puppet. Puppet Enterprise now offers more control and insight, with role-based access control, activity logging and all-new Puppet Apps. Start your free trial today!