All the Perl that's Practical to Extract and Report

labels

KM on 2003-01-29T17:09:59

Many of my co-workers do things like this.. EVERYWHERE:

SWITCH: {
   if (foo) {
      do something
      last SWITCH;
   }

   if (bar) {
      do something
      last SWITCH;
   }

   if (baz) {
      do something
      last SWITCH;
   }

      last SWITCH;
}

And, I tell them over and over that this really isn't what labels are for. I even tell them that it causes Perl to needlessly keep track of more things, and show them some examples of when a label may make better sense, like:

FOO: for my $foo (@bar) {
   blah blah
   BAR: for my $bar (@baz) {
      if (something) {
         next FOO;
      }else{
         next BAR;
      }
}

or

FOO: { something }
BAR: { something }
BAZ: { something }

if (foo) {
   goto FOO;
} elsif (bar) {
   goto BAR;
} else {
   goto BAZ;
}

Basically, using them in a way which isn't simply adding a useless label around a simple block of conditionals.

Am I crazy in my advice here? Is there any other way I could convince people that this just isn't the best programming practice and not just a pet peeve? This is all througout the codebase. A script may have 5 of these (or more) and use multiple modules which also have many of them, etc.. HELP!

Dealing With Co-Workers

jbisbee on 2003-01-29T17:28:25

Slice their ethernet cable. Problem solved. :)

Aack! That's ugly

Ovid on 2003-01-29T23:08:00

No, I wouldn't use a switch-like construct there. What's the point of a label when there's a much cleaner solution? I would probably use a dispatch table like the following:

my %dispatch = (
  foo => \&foo,
  bar => \&bar,
  baz => \&baz
);

#later

if ( exists $dispatch{$function} ) {
  $dispatch{function}->(@args);
}
else {
  die "No such function ($function)";
}

I don't know, maybe it's just me, but that seems much cleaner and easier to read. Plus, even if you forget to test for the existence of the function in the dispatch hash, it will die immediately when you try to use an undefined value as a sub reference. I think that makes it less likely to be buggy.

The only appropriate use of labels that I can think of is to solve thorny control flow problems for which other solutions would be less clear (as you demonstrate). Using them to make a switch statement is not one of those problems. I used to think so, but I found out how easy it is to write bugs in 'em.

Re:Aack! That's ugly

KM on 2003-01-30T00:36:16

Well, I use dispatch tables. But it hasn't caught on with most co-workers. Anyways, the point is trying to get a good way to explain to these people that the way they use labels is wrong. The examples I showed, although not the best way to do things (aside the nested loop), are more 'valid' ways to use labels. Dispatch tables are better than 'goto LABEL', but I show that as an example of a more proper usage. Basically, because people at my work seem to think that when I show them good programming practices, I only do it because I personally think it is better style, as opposed to better programming practices. So, that's a barrier I am dealing with.

I need a good way to show that how they use them (around regular if/elsif conditionals) is Bad. I tell them over and over, I explain it over and over, but no dice. Normally, I wouldn't care if they don't get it. The more piss poor Perl programmers out there means more chances of me getting jobs ;-) But, I have to also maintain this ugly cargo-cult code :-(

Re:Aack! That's ugly

Ovid on 2003-01-30T01:23:20

I humbly submit an example of when SWITCH can go horribly wrong. The temptation to use labels for control flow when they are not required makes bugs like this more likely.

 while ( my $data = $t_sth->fetchrow_arrayref ) {
   my ( $amt, $id ) = @$data;
   $amt /= PRECISION;

   SWITCH: {
     $id == $CASH        && ($tcash        += $amt) && last SWITCH;
     $id == $ACCOUNT     && ($taccount     += $amt) && last SWITCH;
     $id == $CHECK       && ($tcheck       += $amt) && last SWITCH;
     $id == $GIFT        && ($tgift        += $amt) && last SWITCH;
     $id == $VOUCHER     && ($tvoucher     += $amt) && last SWITCH;
     $id == $CC_MAN_AUTH && ($tcc_man_auth += $amt) && last SWITCH;
                            ($tcredit      += $amt);
   }
 }

Many people (including me) will miss the bug. The problem here is the short circuit behavior when the increment results in a false value. Most of the time, this code worked. I asked chromatic for help and he wrote the following:

 my %totals;
 @totals{$CASH, $ACCOUNT, $CHECK, $GIFT, $VOUCHER, $CC_MAN_AUTH} =
   \($tcash, $taccount, $tcheck, $tgift, $tvoucher, $tcc_man_auth);

 while ( my $data = $t_sth->fetchrow_arrayref ) {
   my ( $amt, $id ) = @$data;
   $amt /= PRECISION;
   if (my $totalvar = $totals{ $id }) {
     $$totalvar += $amt;
   }
   else {
     $tcredit += $amt;
   }
 }

His code worked, mine didn't. Now some people would argue that my code's failure is a result of my not paying attention to the short-circuiting behavior of &&, but that's not the real issue. The underlying problem here stems from my having duplicated the same code over and over. chromatic didn't make that mistake. He applied the logic exactly once, I applied it multiple times. Essentially, I cut-n-pasted the code. This leaves less room for thought and more room for error. For every one of your programmers who wants to use that SWITCH construct, point out the duplicate code. That is where a large part of the problem lies.

Re:Aack! That's ugly

KM on 2003-01-30T01:51:35

Yes, I can try this one. I saw the bug when I saw the code. Maybe this one will show why they should avoid labels (although labels are good in deeply nested loops, they don't do that anyways).

Maybe I can turn this into a "THAT IS FIRE, DO NOT TOUCH THE FIRE" ;-)

Re:Aack! That's ugly

TorgoX on 2003-03-05T18:24:18

$id == $CASH && ($tcash += $amt) && last SWITCH;

I saw the "bug" when I saw the code, but then I thought "the programmer wouldn't have used that construct if ($tcash += $amt) could ever yield zero!".

I like pie.