Code reviews are an important part of development processes, but sometimes, some developers manage to sneak around the process. That was the case with Orien's team, where most of the team were constantly checking in with each other, doing peer reviews, and trying to collaborate on a rather gigantic Perl codebase. One team member, on the other hand… didn't. Management wasn't interested in pushing the issue, so this developer got to do their own thing.

And then the developer left. Over the next thirteen months, bug after bug started cropping up. Minor changes that should have been easy would unleash a ball of spaghettified nonsense that was impossible to debug but also emphatically didn't work. Things got so bad that the entire team needed to stop doing any new development for three months, and just fix bugs and refactor.

In those three months, they mostly fixed things up. Mostly. There are still moments where someone is trawling through the code and finds something that just leaves them scratching their heads.

This is one of those finds:

if ($#errors == -1) { 
  $group_name = $group_name; 
  $group_name = "$group_name"; 
}

This code first checks the last index of the errors array- if it's empty, we want to update the $group_name variable… by setting it equal to itself, then setting it equal to a string of it's value. And yes, that does do string interpolation, so the actual result is that $group_name is converted into a string. Given that the variable is named group name, I suspect it contained a string in the first place, which means this code effectively does nothing useful.

The only thing worse would be if $group_name didn't contain a string.

The good news is that the 13 months of disaster got the entire team, including management, on board with strict code reviews, coding style guides, and pair programming.

[Advertisement] ProGet’s got you covered with security and access controls on your NuGet feeds. Learn more.