Code Review: Simple Credit Card Number Validation

[marquezc329]

I'm still new to programming and have been working through Lerning Perl and LPORM in conjunction with the perldoc. While reading an article I came across about how credit card numbers work, I figured it was a decent opportunity to practice what I've been learning. I know this is basic code, but I was hoping that in offering it up, the monks would review it and tell me if I'm striking out syntactically/semantically and/or demonstrating ignorance of generally accepted standards/conventions. The code runs, and does what it's supposed to. I plan on modifying it later to include validation for 13,14, and 15 digit credit card numbers as well as possibly reading a list of numbers from a text file and writing only valid numbers to a new file. I wanted to wait on expanding the program until I was sure that I wasn't completely off track.

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

use strict;
use warnings;

sub checkNumber {
   my $in = shift;
   my @cardNum = @{$in};
   my $i = $#cardNum - 1; 

   while ($i >= 0) {
      $cardNum[$i] = $cardNum[$i] * 2;
      if ($cardNum[$i] >= 10) { $cardNum[$i] -= 9; }
      $i -= 2;
   }
   
   my $total = 0;
   $total += $_ foreach (@cardNum);
   if ($total % 10 == 0) { 
      return 1;
   } else {
      return 0;
   }
}

sub cardInfo {
   my $cardNum = shift;
   my $issuer; 
   if ($cardNum =~ /^4/) { $issuer = "VISA" }
   if ($cardNum =~ /^5/) { $issuer = "MasterCard" }
   if ($cardNum =~ /^6/) { $issuer = "Discover" }
   print "\n   Card Number: $cardNum\n";
   print "   Issuer:  ",  $issuer,"\n\n";
}

print "Enter Number:   ";
chomp(my $cardNum = <STDIN>);

if ($cardNum =~ /^[0-9]+$/) {
   my @number = split(//,$cardNum);
   if ($#number == 15) {
      if (checkNumber(\@number)) {
         cardInfo($cardNum);
      } else { 
         print "\nInvalid Card Number\n";
      }
   } else {
      print "\n16 digit cards only for now.\n";
   }
} else { 
   print "\nInvalid Input.\n";
}

[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.

[toolic]

CPAN is a good place to search if anyone else has created Perl code for credit cards.

[marquezc329]

Thank you for your response. I'll be sure to search CPAN for ideas as I continue to refine this piece and attempt to create a more full-featured work.