patterncsharpMinor
Luhn Algoritm for social security numbers
Viewed 0 times
algoritmsocialnumberssecurityforluhn
Problem
I just got some negative feedback on some code I wrote and I was not able to know exactly what was wrong with it (I tried). It is a program that verifies to see if a social security number is valid or not. The program is working fine.So I would really appreciate it if somebody could take a look and tell me if this can be optimized in some way.
Here is the code:
Here is the code:
public class Logic
{
public static bool ValidityChecker(string socialSecurityNumber)
{
FileInfo f = new FileInfo("C:\\Log\\log.txt"); //Checks to see if folder path exists. Otherwise creates a C:\Log\ folder.
if (!f.Directory.Exists)
{
Directory.CreateDirectory(f.DirectoryName);
}
if (ValidityCheckIsNotNull(socialSecurityNumber) == true && ValidityCheckIsSSN(socialSecurityNumber) == true) //If the string is not null/empty and if the Social Security Number is valid, then return true.
return true;
else //If the string is null/empty or the social security number is invalid. Then log the invalid data and return false.
using (StreamWriter w = new StreamWriter("C:\\Log\\log.txt", true))
{
w.WriteLine(DateTime.Now + " Kandidatdata: " + socialSecurityNumber.ToString());
w.Close();
}
return false;
}
public static bool ValidityCheckIsNotNull(string socialSecurityNumber) //If string is null or empty return false. Otherwise return true.
{
if (string.IsNullOrEmpty(socialSecurityNumber))
return false;
else return true;
}
public static bool ValidityCheckIsSSN(string socialSecurityNumber) //Uses the algoritm (from the task documentation) to see if a Social Security number is valid or not.
{
int totalSSN = socialSecurityNumber.Where((e) => e >= '0' && e (e - 48) * (i % 2 == 0 ? 1 : 2)).Sum((e) => e / 10 + e % 10);
return totalSSN % 10 == 0;
}
}Solution
Some quick remarks:
Here is that same code, split over multiple lines:
Already it's a lot clearer. However, you need to comment this:
- Use clear variable names:
fdoesn't tell me anything.
- Why do you call
Directory.CreateDirectorybefore you do the actual validity check?
- Do you even need to call
Directory.CreateDirectory? I'm not sure, but I wouldn't be surprised ifnew StreamWriter("C:\\Log\\log.txt", true)creates any missing directories etc.
"C:\\Log\\log.txt"is used in two places, so it should be at least centralized. Assign it to a variable or perhaps even a constant.
- You wrote a whole method --
ValidityCheckIsNotNull-- to encapsulatestring.IsNullOrEmpty? WHY? What do you gain from this? What's the point?
- Your lines are waaay too long. 170 characters which contain multiple LINQ methods are simply unclear. At least split it up into multiple lines.
Here is that same code, split over multiple lines:
socialSecurityNumber
.Where((e) => e >= '0' && e (e - 48) * (i % 2 == 0 ? 1 : 2))
.Sum((e) => e / 10 + e % 10);Already it's a lot clearer. However, you need to comment this:
.Select((e, i) => (e - 48) * (i % 2 == 0 ? 1 : 2)) because I cannot figure out easily what this logic is supposed to do. Properly naming e and i would help, I guess. Or even better: move it to a properly named method of its own.- Comments like
//If the string is not null/empty and if the Social Security Number is valid, then return true.don't tell me anything your code doesn't tell me.
ValidityCheckerisn't a proper method name: "Use verbs or verb phrases to name methods."
- All of your logging logic should be in a separate method. It should not be part of
ValidityChecker.
Logicas the name of your class? Far too generic.
Code Snippets
socialSecurityNumber
.Where((e) => e >= '0' && e <= '9')
.Reverse()
.Select((e, i) => (e - 48) * (i % 2 == 0 ? 1 : 2))
.Sum((e) => e / 10 + e % 10);Context
StackExchange Code Review Q#128500, answer score: 2
Revisions (0)
No revisions yet.