Re: Code Review: Simple Credit Card Number Validation

[GrandFather]

First off put the card processing into a sub to make it easier to test the code. That way you can toss a bunch of test case numbers at the code and see that it does the right thing.

Next, deal with problems by bailing as soon as you can so that you don't clutter later code. In particular use early exits to avoid nested conditional blocks and loops.

There are a few other things I've done to make maintenance easier:

#!/usr/perl/bin
#Simple program to check validity of 16 digit credit card numbers

use strict;
use warnings;

print processCard($_)
    for 'x', '41245678912345', '3124567891234560', '3324567891234560',
    '4124567891234560';


sub processCard {
    my ($cardNum) = @_;

    return "Invalid Input: $cardNum.\n" if $cardNum !~ /^[0-9]+$/;
    return "16 digit cards only for now ($cardNum).\n"
        if length $cardNum != 16;
    return "Invalid Card Number $cardNum\n" if !numberOK($cardNum);
    return cardInfo($cardNum);
}


sub numberOK {
    my @cardNum = split //, shift;

    for my $i (map {$_ * 2} 0 .. $#cardNum / 2) {
        $cardNum[$i] *= 2;
        $cardNum[$i] -= 9 if $cardNum[$i] >= 10;
    }

    my $total = 0;
    $total += $_ for (@cardNum);
    return $total % 10 == 0;
}


sub cardInfo {
    my ($cardNum) = @_;
    my %issuers = (4 => "VISA", 5 => "MasterCard", 6 => "Discover");
    my ($iDigit) = $cardNum =~ /^(\d)/;

    return "Unknown issuer code: $iDigit\n" if !exists $issuers{$iDigi
+t};
    return <<INFO;
    
   Card Number: $cardNum
   Issuer:      $issuers{$iDigit}

INFO
}

Prints:

Invalid Input: x.
16 digit cards only for now (41245678912345).
Invalid Card Number 3124567891234560
Unknown issuer code: 3
    
   Card Number: 4124567891234560
   Issuer:      VISA

[marquezc329]

Thank you for your quick and thorough reply. After looking through your revision I can quickly see how my nested control structures were exaggerated nearly to the point of obnoxious. In particular I really liked your use of:

    for my $i (map {$_ * 2} 0 .. $#cardNum / 2) {
        $cardNum[$i] *= 2;
        $cardNum[$i] -= 9 if $cardNum[$i] >= 10;
    }

to cycle through @cardNum. I had thought of trying to use map, but wasn't quite sure how to pick every other element. The only part I don't quite understand is:

   return <<INFO;
    
   Card Number: $cardNum
   Issuer:      $issuers{$iDigit}

INFO

I can see that the subroutine is returning a block called INFO, but I don't quite get the syntax behind the way you defined that block. Is there a name for this style of definition that I can research for a better understanding? Thank you again for reviewing my code. I'm a bit disappointed with myself looking back, but I suppose sophisticated code comes with time and practice.

[GrandFather]

The <<INFO syntax is (*nix) shell like here-doc line oriented quoting. It's really handy when you have multiple lines of quoted text you want to shovel into a parameter list. You'll want to search the linked documentation for here-doc.