CodeSOD: Irritants Make Perls

Grün works for a contracting company. It’s always been a small shop, but a recent glut of contracts meant that they needed to staff up. Lars, the boss, wanted more staff, but didn’t want to increase the amount paid in salaries any more than absolutely necessary, so he found a “clever” solution. He hired college students, part time, and then threw them in the deep end of Perl code, a language some of them had heard of, but none of them had used.

It didn’t go great.

# note that $req is immutable (no method apart from constructor sets a value for its members)
sub release {
   my $req = shift;
  
   my $body = 'operation:' . ' ';

   if (uc($req->op()) eq 'RELEASE') {
        $body .= 'release' . "n";
        # do more stuff to body
        ...
   }
   else {
       $body = 'operation: request' . "n";
   }

   if (uc($req->op()) ne 'RELEASE') {
           register_error('unable to send release mail');    
   }
   
   # and so on
   ...
}

This method checks a $req parameter. Notably, it’s not being passed as a prototype parameter, e.g. as part of the signature– sub release($req)– but accessed by shifting out of @_, the special variable which holds all the parameters. This is the kind of move that gives Perl it’s reputation for being write only, and it’s also a sign that they were cribbing off the Perl documentation as they write. For whatever reason, using shift seems to be the first way Perl documentation teaches people to write subroutines.

This whole thing is doing string concatenation on a $body variable, presumably an email body. I’d normally have unkind words here, but this is Perl- giant piles of string concatenation is just basically par for the course.

The “fun” part in this, of course, is the if statements. If the $req is to “RELEASE”, we append one thing to the body, if it’s not, we append a different thing. But if it’s not, we also register_error. Why couldn’t that be in the else block? Likely because the poor developers didn’t have a good understanding of the code, and the requirements kept changing. But it’s a little head scratcher, especially when we look at the one place this function is called:

if (uc($req->op()) eq 'RELEASE') {
     return release($req);
}

Now, on one hand, having the function check for its error condition and avoiding triggering the error condition at the call site is good defensive programming. But on the other, this all sorta smacks of a developer not fully understanding the problem and spamming checks in there to try and prevent a bug from appearing.

But the real fun one is this snippet, which seems like another case of not really understanding what’s happening:

if(($ok1==1 and $ok3==1)or($ok1==1 and $ok3==1))
{
    print p("Master changed!");
}

We just check the same condition twice.

Now, of course, it’s not the developers’ fault that they didn’t have a good picture of what they should have been doing. Lars was trying to save money by hiring the inexperienced, and as usually happens, the entire thing cost him more money, because Grün and the rest of the team needed to go back over the code and rewrite it.

The upshot, for our college students, is that this was a good resume builder. They’ve all since moved on to bigger companies with better paychecks and actual mentoring programs that will develop their skills.

[Advertisement]
BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!

Source: Read More