-1
\$\begingroup\$

I am writing a credit card generator. I have method GenerateVisa() who will return either a 13 or 16 numbers string. This is the default value but GenerateVisa can also take a parameter. THis parameter will allow us to force the output to either 13 or 16. What is this parameter? I could use a int like GenerateVisa(int length = -1) and write

GenerateVisa(int length = -1)
{
    if (length == -1)
        return randomly 13 or 16
    if (length == 13)
        return 13 numbers string
    if (length == 16)
        return 16 numbers string
    
    // what if we have any other value? fallback to random? throw?
}

I dont like this solution because it is not very good for discoverability. My current solution is to rely on an enum:

enum VisaLength {
    Random = 0,
    Thirteen = 1,
    Sixteen = 2
}

GenerateVisa(VisaLength length = VisaLength.Random)
{
    if (length == VisaLength.Random)
        return randomly 13 or 16
    if (length == VisaLength.Thirteen)
        return 13 numbers string
    if (length == VisaLength.Sixteen)
        return 16 numbers string
    
    // what if we have any other value? fallback to random? throw?
}

This work quite well until I learnt that

  • Maestro card can go from 12 to 19
  • Solo can accept 16, 18 or 19
  • Some are 16 only
  • I have 28 credit card to support with all kind of other variation

You can find all this card on Wikipedia

Should I write a different enum each time? Is there a better way to handle that?

My original code is in F#, but I will gladly see how others languages deals with it.

\$\endgroup\$
6
  • \$\begingroup\$ Hi Aloisdg - the others aren't "VISA" - perhaps the generate visa method shouldn't be creating them?? \$\endgroup\$ – Mr R May 22 at 9:58
  • \$\begingroup\$ @MrR I have a GenerateVisa, a GenerateMaestro, a GenerateSolo, etc. \$\endgroup\$ – aloisdg May 22 at 10:12
  • 2
    \$\begingroup\$ Please follow the guidelines: codereview.stackexchange.com/help/how-to-ask \$\endgroup\$ – BCdotWEB May 22 at 13:46
  • \$\begingroup\$ @BCdotWEB would this question be more fitting over softwareengineering.stackexchange.com ? \$\endgroup\$ – aloisdg May 22 at 14:05
  • \$\begingroup\$ from what you have posted here it appears that you aren't finished with this portion of the code. Code Review is for Reviewing code that is fully functional as is, without any changes. \$\endgroup\$ – Malachi May 24 at 13:42
1
\$\begingroup\$

You may need to have a class for each card type, and an abstract class that would be shared between them.

from the Wikipedia page, I can see each card can have Name, IIN Range , Active, Length and Validation. Now, from there we can get the common values for each card (what are the common things that most cards shared ?) to have a base line to start from. I can see that card number is mostly between 12 and 19 digits. Which give us a base values for CardNumberMaxLength and CardNumberMinLength. Now, the Validation for each card can vary, but from the Wikipedia page it shows that most cards uses Luhn algorithm validation. This means, we can use it as a base validation process in our abstract class. In which, would make things much easier for us.

We can now construct these into an abstract class something like this :

public abstract class PaymentCard
{
    private static readonly Random _random = new Random(); 
    
    public virtual int CardNumberMaxLength { get; } = 19;
    
    public virtual int CardNumberMinLength { get; } = 12;
    
    public virtual string Name { get; }
    
    public virtual string Number { get; set; }
    
    public virtual bool IsActive { get; } = true;
    
    private int[] InternalGenerateCardNumber(int length, string prefix = null)
    {
        var startIndex = 0; 
        
        var cardNumbers = new int[length];

        if(!string.IsNullOrWhiteSpace(prefix))
        {
            startIndex += prefix.Length; 
            
            for(var x = 0; x < prefix.Length; x++)
            {
                cardNumbers[x] = prefix[x] - '0';
            }
        }
        
        while(startIndex < length)
        {
            cardNumbers[startIndex] = _random.Next(0, 10);
            startIndex++;
        }

        return cardNumbers;
    }
    
    protected virtual string GenerateNewCardNumber(int length, string prefix = null)
    {
        
        string str = null; 
        
        do 
        {
            var cardNumbers = InternalGenerateCardNumber(length);
            str = string.Concat(cardNumbers);   
        }
        while(!Validate(str));
        
        return str;
    }

    /// <summary>
    /// Validate Credit Card Number using Luhn algorithm
    /// </summary>
    /// <param name="cardNumber"></param>
    /// <returns></returns>
    public virtual bool Validate(string cardNumber)
    {
        if(!string.IsNullOrWhiteSpace(cardNumber) && cardNumber.Length >= CardNumberMinLength && cardNumber.Length <= CardNumberMaxLength)
        {
            var digits = cardNumber.ToCharArray().Select(x=> Convert.ToInt16(x)).ToArray();

            var sum = 0;

            for(int x = 0; x < digits.Length; x++)
            {
                var digit = digits[x];

                if(x % 2 != 0)
                {
                    sum += digit;
                }
                else
                {
                    var doubled = digit * 2;
                    sum += doubled < 10 ? doubled : ( doubled / 10 ) + ( doubled % 10 );
                }
            }

            return sum % 10 == 0;
        }

        return false;
    }   

    public abstract string GenerateNewCardNumber();
}

Now, we can use this abstract to implement each card something like this :

public class VisaCard : PaymentCard
{
    public override string Name => "Visa"; 
    
    public override int CardNumberMaxLength => 16; 
    
    public override int CardNumberMinLength => 16; 
    
    public override string GenerateNewCardNumber() {
        return base.GenerateNewCardNumber(16);
    }
    
}

public class MaestroCard : PaymentCard
{
    public override string Name => "Maestro"; 

    public override string GenerateNewCardNumber() {
        return base.GenerateNewCardNumber(16, "5893");
    }
}

this way, you'll have a full control on each card type. for instance, if you need a custom validate for any card, you just override the Validation method and do your own custom validation. The rest of work should be easy to handle.

usage example :

public string GenerateCardNumber(PaymentCard card) {
   return card.GenerateNewCardNumber(); 
} 

now we can create it this way :

var visa = new VisaCard();  
var cardNumber = GenerateCardNumber(visa); 
// this would return the VisaCard.GenerateNewCardNumber() result.
\$\endgroup\$
8
  • \$\begingroup\$ Thank you for writing the whole code. I am not sure how OOP solve the issue. Now how would you create a Visa card with 13 number? Would you create a new constructor? What about a card with n different available length? Would you create n constructor? \$\endgroup\$ – aloisdg May 22 at 12:52
  • \$\begingroup\$ @aloisdg, yes you'll need to define a new type for each card type. the example above VisaCard shows how to have a fixed length (exactly 16 digits) while MaestroCard shows a length range (taken from the base class default values). \$\endgroup\$ – iSR5 May 22 at 15:49
  • \$\begingroup\$ Yes I saw but in your code everything is random. This is not what I am looking for. I want to force a bunch of input (range or not) while sharing the information with the function caller. \$\endgroup\$ – aloisdg May 22 at 18:12
  • \$\begingroup\$ @aloisdg the random part is just an example, which you need to change it to suit your needs. for instance, if a card has a range, then adjust the max and min length, and override the generate method to your custom range part, and so on. \$\endgroup\$ – iSR5 May 22 at 19:35
  • \$\begingroup\$ As a function caller, how to I know if I can create a card with 16, 18 or only 19 numbers or any of those valid value? I would like to define a guard (akin to the guard pattern) at parameter level. I can achieve something similar with enums but it is quite cumbersome. \$\endgroup\$ – aloisdg May 22 at 20:28

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