Problem
I wanted to challenge myself in a new language that I am learning (C#) by doing a simple exercise: check if a “modulus 10” Luhn number is valid or not. In this case the common application if verifying credit card numbers. So I discovered that an exist implementation of this algorithm.
I have tested the code works with generated card numbers)
How might this be improved. I saw on other posts that all the procedure can be done in very few lines of code compared to mine.
Program.cs
using System;
using System.Collections.Generic;
using System.Configuration;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace CreditCardChecker
{
class Program
{
static void Main(string[] args)
{
string cardNumber = Operations.UInput();
LuhnValidator.Validator(cardNumber);
Operations.DoWork();
Console.ReadLine();
}
}
class Operations
{
static string userInput;
static string cardType = "";
public static string UInput()
{
Console.ForegroundColor = ConsoleColor.White;
Console.WriteLine("Insert credit card number");
userInput = Console.ReadLine();
return userInput;
}
public static void DoWork()
{
if (LuhnValidator.IsValid(userInput) == true)
{
Console.ForegroundColor = ConsoleColor.Green;
Console.WriteLine("Valid Card");
var input = userInput.Substring(0, 1);
switch (input)
{
case "3":
Console.ForegroundColor = ConsoleColor.Cyan;
break;
case "4":
Console.ForegroundColor = ConsoleColor.Blue;
break;
case "5":
Console.ForegroundColor = ConsoleColor.Yellow;
break;
case "6":
Console.ForegroundColor = ConsoleColor.DarkYellow;
break;
default:
Console.WriteLine("Card type unidentified");
break;
}
cardType = ConfigurationManager.AppSettings[input];
Console.WriteLine(cardType);
}
else
{
Console.ForegroundColor = ConsoleColor.Red;
Console.WriteLine("Invalid Card");
}
}
}
}
LuhnValidator.cs
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
namespace CreditCardChecker
{
// TODO: refactoring
//CheckDigit included
class LuhnValidator
{
static string invertedCardNumber = "";
static string noCheckDigit = "";
static List<int> numbersArray = new List<int>();
static List<int> listEvenPosition = new List<int>();
static List<int> listOddPosition = new List<int>();
static List<int> evenNumbers = new List<int>();
static List<int> notSumEvenNumbers = new List<int>();
static int sumDoubleEvenNumbers;
static int sumOfAll;
static int magicNumber = 0;
static int checkDigit = 0;
public static void Validator(string cardNumber)
{
Inverser(cardNumber);
AddToList(invertedCardNumber);
EvenOddLists();
Multiply();
Sums();
IsValid(cardNumber);
}
public static void Inverser(string cardNumber)
{
try
{
cardNumber = cardNumber.Replace(" ", string.Empty);
switch (cardNumber.Length)
{
case 16:
noCheckDigit = cardNumber.Substring(0, 15);
break;
case 15:
noCheckDigit = cardNumber.Substring(0, 14);
break;
default:
Console.WriteLine("Insert a valid number");
break;
}
// Invert card number so I can operate from left to right
for (int i = noCheckDigit.Length - 1; i >= 0; i--)
{
invertedCardNumber = invertedCardNumber + cardNumber[i];
}
}
catch (Exception)
{
Console.ForegroundColor = ConsoleColor.Red;
Console.WriteLine("Number too short");
}
}
public static void AddToList(string noCheckDigit)
{
// Convert the numbers from char to int and add them to array
numbersArray = invertedCardNumber.Substring(0, invertedCardNumber.Length).Select(c => c - '0').ToList();
}
public static void EvenOddLists()
{
// Separate Even and Odd numbers into two lists
listEvenPosition = numbersArray.Where((item, index) => index % 2 == 0).ToList();
listOddPosition = numbersArray.Where((item, index) => index % 2 != 0).ToList();
}
public static void Multiply()
{
foreach (var digit in listEvenPosition)
{
int sum = digit * 2;
if (sum > 9)
{
var digits = sum.ToString().Select(x => int.Parse(x.ToString()));
evenNumbers.Add(digits.Sum());
}
if (sum < 9)
{
notSumEvenNumbers.Add(sum);
}
}
sumDoubleEvenNumbers = evenNumbers.Sum();
}
public static void Sums()
{
int a = listOddPosition.Sum();
int b = notSumEvenNumbers.Sum();
sumOfAll = a + b + sumDoubleEvenNumbers;
checkDigit = Convert.ToInt32((sumOfAll * 9).ToString().AsEnumerable().Last().ToString());
if (invertedCardNumber.Length < 16)
{
magicNumber = sumOfAll + checkDigit;
}
if (invertedCardNumber.Length >= 16)
{
magicNumber = sumOfAll;
}
}
public static bool IsValid(string cardNumber)
{
bool status = true;
if (magicNumber % 10 == 0 & cardNumber.EndsWith(checkDigit.ToString()))
{
status = true;
}
else
{
status = false;
}
return status;
}
}
}
Where the code can be improved ? It’s a very basic version, did just to see if it works (in fact, I wrote this code two times before having a working version, this one).
Can I get rid of all that lists at the beggining?
Solution
First of all, I really acknowledge your efforts, and secondly it seems that the code does what it is expected to do.
You split the code into some meaningful methods and the naming is easy to understand in the context.
There’s a lot to comment and I can not cover it all so here is what I have:
The class LuhnValidator:
I wouldn’t make everything static. If you check more card numbers in the same session (sequentially or in parallel) you are in trouble, because all the static variables retain their state throughout the session and may be in an invalid state after the first card number has been checked.
For instance evenNumbers is not reset before checking a number, so for the second number to be checked it contains all the even numbers from the previous card.
It can be convenient to have a static function to call, but then you should do it in this way:
public class LuhnValidator
{
public static bool Validate(string cardNumber)
{
LuhnValidator validator = new LuhnValidator(cardNumber);
return validator.IsValid();
}
}
All members of LuhnValidator should be instance members except the shown Validate()
Some details:
In Inverser(string cardNumber):
switch (cardNumber.Length)
{
case 16:
noCheckDigit = cardNumber.Substring(0, 15);
break;
case 15:
noCheckDigit = cardNumber.Substring(0, 14);
break;
default:
Console.WriteLine("Insert a valid number");
break;
}
It is fine to check for a valid length, but here you continue the process even if the length of the number is invalid (although you write a message). Instead you should throw an exception if the length is invalid:
switch (cardNumber.Length)
{
case 15:
case 16:
noCheckDigit = cardNumber.Substring(0, cardNumber.Length - 1);
break;
default:
throw new ArgumentException("Invalid length of carNumber", nameof(cardNumber));
}
// Invert card number so I can operate from left to right
for (int i = noCheckDigit.Length - 1; i >= 0; i--)
{
invertedCardNumber = invertedCardNumber + cardNumber[i];
}
Instead of
invertedCardNumber = invertedCardNumber + cardNumber[i];
you can write:
invertedCardNumber += cardNumber[i];
Or you can use LINQ:
invertedCardNumber = new string(noCheckDigit.Reverse().ToArray());
In AddToList(string noCheckDigit):
Instead of
numbersArray = invertedCardNumber.Substring(0, invertedCardNumber.Length).Select(c => c - '0').ToList();
}
it seems sufficient to write:
numbersArray = invertedCardNumber.Select(c => c - '0').ToList();
In Multiply():
int sum = digit * 2;
it is actually not a sum but a product:
int product = digit * 2;
if (sum > 9)
{
var digits = sum.ToString().Select(x => int.Parse(x.ToString()));
evenNumbers.Add(digits.Sum());
}
Here if the sum (product) > 9 then you can just subtract 9 from the product:
if (sum > 9)
{
evenNumbers.Add(sum - 9);
}
In Sums():
Instead of
checkDigit = Convert.ToInt32((sumOfAll * 9).ToString().AsEnumerable().Last().ToString());
you can do:
checkDigit = (sumOfAll * 9) % 10;
This:
if (invertedCardNumber.Length < 16)
{
magicNumber = sumOfAll + checkDigit;
}
if (invertedCardNumber.Length >= 16)
{
magicNumber = sumOfAll;
}
can be simplified to:
if (invertedCardNumber.Length < 16)
{
magicNumber = sumOfAll + checkDigit;
}
else
{
magicNumber = sumOfAll;
}
As you’re mentioning there are simpler ways to calculate the Luhn value. One place to start could be Wikipedia
Something else to consider.
It looks like you might be overthinking the problem. An algorithm to create the checksum digit is very simple. To check if a number is valid re-create the checksum digit and check if they’re the same. Basically this reduces the validation to 3 small functions:
public class IDNumber
{
public static bool CheckNumber(string idNumber)
{
int truncatedLength = idNumber.Length - 1;
return idNumber[truncatedLength] == CreateCheckDigit(idNumber.Substring(0, truncatedLength));
}
public static char CreateCheckDigit(string acctNumber)
{
int sum = 0;
for (int i = acctNumber.Length - 1; i >= 1; i -= 2)
{
sum += Get2DigitSum((acctNumber[i] - '0') * 2) + (acctNumber[i - 1] - '0');
}
//an odd number length means that the 0 index isn't evaluated.
//if the length is even this will evaluate to 0.
sum += Get2DigitSum((acctNumber[0] - '0') * (acctNumber.Length % 2) * 2);
//the checksum digit is 10 minus the lastdigit of the sum(sum % 10).
//In case the answer is 10 then we need to % 10 again so that the result is 0.
return (char)(((10 - (sum % 10))% 10) + '0');
}
//This will calculate the correct sum for one or two digits.
static int Get2DigitSum(int num)
{
return (num / 10) + (num % 10);
}
}