# Validating a string under few conditions

Following code validates NIC number of a person (National Identity Card, no like SSN) Requirements are...

1. Length should be 10 digits or characters
2. All should be digits except the last one
3. Last one would be x or v in simple letters

Currently I'm using following method and its working. I just want to know that could this be make better or simplified?

    private bool ValidateNIC(string nic)
{
if (!string.IsNullOrEmpty(nic))
{
char[] letter = nic.ToCharArray();
if (letter.Length == 10)
{
if (letter[9] == 'v' | letter[9] == 'x')
{
for (int i = 0; i <= (letter.Length - 2); i++)
{
if (char.IsNumber(letter[i]) == false)
{
MessageBox.Show(string.Format("letter {0 } is not allowed, all Should be numbers except the last one.", letter[i]));
return false;
}
}
return true;
}
else
{
MessageBox.Show("Last letter should be x or v in simple letters");
return false;
}
}
else
{
MessageBox.Show("Length should be 10 ");
return false;
}
}
else
{
MessageBox.Show("NIC is null");
return false;
}
}


1. As I think George Howarth was suggesting in his answer you can reduce nesting and avoid the Arrow code effect by returning early from the method. This helps with code flow as well as readability.

2. As already mentioned if you can remove constant literals from your code that is good. One way to do this is to move these to private static or const variables or even supply them to the method.

private const int RequiredLength = 10;
private readonly char[] _acceptedLastCharacters = { 'x','v' };

3. I like to try and remove dependecies from the view especially in business logic which validation can often be. So in this case I would consider returning a result class and let your caller determine how to handle it. This would also mean creating unit tests would be so much easier which I think in this case would be ideal.

internal class ValidationResult
{
public bool IsValid { get { return string.IsNullOrEmpty(ErrorMessage); } }
public string ErrorMessage { get; private set; }

private ValidationResult(string errorMessage)
{
ErrorMessage = errorMessage;
}

public static ValidationResult Success()
{
return new ValidationResult(string.Empty);
}

public static ValidationResult Failure(string errorMessage)
{
return new ValidationResult(errorMessage);
}


}

4. A great thing about writing unit tests is that it can sometimes ensure you think about your requirements before you even code and so help cover everything. In this case I was writing some unit tests for the method and it occured to me that the code might not cover the case of empty strings. I created the test and hello, it had worked. That was great. Now I also have a test! And not only that, doing a couple of other tests showed some flaws in my code which meant I identified problems with tests. Awesome!

Here is an example of the entire re-factored code:

internal class ValidationResult
{
public bool IsValid { get { return string.IsNullOrEmpty(ErrorMessage); } }
public string ErrorMessage { get; private set; }

private ValidationResult(string errorMessage)
{
ErrorMessage = errorMessage;
}

public static ValidationResult Success()
{
return new ValidationResult(string.Empty);
}

public static ValidationResult Failure(string errorMessage)
{
return new ValidationResult(errorMessage);
}
}

class NicValidator
{
private const int RequiredLength = 10;
private readonly char[] _acceptedLastCharacters = { 'x','v' };

public ValidationResult ValidateNic(string nic)
{
var trimmedNic = nic.Trim();

if (trimmedNic.Length != RequiredLength)
{
return Failed("NIC cannot be empty and is not of required length " + RequiredLength);
}

if (!_acceptedLastCharacters.Contains(trimmedNic.Last()))
{
return Failed(string.Format("Last letter is not supported and should be one of [{0}]", string.Join(",", _acceptedLastCharacters)));
}

// Ignore last character as that is handled in earlier check
var illegalCharacters = trimmedNic
.Take(trimmedNic.Length - 1)
.Where(p => !char.IsNumber(p))
.ToList();

if (illegalCharacters.Any())
{
string errorMessage = string.Format("letters {0} are not allowed, all Should be numbers except the last one.",
string.Join(",", illegalCharacters));

return Failed(errorMessage);
}

return ValidationResult.Success();
}

private ValidationResult Failed(string errorMessage)
{
return ValidationResult.Failure(errorMessage);
}
}


With some unit tests (I didn't do all, just enough to test base functionality). Note. I started these tests before I even started re-factoring the code so that I had them passing first. Then during re-factoring I could confidently know whether what I did was still returning the correct results.

[TestMethod]
public void StringIsOfRequiredLength()
{
// arrange
var validator = new NicValidator();
const string nic = "123456789x";

// assert
var result = validator.ValidateNic(nic);

// act
Assert.IsTrue(result.IsValid, result.ErrorMessage);
}

[TestMethod]
public void StringIsNotOfRequiredLength()
{
// arrange
var validator = new NicValidator();
const string nic = "12345678x";

// assert
var result = validator.ValidateNic(nic);

// act
Assert.IsFalse(result.IsValid, result.ErrorMessage);
}

[TestMethod]
public void NicIsNotAllSpaces()
{
// arrange
var validator = new NicValidator();
const string nic = "          ";

// assert
var result = validator.ValidateNic(nic);

// act
Assert.IsFalse(result.IsValid, result.ErrorMessage);
}


I don't like hardcoding values like this. I'm sure you'll be ok, because I doubt the format of your NIC will ever change, but for clarity's sake...

This:

if (letter[9] == 'v' | letter[9] == 'x')


Should be this:

if (letter[letter.length - 1] == 'v' | letter[letter.length - 1] == 'x')


Were the message boxes for debugging, or do you intend to display data to your users with them? I don't think your validation routine should be concerned with displaying messages. If anything, you should be throwing errors and catching them higher up in the control flow.

private bool ValidateNIC(string nic)
{
if (String.IsNullOrWhiteSpace(nic))
{
ShowMessage("NIC is null");
return false;
}

if (nic.Length != 10)
{
ShowMessage("Length should be 10");
return false;
}

var lastChar = nic[9];

// Case sensitive
if (lastChar != 'v' && lastChar != 'x')
{
ShowMessage("Last character should be 'v' or 'x'");
return false;
}

var otherChars = nic.Substring(0, 9);

if (otherChars.Any(x => !Char.IsNumber(x)))
{
ShowMessage("All characters except the last one should be numbers");
return false;
}

return true;
}

private void ShowMessage(string message)
{
MessageBox.Show(message);
}


Edit: Forgot to mention that the character check is case-sensitive.

• Perhaps you might like to mention why your code is a good alternative to the OP question? Jun 19, 2014 at 17:41