6
\$\begingroup\$

I have a validation which checks that a string is a valid asset number. There are currently three formats it could be in, like: 001-123456, WSH002M52B, or CTR0025MLD. The way my code is set up now is like:

public bool IsValidAssetNum(string assetNum)
{
    if (this.IsNorthAmericanAssetFormat(assetNum)) return true;
    if (this.IsGermanAssetFormat(assetNum)) return true;
    if (this.IsAustralianAssetFormat(assetNum)) return true;

    return false;
}

With each of those checks being like:

private bool IsNorthAmericanAssetFormat(string assetNum)
{
    var regex = new Regex(ComponentsResources.NorthAmericaAssetNumValidationRegex);
    var match = regex.Match(assetNum);

    return match.Success;
}

In the future, if more formats come up, we'll have to add a new specific format check as well as add it to the IsValidAssetNum function. Is there a pattern out there to solve this issue? I know a lot of if statement refactorings come from the command pattern or strategy pattern, but this seems like a different type of issue.

\$\endgroup\$
0

2 Answers 2

7
\$\begingroup\$

Regular expressions, when compiled down, are very high performing. I would suggest that you build a composite expression, let the Regex engine optimize it, and have just a single check... something like:

string[] validators = {ComponentsResources.NorthAmericaAssetNumValidationRegex, ....};
var compoundValidator = new Regex("(" + string.join(")|(", validators) + ")");

Then, in your code, you can reuse that single regex just once:

public bool IsValidAssetNum(string assetNum)
{
    return compoundValidator.Match(assetNum).Success;
}

There are any number of ways you can add validators to the the system, config files come to mind as being an easy way to manage them without code changes.

\$\endgroup\$
1
  • \$\begingroup\$ This is what I ended up doing. I now have a config section, AssetNumberValidators, with items, like: <Validator description="North American Asset Number" regex="..."> and then implement them as you describe. \$\endgroup\$
    – Tom
    May 15, 2015 at 19:11
3
\$\begingroup\$

There is a common responsability with each of your methods, which is to validate a string. Each of your method has the same signature which is :

bool IsValid(string asset);

Let's start with this, you should create an interface, let's call it IAssetFormatValidator.

public interface IAssetFormatValidator
{
    bool IsValid(string asset):
}

Now you can create 3 implementation of the interface, NorthAmericanFormatAssetValidator,GermanFormatAssetValidator,AustralianFormatAssetValidator. I'll write one for example :

public class NorthAmericanFormatAssetValidator : IAssetFormatValidator
{    
    public bool IsValid(string asset)
    {
        var regex = new Regex(ComponentsResources.NorthAmericaAssetNumValidationRegex);
        var match = regex.Match(assetNum);

        return match.Success;
    } 
}

You now have 3 classes, one interface which define your responsability. In the class that has the IsValidAssetNum method, you can add a List<IAssetFormatValidator> then loop through the validations in order to verify them all. You won't have more than one condition, and it will be easy to add other implementation of the interface.

//I don't have the name, consider adding it to your code to be reviewed :)
public class ThatClass 
{
    private List<IAssetFormatValidator> assetFormatValidators;

    public ThatClass()
    {
        assetFormatValidators = new List<IAssetFormatValidator>();
        assetFormatValidators.Add(new NorthAmericanFormatAssetValidator());
        //add the others in order.
    }

    public bool IsValidAssetNum(string assetNum)
    {
        //If the asset matches with one validator, it will return true
        return assetFormatValidators.Any(a => a.IsValid(assetNum);
    }
}

If you add the class containing IsValidAssetNum to your question, I'm pretty sure the reviews would be much more precise to your problem, consider adding it! :)

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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