I'm posting this because it's a demonstration of an interesting feature in Perl that can cause strange bugs. ... It's a subtle bug and I'm not too embarrassed at missing it.

The problem isn't that code contains bugs -- because it is the nature of humans to make mistakes. The cloud of shame that surrounds the occurrence of bugs in code is counterproductive. Some have the attitude that a coder is inferior for having made a mistake. This attitude is toxic to creating better software. Finding bugs should be a celebrated event. After all, you've just made your code more correct. So, I begin this long missive with: "For those about to debug, we salute you!"

In this case, I think there's a semantic bug in the switch statement. As others have pointed out each "case" statement depends on all three conditions being true. In some rare cases where you want a particular case to execute, it won't because the middle expression will evaluate to a false value. The implication made in the post is that this is a Perl mis-feature. That isn't entirely fair. After all, Perl is faithfully obeying the rules of boolean logic. It would be more surprising if it didn't.

Mark-Jason Dominus has given a talk called "Red Flags" several times. Instead of giving "anti-patterns" per se, he exhibits code that can be rewritten in a safer and often faster way. There appear to be at least two Red Flags in this bit of code.

The first Red Flag is one that MJD calls "Families of Variables," which can be defined as the use of several scalars to hold related values. In many cases, it is better to store these values in a collection type, such as an array or hash, rather than complicate code with many rarely used scalars. Chromatic's solution addresses this Red Flag well. By storing $tcash, $taccount, $tcheck, $tgift, $tvoucher, $tcc_man_auth in one hash called %totals, the code becomes cleaner and these random scalars are coraled into one easy-to-handle place.

I have noticed that newbies "phear" Perl hashes and I'm not entirely sure why. Hashes, of all data structures, operate most like the human mind -- using word associations to remember things. Hashes, regexes and CPAN are the three most important reasons to use Perl over other languages.

The other Red Flag is the use of boolean operators for control statements. As this code has just demonstrated, boolean logic can be trickier than it first appears. Of course, most of us can see what's going on here by sitting down with pencil and paper and drawing truth tables, but that's work. Perl is about getting done with work faster than your Java friends and drinking a whole pint before they arrive at the bar (well, that's the reason I use Perl). So, I submit that a complex use of boolean operators for control statements is a Red Flag. When one comes across such code, ask yourself whether the code would be clearer with if statements. You'll enjoy debugging if statements much more than a series of ampersands, double pipes and parentheses. Of course, sometimes you really, really need the complex boolean statements. Run far away from that kind of code.

Uncle Joe's Programming Tip #3: Tricky code makes debugging hard.

Perhaps I'm being dense, but I think Chromatic's solution is overkill. Let's look at the (shorten) line:

 
if (my $totalvar = $totals{ $id }) {
    $$totalvar += $amt;
}
[download]

It appears that %totals is a hash that has $id values as the key and scalar references as values. That seems weird. I think the idea of this code is to see if there is an amount of $id type in the hash (even if that value is zero), and add the new $amt to it. Because references are always true, this code won't get bogged down in the problems of the first code example. I think there is an easier way to write without references:

 
if( defined $total{$id} ){
     $total{$id} += $amt;
}else{
     $total{tocredit} += $amt;
}
[download]

Again, I think it's good to keep the account totals together. Therefore, I would stuff even the 'tocredit' values into this hash. I assume that %totals will be populated with code that will look something like:

 
my $sth = $dbh->prepare('select * from accounts where id = ?'); 
$sth->execute($current_account_id); 
my $hr = $sth->fetchrow_hashref;

# a naive way 
my %totals = %{$hr};
[download]

This assumes that the $id value presented in the original code is one of the SQL column names in the accounts table.

Originally I used 'exists' rather than 'defined,' but that makes populating the %total hash trickier. In fact we only care if the value is not undef.

In this case, a switch statement isn't needed at all. Perl 6 will have a much cleaner syntax for creating switches (given, when). Until then, try to think about using hashes instead of Perl 5's fishy switches.

Cheers.