CodeSOD: And Now You Have Two Problems

We all know the old saying: "Some people, when confronted with a problem, think 'I know, I'll use regular expressions.' Now they have two problems." The quote has a long and storied history, but Roger A's co-worker decided to take it quite literally.

Specifically, they wanted to be able to build validation rules which could apply a regular expression to the input. Thus, they wrote the RegExpConstraint class:

public class RegExpConstraint{ private readonly Regex _pattern; private readonly string _unmatchedErrorMessage; protected string UnmatchedErrorMessage => _unmatchedErrorMessage; public RegExpConstraint(string pattern, string unmatchedErrorMessage) { _pattern = new Regex(pattern); _unmatchedErrorMessage = unmatchedErrorMessage; } /// <summary> /// Check if the given value match the RegExp. Return the unmatched error message if it doesn't, null otherwise. /// </summary> public virtual string CheckMatch(string value) { if (!_pattern.IsMatch(value)) { return _unmatchedErrorMessage; } return null; }}

This "neatly" solved the problem of making sure that an input string matched a regex, if by "neatly" you mean, "returns a string instead of a boolean value", but it introduced a new problem: what if you wanted to make certain that it absolutely didn't match a certain subset of characters. For example, if you wanted "\:*<>|@" to be illegal characters, how could you do that with the RegExpConstraint? By writing a regex like this: [^\:*<>|@]? Don't be absurd. You need a new class.

public class RegExpExcludeConstraint : RegExpConstraint{ private Regex _antiRegex; public Regex AntiRegex => _antiRegex; public RegExpExcludeConstraint() : base(null, null) { } /// <summary> /// Constructor /// </summary> /// <param name="pattern">Regex expression to validate</param> /// <param name="antiPattern">Regex expression to invalidate</param> /// <param name="unmatchedErrorMessage">Error message in case of invalidation</param> public RegExpExcludeConstraint(string pattern, string antiPattern, string unmatchedErrorMessage) : base(pattern, unmatchedErrorMessage) { _antiRegex = new Regex(antiPattern); } /// <summary> /// Check if the constraint match /// </summary> public override string CheckMatch(string value) { var baseMatch = base.CheckMatch(value); if (baseMatch != null || _antiRegex.IsMatch(value)) { return UnmatchedErrorMessage; } return null; }}

Not only does this programmer not fully understand regular expressions, they also haven't fully mastered inheritance. Or maybe they know that this code is bad, as they named one of their parameters antiPattern. The RegExpExcludeConstraint accepts two regexes, requires that the first one matches, and the second one doesn't, helpfully continuing the pattern of returning null when there's nothing wrong with the input.

Perhaps the old saying is wrong. I don't see two problems. I see one problem: the person who wrote this code.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!