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.
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' };
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);
}
}
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);
}
