CodeSOD: A True Leader's Enhancement

Chuck had some perfectly acceptable C# code running in production. There was nothing terrible about it. It may not be the absolute "best" way to build this logic in terms of being easy to change and maintain in the future, but nothing about it is WTF-y.

if (report.spName == "thisReport" || report.spName == "thatReport"){ LoadUI1();}else if (report.spName == "thirdReport" || report.spName == "thirdReportButMoreSpecific"){ LoadUI2();}else{ LoadUI3();}

At worst, we could argue that using string-ly typed logic for deciding the UI state is suboptimal, but this code is hardly "burn it down" bad.

Fortunately, Chuck's team leader didn't like this code. So that team leader "fixed" it.

if ("thisReport, thatReport".Contains(report.spName)){ LoadUI1();}else if ("thirdReport, thirdReportButMoreSpecific".Contains(spName)){ LoadUI2();}else{ LoadUI3();}

So we keep the string-ly typed logic, but instead of straight equality comparisons, we change it into a Contains check. A Contains check on a string which contains all the possible report names, as a comma separated list. Not only is it less readable, and peforms significantly worse, but if spName is an invalid value, we might get some fun, unexpected results.

Perhaps the team lead was going for an ["array", "of", "allowed", "names"] and missed?

The end result though is that this change definitely made the code worse. The team lead, though, doesn't get their code reviewed by their peers. They're the leader, they have no peers, clearly.

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