3
\$\begingroup\$

As part of processing card payments, before even attempting to process, I need to check and validate the card number to make sure it's valid. For this, I've encapsulated this validation in a record called TCardNumber. It does the trick for what it needs to do, tested with a Visa card and an Amex card. Since this is a sensitive process, I need to make sure I'm going about this right. A card number string can be implicitly passed to/from this record.

NOTE: The processing library supports more card types than standard, but I'm only worried about the 4 main ones (American Express, Visa, MasterCard, and Discover). So the TCardType does indeed include things which I don't use. The index of each corresponds with a unique identifier within the processing library I'm using.

NOTE: The function IsValid is the main function I need to be reviewed, but any input on the rest is more than welcome.

Definition:

type
  TCardType = (ctUnknown = 0, ctAmex = 1, ctDiscover = 2, ctMastercard = 3, ctVisa = 4,
    ctDebit = 5, ctEbt = 6, ctEgc = 7, ctWex = 8, ctVoyager = 9, ctJcb = 10, ctCup = 11);    

  TCardNumber = record
    Num: String;
    class operator implicit(Value: TCardNumber): String;
    class operator implicit(Value: String): TCardNumber;
    function GetStr(const Delim: String = ''): String;
    function Masked: String;
    function IsValid: Boolean;
    function CardType: TCardType;
  end;

And the implementation:

{ TCardNumber }

class operator TCardNumber.implicit(Value: TCardNumber): String;
begin
  Result:= Value.Num;
end;

class operator TCardNumber.implicit(Value: String): TCardNumber;
var
  S: String;
  X: Integer;
  C: Char;
begin
  S:= Value;
  //Strip away any non-numeric characters
  for X := Length(S) downto 1 do begin
    C:= S[X];
    if not (C in ['0'..'9']) then
      Delete(S, X, 1);
  end;
  Result.Num:= S;
end;

function TCardNumber.IsValid: Boolean;
var
  S: String;
  C: Char;
  CheckSum: string;
  i,j: Integer;
  function ReverseStr(const Str: string): string;
  var
    i, Len: Integer;
  begin
    //Reverses string for checksum validation
    Len := Length(Str);
    SetLength(Result, Len);
    for i := 1 to Len do
      Result[i] := Str[Succ(Len-i)];
  end;
begin
  Result:= True;
  S:= Num;

  //Strip extra characters and validate numeric characters
  for i := Length(S) downto 1 do begin //From end to beginning
    C:= S[i];
    if C in ['-',' '] then begin
      Delete(S, i, 1);
    end else
    if (not CharInSet(C, ['0'..'9'])) then begin
      Result:= False;
      Break;
    end;
  end;

  //Validate Length
  if Result then
    Result:= Length(S) in [15,16];

  //Check first digit for card type
  if Result then begin
    C:= S[1];
    //3 = American Express
    //4 = Visa
    //5 = MasterCard
    //6 = Discover
    Result:= CharInSet(C, ['3'..'6']);
  end;

  //Validate Checksum
  //http://www.delphicode.co.uk/is-credit-card-number-valid/
  if Result then begin
    S := ReverseStr(S);
    CheckSum := '';
      for i := 1 to Length(S) do
        if Odd(i) then
          CheckSum := CheckSum + S[i]
        else
          CheckSum := CheckSum + IntToStr(StrToInt(S[i]) * 2);
    j := 0;
    for i := 1 to Length(CheckSum) do
      j := j + StrToInt(CheckSum[i]);
    Result := (j mod 10) = 0;
  end;
end;

function TCardNumber.CardType: TCardType;
var
  I: Integer;
begin
  Result:= TCardType.ctUnknown;
  if IsValid then begin
    I:= StrToIntDef(Num[1], 0);
    case I of
      3: Result:= ctAmex;
      4: Result:= ctVisa;
      5: Result:= ctMasterCard;
      6: Result:= ctDiscover;
    end;
  end;
end;

function TCardNumber.GetStr(const Delim: String): String;
var
  X: Integer;
  S: String;
begin
  S:= Num;
  if (Delim <> '') and (IsValid) then begin
    if CardType = ctAmex then begin
      //4 - 6 - 5
      //xxxx-xxxxxx-xxxxx (15 --> 17)
      Insert(Delim, S, 5);
      Insert(Delim, S, 12);
    end else begin
      //4 - 4 - 4 - 4
      //xxxx-xxxx-xxxx-xxxx (16 --> 19)
      Insert(Delim, S, 5);
      Insert(Delim, S, 10);
      Insert(Delim, S, 15);
    end;
  end;
  Result:= S;
end;

function TCardNumber.Masked: String;
begin
  //Return last 4 digits, prefixed by asterisk characters
  Result:= Trim(Num);
  Result:= Copy(Result, Length(Result)-3, 4);
  Result:= '************'+Result;
end;
\$\endgroup\$
  • \$\begingroup\$ I removed the eCommerce tag, this is actually for a point-of-sale application (although irrelevant). It can be used for eCommerce, but that's not the goal. \$\endgroup\$ – Jerry Dodge Sep 7 '15 at 21:19
1
\$\begingroup\$

It's overkill to use string functions for just computing the digit sum.

const Digit: Integer = Ord(S[i]) - Ord('0');
if Odd(i) then
  CheckSum := CheckSum + Digit
else
  CheckSum := CheckSum + 2 * Digit div 10 + 2 * Digit mod 10;

Also, you can get rid of the ReverseStr function by letting i be the count from the end.

Finally, you should test your code using the official test suite for credit card numbers. I hope there is such a thing; if not, take the test suite from others who have programmed the same. But then, you can also just take their code as well, since it is probably better tested than your newly invented code. Don't reinvent the wheel.

\$\endgroup\$
0
\$\begingroup\$

If you remember from your OOP classes at Uni, you have probably heard about the Single Responsibility Principle. Now, your record is responsible for a number of different validations, which is generally a no-no.

My suggestion:

Type

   ICard = interface
   [GUID HERE]
     function GetCardNumber: String;
     procedure SetCardNumber( Value: String );

     function ValidateNumber : Boolean;
     property CardNumber : String read GetCardNumber write CardNumber;
  end;

  TBaseCardImplementation = class( TInterfacedObject, ICard )
  strict private
     FNumber : String;
     function GetCardNumber: String;
     procedure SetCardNumber( Value: String );
  strict protected
    function ValidateNumber : Boolean;virtual;abstract;
    property CardNumber: : String read GetCardNumber write CardNumber;
  end;

Now you have an interface and a basic object encapsulating what does not change (the number) and abstracting away what changes. You should not try to have other helper methods until you know for sure that all of your validation code uses similar principles (and I have a vague recollection that they do not, I did this very thing in ASP ages ago).

Now, all you need to do is to create descendants that override the validation. By doing this, you have eliminated the need for the enum and have decoupled the implementation of each single validation. This means you can just use one card type if you so wish (by having each in its own unit, you will need to include 2 units or 3 if your interface is in another unit).

At the same time, with the visibility assigned to the members, it is very difficult (but not impossible!) to misuse the classes.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.