Beefy Boxes and Bandwidth Generously Provided by pair Networks
laziness, impatience, and hubris

Re: Code Review: Simple Credit Card Number Validation

by GrandFather (Sage)
on Oct 01, 2012 at 01:30 UTC ( #996568=note: print w/replies, xml ) Need Help??

in reply to Code Review: Simple Credit Card Number Validation

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 }


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

Replies are listed 'Best First'.
Re^2: Code Review: Simple Credit Card Number Validation
by marquezc329 (Scribe) on Oct 01, 2012 at 02:47 UTC
    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.

      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.

      True laziness is hard work

Log In?

What's my password?
Create A New User
Node Status?
node history
Node Type: note [id://996568]
and all is quiet...

How do I use this? | Other CB clients
Other Users?
Others chilling in the Monastery: (5)
As of 2018-06-24 15:18 GMT
Find Nodes?
    Voting Booth?
    Should cpanminus be part of the standard Perl release?

    Results (126 votes). Check out past polls.