really bad code

When you sign up to accept payments, the folks over at Kagi (aside: why does www.kagi.com work and kagi.com doesn’t?) send you a Perl script you can adapt to process and validate registration keys.

It’s bad.

No. It’s really bad. It would make Wil Shipley vomit.

First off, the thing won’t even run, because it starts with C-style comments:

 /*
  *  Copyright(c) 2004  Kagi.  All Rights Reserved.
  * Permission is granted to use this code module for development of decoding algorithm
  * for the generic acg.No warranty is made as to the suitability to your application 

Next, the comments are in the wrong spot in an if block:

            if($type eq 'U')#user seed {
                $userSeed[$position] = $decimalNumber[$count];
            }
            elsif($type eq 'C')#constant {
                $constant[$position] = $decimalNumber[$count];
            }
            elsif($type eq 'S')#sequence {
                $sequence[$position] = $decimalNumber[$count];
            }
            elsif($type eq 'D')#date {
                $date[$position] = $decimalNumber[$count];
            }

Once you fix that up, it actually runs. But without any arguments, you get useless data:

 User Seed is : U
 Constant is: 
 Sequence is: 
 Date is: 

Yes, that extra leading whitespace is theirs. And they forgot a trailing newline. A usage message will be nice.

Looking further down the code, we observe this subroutine:

    #this method returns the constant
    sub getConstant {
        return $constant
    }

It has so many things wrong with, I’m not sure where to begin. But I will.

• Why do you have an accessor method for a global variable?
• Why do you have a variable named “constant”? A more descriptive name would be incredibly helpful. Plus, this isn’t a constant. It’s an array that’s constructed at runtime.
• The trailing semicolon is missing after the return statement. Now, technically this isn’t required in a one-line block. However, (a) putting it there is good practice and (b) this wasn’t intentional — their three other equally useless accessor methods do have trailing semicolons.
• The comment. What good does “returns the constant” do when you have no idea what “the constant” is? Besides, this type of code should be self-documenting.

They don’t pass parameters. Instead, the initialize method relies on the special Perl variable $_. To make it even less readable, they refer to $_[0].

Next, they needlessly create an array:

            @nameValue = split(/:/,$temp);  # split into key and value
            #store the key value in a hash
            $configDataTable{$nameValue[0]} = $nameValue[1] ;

A cleaner method would be like so:

            ($name, $value) = split(/:/, $temp);  # split into key and value
            $configDataTable{$name} = $value;

Note once again the incredibly unenlightening comment.

Even better, we can replace the whole while loop with one line.

So instead of this:

        @data = split(/%/,$_[0]);
        while(@data) {
            $temp = shift(@data); #get the first name value pair
            @nameValue = split(/:/,$temp);  # split into key and value
            if( $debug == 1) {
                print "\n".$nameValue[0] . " : " .$nameValue[1] ."\n";
            }
            #store the key value in a hash
            $configDataTable{$nameValue[0]} = $nameValue[1] ;
        }

We have this:

        @data = split(/%/,$_[0]);
        %configDataTable = map{  ($k, $v) = split(/:/, $_);  } @data;


That, too, can be consolidated down to:

        %configDataTable = map{  ($k, $v) = split(/:/, $_);  } (split(/%/,$_[0]));

I’m sure they rest is equally bad, but that’s enough for now.