Versatile string validation












10












$begingroup$


I passed a technical test the other day, one part included a cheap string validation and I am wondering this can be further improved to be more versatile to requirement changes.



The requirements were something like that




Create a Validate method, which accepts a string and returns true if it's valid and false if it's not.



A string is valid if it satisfies the rules below:




  • The string must be at least 6 characters long and not exceed 16 characters.

  • The string must contain only letters, numbers and optionally one hyphen (-).

  • The string must start with a letter, and must not end with a hyphen.
    For example, validate("Michelle Belle"); would return false because it contains a space.




My solution was like that:



public static class ComparableExtensions
{
public static bool IsStrictlyLowerThan<TComparable>(this TComparable comparable, TComparable value)
where TComparable : IComparable<TComparable>
{
return comparable.CompareTo(value) < 0;
}

public static bool IsStrictlyGreaterThan<TComparable>(this TComparable comparable, TComparable value)
where TComparable : IComparable<TComparable>
{
return comparable.CompareTo(value) > 0;
}

public static bool IsStrictlyNotBetween<TComparable>(this TComparable comparable, TComparable lowerBound, TComparable upperBound)
where TComparable : IComparable<TComparable>
{
if (lowerBound.IsStrictlyGreaterThan(upperBound))
{
throw new ArgumentOutOfRangeException(nameof(lowerBound) + nameof(upperBound));
}

return comparable.IsStrictlyLowerThan(lowerBound) || comparable.IsStrictlyGreaterThan(upperBound);
}
}

public static class CharExtensions
{
public static bool IsLetterOrDigit(this char c)
{
return char.IsLetterOrDigit(c);
}

public static bool IsLetter(this char c)
{
return char.IsLetter(c);
}

public static bool IsHyphen(this char c)
{
return c == '-';
}
}

public class Test
{
public static bool Validate(string str)
{
if (str.Length.IsStrictlyNotBetween(6, 16))
{
return false;
}

if (!str.First().IsLetter() || str.Last().IsHyphen())
{
return false;
}

var hyphenCount = 0;

for (var i = 1; i < str.Length - 1; i++)
{
if (str[i].IsLetterOrDigit())
{
continue;
}
if (str[i].IsHyphen())
{
hyphenCount++;
if (hyphenCount > 1)
{
return false;
}
}
else
{
return false;
}
}

return true;
}
}


I purposefully decided to not go with Regular Expressions to keep the logic readable and I am wondering if my code can be further refactored to incorporate new business rules.










share|improve this question











$endgroup$








  • 1




    $begingroup$
    Would the regular expression for these rules really be that unreadable?
    $endgroup$
    – Kenneth K.
    Jan 28 at 15:52






  • 6




    $begingroup$
    Why do you have all of those "extentions"? They make the code way harder to read! IsHyphen? IsStrictlyGreaterThan? Why? Just write == '-' in the code itself.
    $endgroup$
    – Todd Sewell
    Jan 28 at 16:07






  • 2




    $begingroup$
    Your solution reminds me of the Enterprise Rules Engine. (That is not a good thing.)
    $endgroup$
    – jpmc26
    Jan 28 at 23:05






  • 1




    $begingroup$
    @EhouarnPerret yeah, please as little extra code as possible (YAGNI). those extension methods have similar names, so sure, they're kinda readable, but considering they do very little, I'd say you should leave them out in this case
    $endgroup$
    – somebody
    Jan 29 at 1:44






  • 1




    $begingroup$
    To me, this huge overkilling with the comparables and charextentions makes it very unreadable.
    $endgroup$
    – Tvde1
    Jan 29 at 12:38
















10












$begingroup$


I passed a technical test the other day, one part included a cheap string validation and I am wondering this can be further improved to be more versatile to requirement changes.



The requirements were something like that




Create a Validate method, which accepts a string and returns true if it's valid and false if it's not.



A string is valid if it satisfies the rules below:




  • The string must be at least 6 characters long and not exceed 16 characters.

  • The string must contain only letters, numbers and optionally one hyphen (-).

  • The string must start with a letter, and must not end with a hyphen.
    For example, validate("Michelle Belle"); would return false because it contains a space.




My solution was like that:



public static class ComparableExtensions
{
public static bool IsStrictlyLowerThan<TComparable>(this TComparable comparable, TComparable value)
where TComparable : IComparable<TComparable>
{
return comparable.CompareTo(value) < 0;
}

public static bool IsStrictlyGreaterThan<TComparable>(this TComparable comparable, TComparable value)
where TComparable : IComparable<TComparable>
{
return comparable.CompareTo(value) > 0;
}

public static bool IsStrictlyNotBetween<TComparable>(this TComparable comparable, TComparable lowerBound, TComparable upperBound)
where TComparable : IComparable<TComparable>
{
if (lowerBound.IsStrictlyGreaterThan(upperBound))
{
throw new ArgumentOutOfRangeException(nameof(lowerBound) + nameof(upperBound));
}

return comparable.IsStrictlyLowerThan(lowerBound) || comparable.IsStrictlyGreaterThan(upperBound);
}
}

public static class CharExtensions
{
public static bool IsLetterOrDigit(this char c)
{
return char.IsLetterOrDigit(c);
}

public static bool IsLetter(this char c)
{
return char.IsLetter(c);
}

public static bool IsHyphen(this char c)
{
return c == '-';
}
}

public class Test
{
public static bool Validate(string str)
{
if (str.Length.IsStrictlyNotBetween(6, 16))
{
return false;
}

if (!str.First().IsLetter() || str.Last().IsHyphen())
{
return false;
}

var hyphenCount = 0;

for (var i = 1; i < str.Length - 1; i++)
{
if (str[i].IsLetterOrDigit())
{
continue;
}
if (str[i].IsHyphen())
{
hyphenCount++;
if (hyphenCount > 1)
{
return false;
}
}
else
{
return false;
}
}

return true;
}
}


I purposefully decided to not go with Regular Expressions to keep the logic readable and I am wondering if my code can be further refactored to incorporate new business rules.










share|improve this question











$endgroup$








  • 1




    $begingroup$
    Would the regular expression for these rules really be that unreadable?
    $endgroup$
    – Kenneth K.
    Jan 28 at 15:52






  • 6




    $begingroup$
    Why do you have all of those "extentions"? They make the code way harder to read! IsHyphen? IsStrictlyGreaterThan? Why? Just write == '-' in the code itself.
    $endgroup$
    – Todd Sewell
    Jan 28 at 16:07






  • 2




    $begingroup$
    Your solution reminds me of the Enterprise Rules Engine. (That is not a good thing.)
    $endgroup$
    – jpmc26
    Jan 28 at 23:05






  • 1




    $begingroup$
    @EhouarnPerret yeah, please as little extra code as possible (YAGNI). those extension methods have similar names, so sure, they're kinda readable, but considering they do very little, I'd say you should leave them out in this case
    $endgroup$
    – somebody
    Jan 29 at 1:44






  • 1




    $begingroup$
    To me, this huge overkilling with the comparables and charextentions makes it very unreadable.
    $endgroup$
    – Tvde1
    Jan 29 at 12:38














10












10








10


3



$begingroup$


I passed a technical test the other day, one part included a cheap string validation and I am wondering this can be further improved to be more versatile to requirement changes.



The requirements were something like that




Create a Validate method, which accepts a string and returns true if it's valid and false if it's not.



A string is valid if it satisfies the rules below:




  • The string must be at least 6 characters long and not exceed 16 characters.

  • The string must contain only letters, numbers and optionally one hyphen (-).

  • The string must start with a letter, and must not end with a hyphen.
    For example, validate("Michelle Belle"); would return false because it contains a space.




My solution was like that:



public static class ComparableExtensions
{
public static bool IsStrictlyLowerThan<TComparable>(this TComparable comparable, TComparable value)
where TComparable : IComparable<TComparable>
{
return comparable.CompareTo(value) < 0;
}

public static bool IsStrictlyGreaterThan<TComparable>(this TComparable comparable, TComparable value)
where TComparable : IComparable<TComparable>
{
return comparable.CompareTo(value) > 0;
}

public static bool IsStrictlyNotBetween<TComparable>(this TComparable comparable, TComparable lowerBound, TComparable upperBound)
where TComparable : IComparable<TComparable>
{
if (lowerBound.IsStrictlyGreaterThan(upperBound))
{
throw new ArgumentOutOfRangeException(nameof(lowerBound) + nameof(upperBound));
}

return comparable.IsStrictlyLowerThan(lowerBound) || comparable.IsStrictlyGreaterThan(upperBound);
}
}

public static class CharExtensions
{
public static bool IsLetterOrDigit(this char c)
{
return char.IsLetterOrDigit(c);
}

public static bool IsLetter(this char c)
{
return char.IsLetter(c);
}

public static bool IsHyphen(this char c)
{
return c == '-';
}
}

public class Test
{
public static bool Validate(string str)
{
if (str.Length.IsStrictlyNotBetween(6, 16))
{
return false;
}

if (!str.First().IsLetter() || str.Last().IsHyphen())
{
return false;
}

var hyphenCount = 0;

for (var i = 1; i < str.Length - 1; i++)
{
if (str[i].IsLetterOrDigit())
{
continue;
}
if (str[i].IsHyphen())
{
hyphenCount++;
if (hyphenCount > 1)
{
return false;
}
}
else
{
return false;
}
}

return true;
}
}


I purposefully decided to not go with Regular Expressions to keep the logic readable and I am wondering if my code can be further refactored to incorporate new business rules.










share|improve this question











$endgroup$




I passed a technical test the other day, one part included a cheap string validation and I am wondering this can be further improved to be more versatile to requirement changes.



The requirements were something like that




Create a Validate method, which accepts a string and returns true if it's valid and false if it's not.



A string is valid if it satisfies the rules below:




  • The string must be at least 6 characters long and not exceed 16 characters.

  • The string must contain only letters, numbers and optionally one hyphen (-).

  • The string must start with a letter, and must not end with a hyphen.
    For example, validate("Michelle Belle"); would return false because it contains a space.




My solution was like that:



public static class ComparableExtensions
{
public static bool IsStrictlyLowerThan<TComparable>(this TComparable comparable, TComparable value)
where TComparable : IComparable<TComparable>
{
return comparable.CompareTo(value) < 0;
}

public static bool IsStrictlyGreaterThan<TComparable>(this TComparable comparable, TComparable value)
where TComparable : IComparable<TComparable>
{
return comparable.CompareTo(value) > 0;
}

public static bool IsStrictlyNotBetween<TComparable>(this TComparable comparable, TComparable lowerBound, TComparable upperBound)
where TComparable : IComparable<TComparable>
{
if (lowerBound.IsStrictlyGreaterThan(upperBound))
{
throw new ArgumentOutOfRangeException(nameof(lowerBound) + nameof(upperBound));
}

return comparable.IsStrictlyLowerThan(lowerBound) || comparable.IsStrictlyGreaterThan(upperBound);
}
}

public static class CharExtensions
{
public static bool IsLetterOrDigit(this char c)
{
return char.IsLetterOrDigit(c);
}

public static bool IsLetter(this char c)
{
return char.IsLetter(c);
}

public static bool IsHyphen(this char c)
{
return c == '-';
}
}

public class Test
{
public static bool Validate(string str)
{
if (str.Length.IsStrictlyNotBetween(6, 16))
{
return false;
}

if (!str.First().IsLetter() || str.Last().IsHyphen())
{
return false;
}

var hyphenCount = 0;

for (var i = 1; i < str.Length - 1; i++)
{
if (str[i].IsLetterOrDigit())
{
continue;
}
if (str[i].IsHyphen())
{
hyphenCount++;
if (hyphenCount > 1)
{
return false;
}
}
else
{
return false;
}
}

return true;
}
}


I purposefully decided to not go with Regular Expressions to keep the logic readable and I am wondering if my code can be further refactored to incorporate new business rules.







c# interview-questions validation






share|improve this question















share|improve this question













share|improve this question




share|improve this question








edited Jan 28 at 15:32









t3chb0t

35k753125




35k753125










asked Jan 28 at 13:52









Ehouarn PerretEhouarn Perret

279111




279111








  • 1




    $begingroup$
    Would the regular expression for these rules really be that unreadable?
    $endgroup$
    – Kenneth K.
    Jan 28 at 15:52






  • 6




    $begingroup$
    Why do you have all of those "extentions"? They make the code way harder to read! IsHyphen? IsStrictlyGreaterThan? Why? Just write == '-' in the code itself.
    $endgroup$
    – Todd Sewell
    Jan 28 at 16:07






  • 2




    $begingroup$
    Your solution reminds me of the Enterprise Rules Engine. (That is not a good thing.)
    $endgroup$
    – jpmc26
    Jan 28 at 23:05






  • 1




    $begingroup$
    @EhouarnPerret yeah, please as little extra code as possible (YAGNI). those extension methods have similar names, so sure, they're kinda readable, but considering they do very little, I'd say you should leave them out in this case
    $endgroup$
    – somebody
    Jan 29 at 1:44






  • 1




    $begingroup$
    To me, this huge overkilling with the comparables and charextentions makes it very unreadable.
    $endgroup$
    – Tvde1
    Jan 29 at 12:38














  • 1




    $begingroup$
    Would the regular expression for these rules really be that unreadable?
    $endgroup$
    – Kenneth K.
    Jan 28 at 15:52






  • 6




    $begingroup$
    Why do you have all of those "extentions"? They make the code way harder to read! IsHyphen? IsStrictlyGreaterThan? Why? Just write == '-' in the code itself.
    $endgroup$
    – Todd Sewell
    Jan 28 at 16:07






  • 2




    $begingroup$
    Your solution reminds me of the Enterprise Rules Engine. (That is not a good thing.)
    $endgroup$
    – jpmc26
    Jan 28 at 23:05






  • 1




    $begingroup$
    @EhouarnPerret yeah, please as little extra code as possible (YAGNI). those extension methods have similar names, so sure, they're kinda readable, but considering they do very little, I'd say you should leave them out in this case
    $endgroup$
    – somebody
    Jan 29 at 1:44






  • 1




    $begingroup$
    To me, this huge overkilling with the comparables and charextentions makes it very unreadable.
    $endgroup$
    – Tvde1
    Jan 29 at 12:38








1




1




$begingroup$
Would the regular expression for these rules really be that unreadable?
$endgroup$
– Kenneth K.
Jan 28 at 15:52




$begingroup$
Would the regular expression for these rules really be that unreadable?
$endgroup$
– Kenneth K.
Jan 28 at 15:52




6




6




$begingroup$
Why do you have all of those "extentions"? They make the code way harder to read! IsHyphen? IsStrictlyGreaterThan? Why? Just write == '-' in the code itself.
$endgroup$
– Todd Sewell
Jan 28 at 16:07




$begingroup$
Why do you have all of those "extentions"? They make the code way harder to read! IsHyphen? IsStrictlyGreaterThan? Why? Just write == '-' in the code itself.
$endgroup$
– Todd Sewell
Jan 28 at 16:07




2




2




$begingroup$
Your solution reminds me of the Enterprise Rules Engine. (That is not a good thing.)
$endgroup$
– jpmc26
Jan 28 at 23:05




$begingroup$
Your solution reminds me of the Enterprise Rules Engine. (That is not a good thing.)
$endgroup$
– jpmc26
Jan 28 at 23:05




1




1




$begingroup$
@EhouarnPerret yeah, please as little extra code as possible (YAGNI). those extension methods have similar names, so sure, they're kinda readable, but considering they do very little, I'd say you should leave them out in this case
$endgroup$
– somebody
Jan 29 at 1:44




$begingroup$
@EhouarnPerret yeah, please as little extra code as possible (YAGNI). those extension methods have similar names, so sure, they're kinda readable, but considering they do very little, I'd say you should leave them out in this case
$endgroup$
– somebody
Jan 29 at 1:44




1




1




$begingroup$
To me, this huge overkilling with the comparables and charextentions makes it very unreadable.
$endgroup$
– Tvde1
Jan 29 at 12:38




$begingroup$
To me, this huge overkilling with the comparables and charextentions makes it very unreadable.
$endgroup$
– Tvde1
Jan 29 at 12:38










7 Answers
7






active

oldest

votes


















7












$begingroup$

Not much to say about the extensions methods, as they are mostly wrappers.



However if you're looking for ways to make the algorithm more readable, LINQ is your friend. You can replace most of your logic with a one-liner:




var hyphenCount = 0;

for (var i = 1; i < str.Length - 1; i++)
{
if (str[i].IsLetterOrDigit())
{
continue;
}
if (str[i].IsHyphen())
{
hyphenCount++;
if (hyphenCount > 1)
{
return false;
}
}
else
{
return false;
}
}

return true;



Like this:



return str.All(c => c.IsHyphen() || c.IsLetterOrDigit()) && str.Count(c => c.IsHyphen()) <= 1;


Which more clearly explains your intent, you can also move those expression to their separate methods to make it as readable as possible, this way you can keep adding new conditions, without modifying the existing logic (unless they interfere with one another that is).






share|improve this answer









$endgroup$





















    8












    $begingroup$


    • There's a bug: you're not checking if the last character is a letter or digit, only that it isn't a hyphen, so this fails to reject "abcdef&". Denis' solution may be less efficient (2 iterations instead of 1), but with at most 16 characters that's not much of a concern, and it's both easier to read and it works correctly.

    • The first two rules read very nicely. I especially like that the business rules are translated to code almost 1:1, that will make updating easier. However, I do think those extension methods are over-engineered. str.Length < 6 || str.Length > 16 and !char.IsLetter(str.First()) || str.Last() == '-' is already quite readable, and that doesn't require extra code that needs to be understood and maintained.

    • You can use => syntax for methods with single-expression bodies.






    share|improve this answer









    $endgroup$













    • $begingroup$
      I agree on the extension methods part, another positive for using standard types and methods is that, experienced developers, know right away the way they function.
      $endgroup$
      – Denis
      Jan 28 at 15:05










    • $begingroup$
      @Pieter Witvoet true, my bad, wrong copy and paste =/
      $endgroup$
      – Ehouarn Perret
      Jan 28 at 15:13



















    6












    $begingroup$

    Little bit late but let me add another alternative solution, t3chb0t already took care of your code.



    You have extension methods for IComparable<T> and Char, this forces you to write your business logic in code instead of a (pseudo) high-level language which resembles your business rules. If you do not want to use a regular expression (I'd use it) then you should try to match 1:1 the language of your requirements:



    StringValidator.Validate(str)
    .LengthIsInRange(6, 16)
    .ContainsOnly(Matches.LetterOrDigits, Matches.Hyphen).HasAtMost(1, Matches.Hyphen)
    .StartsWith(Matches.Letter)
    .DoesNotEndWith(Matches.Hyphen);


    This is simple enough to be self-describing. Does it hurt performance? Maybe but it's seldom a problem in LoB applications and business rules can become incredibly convoluted. Is it a problem? Measure it, for performance critical code it's easy to write an extension method (for StringValidator) to perform a specific task.



    Note that I'm using an hypothetical Matches.LetterOrDigits() function with string -> bool signature instead of, let's say, Char.IsLetterOrDigit() with char -> bool. Why? Because not every character (OK, I won't repeat this again...) is a single Char then you have to compare strings (few examples: à or or 𠀑).



    Your requirements are, probably, vague and your code broken regardless the interpretation you picked. What do they mean with letter? An US-ASCII Latin character? In that case your code is wrong because ā (LATIN SMALL LETTER A MACRON) is accepted. Is is a letter everything in the Letter Unicode category (or even just in the Latin Extended Unicode block)? Your code is wrong again because (LATIN SMALL LETTER A + COMBINING MACRON) is rejected. This very example can be solved normalizing the string before starting the validation but it's not always that easy.



    Imagine that instead of a single validation rule you have...1,000 of them. Or 10,000. I'm not a fan of extension method for primitive types (especially when they're trivial) but the mistake (IMHO) is to work with a too low level of abstraction: you do not want to extend String.Length to replicate what > does: you want to extend String to support high level composable and chainable assertions.



    Note: ContainsOnly() may even accept regexes, pseudo code:



    static ContainsOnly(this StringValidator validator, ...string matches) => ...

    static class Matches {
    public static string LetterOrDigit = "a-zA-Z0-9";
    }


    Composition should now be fast enough even when you cannot really ignore performance (and you can always have a MatchRegEx() extension method).





    How to extend this? In previous example I assumed a pseudo-implementation like this:



    public StringValidator LengthInRange(int min, int max) {
    if (IsValid) {
    IsValid = Value.Length >= min && Value.Length <= max;
    }

    return this;
    }


    You can easily extend it to run all validators to generate a list of errors:



    var result = StringValidator.Validate(str)
    .LengthIsInRange(6, 16)
    .StartsWith(Matches.Letter)
    .DoesNotEndWith(Matches.Hyphen);

    if (result.HasErrors)
    Console.WriteLine(String.Join(Environment.NewLine, result.Errors));


    Or to throw an exception:



    StringValidator.Validate(str)
    .LengthIsInRange(6, 16)
    .StartsWith(Matches.Letter)
    .DoesNotEndWith(Matches.Hyphen)
    .ThrowIfInvalid();


    I think you've got the point.






    share|improve this answer











    $endgroup$









    • 1




      $begingroup$
      So if, for example, length is not between 6 and 16 this throws an exception?
      $endgroup$
      – Kenneth K.
      Jan 28 at 18:27








    • 1




      $begingroup$
      No as per requirements it should return false (the other validators simply fall through after the first one marked the result as invalid?). You can throw explicitly or with an option in Configure() or with a call to ThrowIfInvalid(). For a future requirement it may add an IEnumerable<string> property to collect all errors (for example)
      $endgroup$
      – Adriano Repetti
      Jan 28 at 19:09



















    5












    $begingroup$

    I think you've seen enough alternative solutions so I'll just review your code.




    • You should not invent new terms for x < 0 like IsStrictlyLowerThan as this makes things unnecessarily more difficult to understand. Instead, you should stick to the well known naming. In this case that would be IsLessThan or in case of x <= 0 it would be IsLessThanOrEqual - Everyone who write code would understand these without any further explanation. Or in case of IsStrictlyNotBetween should be IsNotInRangeExclusive etc.

    • There are voices in comments that suggest using == '-' instead of your extensions. I don't agree with that. Your extensions are much better because they make the code look clean and consistent. Having extensions for some operations and not for others would make it look dirty.

    • I don't however agree with your decision for not using Regex. With regex it's usually simpler and easier to express complex validations. It's also easier to make your validations case-insensitive or match other patterns or use such services as regex101.com to test them. It might be enough for now but sooner or later you will need it so don't be so strict about always consider other solutions.


    • Keep also in mind that your IsStrictlyLowerThan APIs are currently case sensitive so you might consider using IEqualityComparer<string> as the last argument and not the TComparable that doesn't offer you any additional functionality in this context. So your API should have signatures similar to this one:



      public static bool IsLessThan(this string value, string other, IComparer<string> comaprer = default)
      {
      return (comaprer ?? StringComparer.Ordinal).Compare(value, other) < 0;
      }


      Then you can use it with other comparers if necessary, e.g.:



      "FOO".IsLessThan("foo", StringComparer.OrdinalIgnoreCase)







    share|improve this answer









    $endgroup$





















      2












      $begingroup$

      N.B. I thought I'd answer the original test question myself in F# as a Kata, so this is a functional suggestion based on what I found.



      I'll only talk about the versatility part of the solution, others have touched on using Linq and I think your code is already quite readable.





      If you can divide the original requirement into a number of smaller rules, where each rule has the same "Shape".



      This "Shape" is take a sequence of characters and return a Boolean.



      So in C# we'd have:



      bool LengthIsValid(string s) => s.Length >= 6 && s.Length <= 16;

      bool Rule2(string s) => true; //e.g must start with Letter, or doesn't with with '-'


      Now that we have each rule in the same "Shape", you can create a composite set of rules and validate them all together:



      var rules = new List<Func<string, bool>> 
      {
      LengthIsValid,
      Rule2,
      //etc.
      };


      Checking the rules is then just a matter of applying those rules to the input string:



      bool CheckAllRules (string s) =>
      rules
      .All(rule => rule(s));


      With this approach you get versatility from been able to create any number of composition of rules.



      e.g. Creating Rules where the powerUsers don't need to check the length (I can be root).



      IEnumerable<Func<string, bool>> CreateRules(bool powerUser)
      {
      if (!powerUser)
      yield return LengthIsValid;

      yield return Rule2;
      }



      The F# version if you're interested in here






      share|improve this answer











      $endgroup$













      • $begingroup$
        In C# you might want to write rules.All(x => x(s)) (unless you want to evaluate all the rules without stopping with the first failed one).
        $endgroup$
        – Adriano Repetti
        Jan 28 at 23:33










      • $begingroup$
        It's OK, because LINQ is lazy evaluated "All" will only keep pulling whilst the predicate returns "true", as soon the result of the predicate is "false" it will stop iterating through the rules. The "select" code only executes when "All" asks for the one more item. Try changing it to .Select(rule => { Console.WriteLine("test"); return rule(s); }) and you will only see one "test" if the first rule fails.
        $endgroup$
        – DaveShaw
        Jan 28 at 23:47










      • $begingroup$
        Yep, I worded that very poorly. The two parts of my comments were unrelated. 1) you don't need Select() and 2) unless you want to extract them all (but in that case you obviously need ToArray() or similar before All()).
        $endgroup$
        – Adriano Repetti
        Jan 28 at 23:59










      • $begingroup$
        Yes, I have a habit of using .Select and .Where followed by .Any/All/Single.First/etc. - it usually makes things clearer for me :) In this case, I think you're correct that just .All would suffice.
        $endgroup$
        – DaveShaw
        Jan 29 at 0:07



















      1












      $begingroup$

      About versatility my thought is a separate class to stipulate the different possible rules:



      class ValidationRules
      {
      public const char HYPHEN = '-';
      public readonly int hyphenCount;
      public readonly bool needUpper;
      public readonly bool needLower;
      public readonly bool needDigit;
      public readonly bool allowSpaces;
      public readonly int minLength;
      public readonly int maxLength;
      /// <summary>
      /// Constructor with min and max length and default rules:
      /// needUpper = true
      /// needLower = true
      /// allowSpaces = false
      /// hyphenCount = 1;
      /// </summary>
      public ValidationRules(int minLength, int maxLength)
      {
      this.minLength = minLength;
      this.maxLength = maxLength;
      hyphenCount = 1;
      needLower = true;
      needUpper = true;
      needDigit = true;
      allowSpaces = false;
      }
      /// <summary>
      /// Constructor with min and max length and supplied rules:
      /// </summary>
      public ValidationRules(int minLength, int maxLength, int hyphenCount, bool needUpper, bool needLower, bool needDigit, bool allowSpaces)
      {
      this.minLength = minLength;
      this.maxLength = maxLength;
      this.hyphenCount = hyphenCount;
      this.needLower = needLower;
      this.needUpper = needUpper;
      this.needDigit = needDigit;
      this.allowSpaces = allowSpaces;
      }
      }


      the method to validate is still quite simple:



      /// <summary>
      /// Validate string according to validation rules
      /// </summary>
      /// <returns></returns>
      public static bool Validate(string input,ValidationRules rules)
      {
      if(input.Length < rules.minLength || input.Length > rules.maxLength)
      {
      return false;
      }
      if(!Char.IsLetter(input[0]) || input[input.Length-1] == ValidationRules.HYPHEN)
      {
      return false;
      }
      return input.Count(x => x == ValidationRules.HYPHEN) <= rules.hyphenCount && input.All(x =>
      (rules.needUpper && char.IsUpper(x)) ||
      (rules.needLower && char.IsLower(x)) ||
      (rules.allowSpaces && char.IsWhiteSpace(x)) ||
      (rules.needDigit && char.IsDigit(x)) ||
      (x == ValidationRules.HYPHEN));
      }





      share|improve this answer











      $endgroup$













      • $begingroup$
        I like your approach!
        $endgroup$
        – Ehouarn Perret
        Jan 28 at 15:11










      • $begingroup$
        For an approach that targets extendability, it will quickly become a mini god class, requiring changes on several different locations. Multiple types with a single visitor would be much better off SOLID and elegance-wise.
        $endgroup$
        – Denis
        Jan 28 at 15:17










      • $begingroup$
        @Denis actually I was thinking about a set of rules that could be built out of a builder: github.com/JeremySkinner/FluentValidation
        $endgroup$
        – Ehouarn Perret
        Jan 28 at 15:27






      • 2




        $begingroup$
        @EhouarnPerret That's similar to what I meant, but this implementation would add way too much overhead for such a simple task, if it were an interview question, you should also consider time. Show off some techniques but don't overdo it.
        $endgroup$
        – Denis
        Jan 28 at 15:30



















      1












      $begingroup$

      Stop it. Just stop it. Unless you have some kind of unusual performance requirements, keep it simple:



      /// <summary>
      /// Regular expression pattern that matches strings containing only Unicode
      /// letters, digits, and hyphens.
      /// </summary>
      public static Regex AllUnicodeLettersNumsHyphen = new Regex(@"^[p{L}p{Nd}-]*$");

      public static bool IsValid(string s)
      {
      return (
      s.Length >= 6 && s.Length <= 16
      && AllUnicodeLettersNumsHyphen.IsMatch(s)
      && s.Count(c => c == '-') <= 1
      && Char.IsLetter(s, 0)
      && s[s.Length - 1] != '-'
      );
      }


      (Those parentheses are optional. I find them visually appealing. Follow your group's coding guidelines on style choices like that.)



      Given the requirements you specify, there's no reason for loops, extension methods, or any of that stuff. I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. The smaller the scope the better when you're reading. It will take someone 30 seconds to fully understand these 6 lines of code. It will take 10 minutes to dive through all yours.



      And this isn't any less flexible. In fact it's more so. You can trivially add new rules to this 6 line method. That is not the case with your code. Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes.



      And look how I dealt with the regular expression impenetrability:




      • Encode as many rules as possible outside the regex.

      • Only use one fairly simple, appropriately named and documented regex for just the one check: that it matches the character classes. That and a comment are all you need.




      If you are completely dead set against regular expressions, consider this LINQ based alternative:



      public static bool IsValid(string s)
      {
      return (
      s.Length >= 6 && s.Length <= 16
      && s.All(c => Char.IsLetterOrDigit(c) || '-' == c)
      && s.Count(c => c == '-') <= 1
      && Char.IsLetter(s, 0)
      && s[s.Length - 1] != '-'
      );
      }





      share|improve this answer











      $endgroup$













      • $begingroup$
        I'd tend to agree to use a single regex but in that case everything can be expressed with a single regex without any other surrounding code (including the length and the first/last character)
        $endgroup$
        – Adriano Repetti
        Jan 28 at 23:30






      • 1




        $begingroup$
        Hmmmmm POV, I guess. To me a well written regex can express clearly the requirement while UnicodeLettersNumsHyphen has the disadvantages of regexes (less easy to write and read) without its advantages (concise, fast and complete). Nitpick: I'd change UnicodeLettersNumsHyphen to a meaningful name because you can't understand what the regex is for without fully decoding it.
        $endgroup$
        – Adriano Repetti
        Jan 28 at 23:38






      • 2




        $begingroup$
        I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. - this is so not true. I wouldn't let this code pass to production.
        $endgroup$
        – t3chb0t
        Jan 29 at 6:21






      • 1




        $begingroup$
        Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes. - not that has to be tested but one that can be tested. Your code isn't testable and is so easy to make a mistake there that you wouldn't notice until something goes wrong for the client. This isn't an improvement. It's a great degradation of both readability and testability.
        $endgroup$
        – t3chb0t
        Jan 29 at 6:23








      • 3




        $begingroup$
        @t3chb0t This is testable...
        $endgroup$
        – Tvde1
        Jan 29 at 12:44












      Your Answer





      StackExchange.ifUsing("editor", function () {
      return StackExchange.using("mathjaxEditing", function () {
      StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix) {
      StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
      });
      });
      }, "mathjax-editing");

      StackExchange.ifUsing("editor", function () {
      StackExchange.using("externalEditor", function () {
      StackExchange.using("snippets", function () {
      StackExchange.snippets.init();
      });
      });
      }, "code-snippets");

      StackExchange.ready(function() {
      var channelOptions = {
      tags: "".split(" "),
      id: "196"
      };
      initTagRenderer("".split(" "), "".split(" "), channelOptions);

      StackExchange.using("externalEditor", function() {
      // Have to fire editor after snippets, if snippets enabled
      if (StackExchange.settings.snippets.snippetsEnabled) {
      StackExchange.using("snippets", function() {
      createEditor();
      });
      }
      else {
      createEditor();
      }
      });

      function createEditor() {
      StackExchange.prepareEditor({
      heartbeatType: 'answer',
      autoActivateHeartbeat: false,
      convertImagesToLinks: false,
      noModals: true,
      showLowRepImageUploadWarning: true,
      reputationToPostImages: null,
      bindNavPrevention: true,
      postfix: "",
      imageUploader: {
      brandingHtml: "Powered by u003ca class="icon-imgur-white" href="https://imgur.com/"u003eu003c/au003e",
      contentPolicyHtml: "User contributions licensed under u003ca href="https://creativecommons.org/licenses/by-sa/3.0/"u003ecc by-sa 3.0 with attribution requiredu003c/au003e u003ca href="https://stackoverflow.com/legal/content-policy"u003e(content policy)u003c/au003e",
      allowUrls: true
      },
      onDemand: true,
      discardSelector: ".discard-answer"
      ,immediatelyShowMarkdownHelp:true
      });


      }
      });














      draft saved

      draft discarded


















      StackExchange.ready(
      function () {
      StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f212381%2fversatile-string-validation%23new-answer', 'question_page');
      }
      );

      Post as a guest















      Required, but never shown

























      7 Answers
      7






      active

      oldest

      votes








      7 Answers
      7






      active

      oldest

      votes









      active

      oldest

      votes






      active

      oldest

      votes









      7












      $begingroup$

      Not much to say about the extensions methods, as they are mostly wrappers.



      However if you're looking for ways to make the algorithm more readable, LINQ is your friend. You can replace most of your logic with a one-liner:




      var hyphenCount = 0;

      for (var i = 1; i < str.Length - 1; i++)
      {
      if (str[i].IsLetterOrDigit())
      {
      continue;
      }
      if (str[i].IsHyphen())
      {
      hyphenCount++;
      if (hyphenCount > 1)
      {
      return false;
      }
      }
      else
      {
      return false;
      }
      }

      return true;



      Like this:



      return str.All(c => c.IsHyphen() || c.IsLetterOrDigit()) && str.Count(c => c.IsHyphen()) <= 1;


      Which more clearly explains your intent, you can also move those expression to their separate methods to make it as readable as possible, this way you can keep adding new conditions, without modifying the existing logic (unless they interfere with one another that is).






      share|improve this answer









      $endgroup$


















        7












        $begingroup$

        Not much to say about the extensions methods, as they are mostly wrappers.



        However if you're looking for ways to make the algorithm more readable, LINQ is your friend. You can replace most of your logic with a one-liner:




        var hyphenCount = 0;

        for (var i = 1; i < str.Length - 1; i++)
        {
        if (str[i].IsLetterOrDigit())
        {
        continue;
        }
        if (str[i].IsHyphen())
        {
        hyphenCount++;
        if (hyphenCount > 1)
        {
        return false;
        }
        }
        else
        {
        return false;
        }
        }

        return true;



        Like this:



        return str.All(c => c.IsHyphen() || c.IsLetterOrDigit()) && str.Count(c => c.IsHyphen()) <= 1;


        Which more clearly explains your intent, you can also move those expression to their separate methods to make it as readable as possible, this way you can keep adding new conditions, without modifying the existing logic (unless they interfere with one another that is).






        share|improve this answer









        $endgroup$
















          7












          7








          7





          $begingroup$

          Not much to say about the extensions methods, as they are mostly wrappers.



          However if you're looking for ways to make the algorithm more readable, LINQ is your friend. You can replace most of your logic with a one-liner:




          var hyphenCount = 0;

          for (var i = 1; i < str.Length - 1; i++)
          {
          if (str[i].IsLetterOrDigit())
          {
          continue;
          }
          if (str[i].IsHyphen())
          {
          hyphenCount++;
          if (hyphenCount > 1)
          {
          return false;
          }
          }
          else
          {
          return false;
          }
          }

          return true;



          Like this:



          return str.All(c => c.IsHyphen() || c.IsLetterOrDigit()) && str.Count(c => c.IsHyphen()) <= 1;


          Which more clearly explains your intent, you can also move those expression to their separate methods to make it as readable as possible, this way you can keep adding new conditions, without modifying the existing logic (unless they interfere with one another that is).






          share|improve this answer









          $endgroup$



          Not much to say about the extensions methods, as they are mostly wrappers.



          However if you're looking for ways to make the algorithm more readable, LINQ is your friend. You can replace most of your logic with a one-liner:




          var hyphenCount = 0;

          for (var i = 1; i < str.Length - 1; i++)
          {
          if (str[i].IsLetterOrDigit())
          {
          continue;
          }
          if (str[i].IsHyphen())
          {
          hyphenCount++;
          if (hyphenCount > 1)
          {
          return false;
          }
          }
          else
          {
          return false;
          }
          }

          return true;



          Like this:



          return str.All(c => c.IsHyphen() || c.IsLetterOrDigit()) && str.Count(c => c.IsHyphen()) <= 1;


          Which more clearly explains your intent, you can also move those expression to their separate methods to make it as readable as possible, this way you can keep adding new conditions, without modifying the existing logic (unless they interfere with one another that is).







          share|improve this answer












          share|improve this answer



          share|improve this answer










          answered Jan 28 at 14:27









          DenisDenis

          6,39221856




          6,39221856

























              8












              $begingroup$


              • There's a bug: you're not checking if the last character is a letter or digit, only that it isn't a hyphen, so this fails to reject "abcdef&". Denis' solution may be less efficient (2 iterations instead of 1), but with at most 16 characters that's not much of a concern, and it's both easier to read and it works correctly.

              • The first two rules read very nicely. I especially like that the business rules are translated to code almost 1:1, that will make updating easier. However, I do think those extension methods are over-engineered. str.Length < 6 || str.Length > 16 and !char.IsLetter(str.First()) || str.Last() == '-' is already quite readable, and that doesn't require extra code that needs to be understood and maintained.

              • You can use => syntax for methods with single-expression bodies.






              share|improve this answer









              $endgroup$













              • $begingroup$
                I agree on the extension methods part, another positive for using standard types and methods is that, experienced developers, know right away the way they function.
                $endgroup$
                – Denis
                Jan 28 at 15:05










              • $begingroup$
                @Pieter Witvoet true, my bad, wrong copy and paste =/
                $endgroup$
                – Ehouarn Perret
                Jan 28 at 15:13
















              8












              $begingroup$


              • There's a bug: you're not checking if the last character is a letter or digit, only that it isn't a hyphen, so this fails to reject "abcdef&". Denis' solution may be less efficient (2 iterations instead of 1), but with at most 16 characters that's not much of a concern, and it's both easier to read and it works correctly.

              • The first two rules read very nicely. I especially like that the business rules are translated to code almost 1:1, that will make updating easier. However, I do think those extension methods are over-engineered. str.Length < 6 || str.Length > 16 and !char.IsLetter(str.First()) || str.Last() == '-' is already quite readable, and that doesn't require extra code that needs to be understood and maintained.

              • You can use => syntax for methods with single-expression bodies.






              share|improve this answer









              $endgroup$













              • $begingroup$
                I agree on the extension methods part, another positive for using standard types and methods is that, experienced developers, know right away the way they function.
                $endgroup$
                – Denis
                Jan 28 at 15:05










              • $begingroup$
                @Pieter Witvoet true, my bad, wrong copy and paste =/
                $endgroup$
                – Ehouarn Perret
                Jan 28 at 15:13














              8












              8








              8





              $begingroup$


              • There's a bug: you're not checking if the last character is a letter or digit, only that it isn't a hyphen, so this fails to reject "abcdef&". Denis' solution may be less efficient (2 iterations instead of 1), but with at most 16 characters that's not much of a concern, and it's both easier to read and it works correctly.

              • The first two rules read very nicely. I especially like that the business rules are translated to code almost 1:1, that will make updating easier. However, I do think those extension methods are over-engineered. str.Length < 6 || str.Length > 16 and !char.IsLetter(str.First()) || str.Last() == '-' is already quite readable, and that doesn't require extra code that needs to be understood and maintained.

              • You can use => syntax for methods with single-expression bodies.






              share|improve this answer









              $endgroup$




              • There's a bug: you're not checking if the last character is a letter or digit, only that it isn't a hyphen, so this fails to reject "abcdef&". Denis' solution may be less efficient (2 iterations instead of 1), but with at most 16 characters that's not much of a concern, and it's both easier to read and it works correctly.

              • The first two rules read very nicely. I especially like that the business rules are translated to code almost 1:1, that will make updating easier. However, I do think those extension methods are over-engineered. str.Length < 6 || str.Length > 16 and !char.IsLetter(str.First()) || str.Last() == '-' is already quite readable, and that doesn't require extra code that needs to be understood and maintained.

              • You can use => syntax for methods with single-expression bodies.







              share|improve this answer












              share|improve this answer



              share|improve this answer










              answered Jan 28 at 14:53









              Pieter WitvoetPieter Witvoet

              6,444826




              6,444826












              • $begingroup$
                I agree on the extension methods part, another positive for using standard types and methods is that, experienced developers, know right away the way they function.
                $endgroup$
                – Denis
                Jan 28 at 15:05










              • $begingroup$
                @Pieter Witvoet true, my bad, wrong copy and paste =/
                $endgroup$
                – Ehouarn Perret
                Jan 28 at 15:13


















              • $begingroup$
                I agree on the extension methods part, another positive for using standard types and methods is that, experienced developers, know right away the way they function.
                $endgroup$
                – Denis
                Jan 28 at 15:05










              • $begingroup$
                @Pieter Witvoet true, my bad, wrong copy and paste =/
                $endgroup$
                – Ehouarn Perret
                Jan 28 at 15:13
















              $begingroup$
              I agree on the extension methods part, another positive for using standard types and methods is that, experienced developers, know right away the way they function.
              $endgroup$
              – Denis
              Jan 28 at 15:05




              $begingroup$
              I agree on the extension methods part, another positive for using standard types and methods is that, experienced developers, know right away the way they function.
              $endgroup$
              – Denis
              Jan 28 at 15:05












              $begingroup$
              @Pieter Witvoet true, my bad, wrong copy and paste =/
              $endgroup$
              – Ehouarn Perret
              Jan 28 at 15:13




              $begingroup$
              @Pieter Witvoet true, my bad, wrong copy and paste =/
              $endgroup$
              – Ehouarn Perret
              Jan 28 at 15:13











              6












              $begingroup$

              Little bit late but let me add another alternative solution, t3chb0t already took care of your code.



              You have extension methods for IComparable<T> and Char, this forces you to write your business logic in code instead of a (pseudo) high-level language which resembles your business rules. If you do not want to use a regular expression (I'd use it) then you should try to match 1:1 the language of your requirements:



              StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .ContainsOnly(Matches.LetterOrDigits, Matches.Hyphen).HasAtMost(1, Matches.Hyphen)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen);


              This is simple enough to be self-describing. Does it hurt performance? Maybe but it's seldom a problem in LoB applications and business rules can become incredibly convoluted. Is it a problem? Measure it, for performance critical code it's easy to write an extension method (for StringValidator) to perform a specific task.



              Note that I'm using an hypothetical Matches.LetterOrDigits() function with string -> bool signature instead of, let's say, Char.IsLetterOrDigit() with char -> bool. Why? Because not every character (OK, I won't repeat this again...) is a single Char then you have to compare strings (few examples: à or or 𠀑).



              Your requirements are, probably, vague and your code broken regardless the interpretation you picked. What do they mean with letter? An US-ASCII Latin character? In that case your code is wrong because ā (LATIN SMALL LETTER A MACRON) is accepted. Is is a letter everything in the Letter Unicode category (or even just in the Latin Extended Unicode block)? Your code is wrong again because (LATIN SMALL LETTER A + COMBINING MACRON) is rejected. This very example can be solved normalizing the string before starting the validation but it's not always that easy.



              Imagine that instead of a single validation rule you have...1,000 of them. Or 10,000. I'm not a fan of extension method for primitive types (especially when they're trivial) but the mistake (IMHO) is to work with a too low level of abstraction: you do not want to extend String.Length to replicate what > does: you want to extend String to support high level composable and chainable assertions.



              Note: ContainsOnly() may even accept regexes, pseudo code:



              static ContainsOnly(this StringValidator validator, ...string matches) => ...

              static class Matches {
              public static string LetterOrDigit = "a-zA-Z0-9";
              }


              Composition should now be fast enough even when you cannot really ignore performance (and you can always have a MatchRegEx() extension method).





              How to extend this? In previous example I assumed a pseudo-implementation like this:



              public StringValidator LengthInRange(int min, int max) {
              if (IsValid) {
              IsValid = Value.Length >= min && Value.Length <= max;
              }

              return this;
              }


              You can easily extend it to run all validators to generate a list of errors:



              var result = StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen);

              if (result.HasErrors)
              Console.WriteLine(String.Join(Environment.NewLine, result.Errors));


              Or to throw an exception:



              StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen)
              .ThrowIfInvalid();


              I think you've got the point.






              share|improve this answer











              $endgroup$









              • 1




                $begingroup$
                So if, for example, length is not between 6 and 16 this throws an exception?
                $endgroup$
                – Kenneth K.
                Jan 28 at 18:27








              • 1




                $begingroup$
                No as per requirements it should return false (the other validators simply fall through after the first one marked the result as invalid?). You can throw explicitly or with an option in Configure() or with a call to ThrowIfInvalid(). For a future requirement it may add an IEnumerable<string> property to collect all errors (for example)
                $endgroup$
                – Adriano Repetti
                Jan 28 at 19:09
















              6












              $begingroup$

              Little bit late but let me add another alternative solution, t3chb0t already took care of your code.



              You have extension methods for IComparable<T> and Char, this forces you to write your business logic in code instead of a (pseudo) high-level language which resembles your business rules. If you do not want to use a regular expression (I'd use it) then you should try to match 1:1 the language of your requirements:



              StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .ContainsOnly(Matches.LetterOrDigits, Matches.Hyphen).HasAtMost(1, Matches.Hyphen)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen);


              This is simple enough to be self-describing. Does it hurt performance? Maybe but it's seldom a problem in LoB applications and business rules can become incredibly convoluted. Is it a problem? Measure it, for performance critical code it's easy to write an extension method (for StringValidator) to perform a specific task.



              Note that I'm using an hypothetical Matches.LetterOrDigits() function with string -> bool signature instead of, let's say, Char.IsLetterOrDigit() with char -> bool. Why? Because not every character (OK, I won't repeat this again...) is a single Char then you have to compare strings (few examples: à or or 𠀑).



              Your requirements are, probably, vague and your code broken regardless the interpretation you picked. What do they mean with letter? An US-ASCII Latin character? In that case your code is wrong because ā (LATIN SMALL LETTER A MACRON) is accepted. Is is a letter everything in the Letter Unicode category (or even just in the Latin Extended Unicode block)? Your code is wrong again because (LATIN SMALL LETTER A + COMBINING MACRON) is rejected. This very example can be solved normalizing the string before starting the validation but it's not always that easy.



              Imagine that instead of a single validation rule you have...1,000 of them. Or 10,000. I'm not a fan of extension method for primitive types (especially when they're trivial) but the mistake (IMHO) is to work with a too low level of abstraction: you do not want to extend String.Length to replicate what > does: you want to extend String to support high level composable and chainable assertions.



              Note: ContainsOnly() may even accept regexes, pseudo code:



              static ContainsOnly(this StringValidator validator, ...string matches) => ...

              static class Matches {
              public static string LetterOrDigit = "a-zA-Z0-9";
              }


              Composition should now be fast enough even when you cannot really ignore performance (and you can always have a MatchRegEx() extension method).





              How to extend this? In previous example I assumed a pseudo-implementation like this:



              public StringValidator LengthInRange(int min, int max) {
              if (IsValid) {
              IsValid = Value.Length >= min && Value.Length <= max;
              }

              return this;
              }


              You can easily extend it to run all validators to generate a list of errors:



              var result = StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen);

              if (result.HasErrors)
              Console.WriteLine(String.Join(Environment.NewLine, result.Errors));


              Or to throw an exception:



              StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen)
              .ThrowIfInvalid();


              I think you've got the point.






              share|improve this answer











              $endgroup$









              • 1




                $begingroup$
                So if, for example, length is not between 6 and 16 this throws an exception?
                $endgroup$
                – Kenneth K.
                Jan 28 at 18:27








              • 1




                $begingroup$
                No as per requirements it should return false (the other validators simply fall through after the first one marked the result as invalid?). You can throw explicitly or with an option in Configure() or with a call to ThrowIfInvalid(). For a future requirement it may add an IEnumerable<string> property to collect all errors (for example)
                $endgroup$
                – Adriano Repetti
                Jan 28 at 19:09














              6












              6








              6





              $begingroup$

              Little bit late but let me add another alternative solution, t3chb0t already took care of your code.



              You have extension methods for IComparable<T> and Char, this forces you to write your business logic in code instead of a (pseudo) high-level language which resembles your business rules. If you do not want to use a regular expression (I'd use it) then you should try to match 1:1 the language of your requirements:



              StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .ContainsOnly(Matches.LetterOrDigits, Matches.Hyphen).HasAtMost(1, Matches.Hyphen)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen);


              This is simple enough to be self-describing. Does it hurt performance? Maybe but it's seldom a problem in LoB applications and business rules can become incredibly convoluted. Is it a problem? Measure it, for performance critical code it's easy to write an extension method (for StringValidator) to perform a specific task.



              Note that I'm using an hypothetical Matches.LetterOrDigits() function with string -> bool signature instead of, let's say, Char.IsLetterOrDigit() with char -> bool. Why? Because not every character (OK, I won't repeat this again...) is a single Char then you have to compare strings (few examples: à or or 𠀑).



              Your requirements are, probably, vague and your code broken regardless the interpretation you picked. What do they mean with letter? An US-ASCII Latin character? In that case your code is wrong because ā (LATIN SMALL LETTER A MACRON) is accepted. Is is a letter everything in the Letter Unicode category (or even just in the Latin Extended Unicode block)? Your code is wrong again because (LATIN SMALL LETTER A + COMBINING MACRON) is rejected. This very example can be solved normalizing the string before starting the validation but it's not always that easy.



              Imagine that instead of a single validation rule you have...1,000 of them. Or 10,000. I'm not a fan of extension method for primitive types (especially when they're trivial) but the mistake (IMHO) is to work with a too low level of abstraction: you do not want to extend String.Length to replicate what > does: you want to extend String to support high level composable and chainable assertions.



              Note: ContainsOnly() may even accept regexes, pseudo code:



              static ContainsOnly(this StringValidator validator, ...string matches) => ...

              static class Matches {
              public static string LetterOrDigit = "a-zA-Z0-9";
              }


              Composition should now be fast enough even when you cannot really ignore performance (and you can always have a MatchRegEx() extension method).





              How to extend this? In previous example I assumed a pseudo-implementation like this:



              public StringValidator LengthInRange(int min, int max) {
              if (IsValid) {
              IsValid = Value.Length >= min && Value.Length <= max;
              }

              return this;
              }


              You can easily extend it to run all validators to generate a list of errors:



              var result = StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen);

              if (result.HasErrors)
              Console.WriteLine(String.Join(Environment.NewLine, result.Errors));


              Or to throw an exception:



              StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen)
              .ThrowIfInvalid();


              I think you've got the point.






              share|improve this answer











              $endgroup$



              Little bit late but let me add another alternative solution, t3chb0t already took care of your code.



              You have extension methods for IComparable<T> and Char, this forces you to write your business logic in code instead of a (pseudo) high-level language which resembles your business rules. If you do not want to use a regular expression (I'd use it) then you should try to match 1:1 the language of your requirements:



              StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .ContainsOnly(Matches.LetterOrDigits, Matches.Hyphen).HasAtMost(1, Matches.Hyphen)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen);


              This is simple enough to be self-describing. Does it hurt performance? Maybe but it's seldom a problem in LoB applications and business rules can become incredibly convoluted. Is it a problem? Measure it, for performance critical code it's easy to write an extension method (for StringValidator) to perform a specific task.



              Note that I'm using an hypothetical Matches.LetterOrDigits() function with string -> bool signature instead of, let's say, Char.IsLetterOrDigit() with char -> bool. Why? Because not every character (OK, I won't repeat this again...) is a single Char then you have to compare strings (few examples: à or or 𠀑).



              Your requirements are, probably, vague and your code broken regardless the interpretation you picked. What do they mean with letter? An US-ASCII Latin character? In that case your code is wrong because ā (LATIN SMALL LETTER A MACRON) is accepted. Is is a letter everything in the Letter Unicode category (or even just in the Latin Extended Unicode block)? Your code is wrong again because (LATIN SMALL LETTER A + COMBINING MACRON) is rejected. This very example can be solved normalizing the string before starting the validation but it's not always that easy.



              Imagine that instead of a single validation rule you have...1,000 of them. Or 10,000. I'm not a fan of extension method for primitive types (especially when they're trivial) but the mistake (IMHO) is to work with a too low level of abstraction: you do not want to extend String.Length to replicate what > does: you want to extend String to support high level composable and chainable assertions.



              Note: ContainsOnly() may even accept regexes, pseudo code:



              static ContainsOnly(this StringValidator validator, ...string matches) => ...

              static class Matches {
              public static string LetterOrDigit = "a-zA-Z0-9";
              }


              Composition should now be fast enough even when you cannot really ignore performance (and you can always have a MatchRegEx() extension method).





              How to extend this? In previous example I assumed a pseudo-implementation like this:



              public StringValidator LengthInRange(int min, int max) {
              if (IsValid) {
              IsValid = Value.Length >= min && Value.Length <= max;
              }

              return this;
              }


              You can easily extend it to run all validators to generate a list of errors:



              var result = StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen);

              if (result.HasErrors)
              Console.WriteLine(String.Join(Environment.NewLine, result.Errors));


              Or to throw an exception:



              StringValidator.Validate(str)
              .LengthIsInRange(6, 16)
              .StartsWith(Matches.Letter)
              .DoesNotEndWith(Matches.Hyphen)
              .ThrowIfInvalid();


              I think you've got the point.







              share|improve this answer














              share|improve this answer



              share|improve this answer








              edited Jan 31 at 11:24

























              answered Jan 28 at 18:09









              Adriano RepettiAdriano Repetti

              10k11442




              10k11442








              • 1




                $begingroup$
                So if, for example, length is not between 6 and 16 this throws an exception?
                $endgroup$
                – Kenneth K.
                Jan 28 at 18:27








              • 1




                $begingroup$
                No as per requirements it should return false (the other validators simply fall through after the first one marked the result as invalid?). You can throw explicitly or with an option in Configure() or with a call to ThrowIfInvalid(). For a future requirement it may add an IEnumerable<string> property to collect all errors (for example)
                $endgroup$
                – Adriano Repetti
                Jan 28 at 19:09














              • 1




                $begingroup$
                So if, for example, length is not between 6 and 16 this throws an exception?
                $endgroup$
                – Kenneth K.
                Jan 28 at 18:27








              • 1




                $begingroup$
                No as per requirements it should return false (the other validators simply fall through after the first one marked the result as invalid?). You can throw explicitly or with an option in Configure() or with a call to ThrowIfInvalid(). For a future requirement it may add an IEnumerable<string> property to collect all errors (for example)
                $endgroup$
                – Adriano Repetti
                Jan 28 at 19:09








              1




              1




              $begingroup$
              So if, for example, length is not between 6 and 16 this throws an exception?
              $endgroup$
              – Kenneth K.
              Jan 28 at 18:27






              $begingroup$
              So if, for example, length is not between 6 and 16 this throws an exception?
              $endgroup$
              – Kenneth K.
              Jan 28 at 18:27






              1




              1




              $begingroup$
              No as per requirements it should return false (the other validators simply fall through after the first one marked the result as invalid?). You can throw explicitly or with an option in Configure() or with a call to ThrowIfInvalid(). For a future requirement it may add an IEnumerable<string> property to collect all errors (for example)
              $endgroup$
              – Adriano Repetti
              Jan 28 at 19:09




              $begingroup$
              No as per requirements it should return false (the other validators simply fall through after the first one marked the result as invalid?). You can throw explicitly or with an option in Configure() or with a call to ThrowIfInvalid(). For a future requirement it may add an IEnumerable<string> property to collect all errors (for example)
              $endgroup$
              – Adriano Repetti
              Jan 28 at 19:09











              5












              $begingroup$

              I think you've seen enough alternative solutions so I'll just review your code.




              • You should not invent new terms for x < 0 like IsStrictlyLowerThan as this makes things unnecessarily more difficult to understand. Instead, you should stick to the well known naming. In this case that would be IsLessThan or in case of x <= 0 it would be IsLessThanOrEqual - Everyone who write code would understand these without any further explanation. Or in case of IsStrictlyNotBetween should be IsNotInRangeExclusive etc.

              • There are voices in comments that suggest using == '-' instead of your extensions. I don't agree with that. Your extensions are much better because they make the code look clean and consistent. Having extensions for some operations and not for others would make it look dirty.

              • I don't however agree with your decision for not using Regex. With regex it's usually simpler and easier to express complex validations. It's also easier to make your validations case-insensitive or match other patterns or use such services as regex101.com to test them. It might be enough for now but sooner or later you will need it so don't be so strict about always consider other solutions.


              • Keep also in mind that your IsStrictlyLowerThan APIs are currently case sensitive so you might consider using IEqualityComparer<string> as the last argument and not the TComparable that doesn't offer you any additional functionality in this context. So your API should have signatures similar to this one:



                public static bool IsLessThan(this string value, string other, IComparer<string> comaprer = default)
                {
                return (comaprer ?? StringComparer.Ordinal).Compare(value, other) < 0;
                }


                Then you can use it with other comparers if necessary, e.g.:



                "FOO".IsLessThan("foo", StringComparer.OrdinalIgnoreCase)







              share|improve this answer









              $endgroup$


















                5












                $begingroup$

                I think you've seen enough alternative solutions so I'll just review your code.




                • You should not invent new terms for x < 0 like IsStrictlyLowerThan as this makes things unnecessarily more difficult to understand. Instead, you should stick to the well known naming. In this case that would be IsLessThan or in case of x <= 0 it would be IsLessThanOrEqual - Everyone who write code would understand these without any further explanation. Or in case of IsStrictlyNotBetween should be IsNotInRangeExclusive etc.

                • There are voices in comments that suggest using == '-' instead of your extensions. I don't agree with that. Your extensions are much better because they make the code look clean and consistent. Having extensions for some operations and not for others would make it look dirty.

                • I don't however agree with your decision for not using Regex. With regex it's usually simpler and easier to express complex validations. It's also easier to make your validations case-insensitive or match other patterns or use such services as regex101.com to test them. It might be enough for now but sooner or later you will need it so don't be so strict about always consider other solutions.


                • Keep also in mind that your IsStrictlyLowerThan APIs are currently case sensitive so you might consider using IEqualityComparer<string> as the last argument and not the TComparable that doesn't offer you any additional functionality in this context. So your API should have signatures similar to this one:



                  public static bool IsLessThan(this string value, string other, IComparer<string> comaprer = default)
                  {
                  return (comaprer ?? StringComparer.Ordinal).Compare(value, other) < 0;
                  }


                  Then you can use it with other comparers if necessary, e.g.:



                  "FOO".IsLessThan("foo", StringComparer.OrdinalIgnoreCase)







                share|improve this answer









                $endgroup$
















                  5












                  5








                  5





                  $begingroup$

                  I think you've seen enough alternative solutions so I'll just review your code.




                  • You should not invent new terms for x < 0 like IsStrictlyLowerThan as this makes things unnecessarily more difficult to understand. Instead, you should stick to the well known naming. In this case that would be IsLessThan or in case of x <= 0 it would be IsLessThanOrEqual - Everyone who write code would understand these without any further explanation. Or in case of IsStrictlyNotBetween should be IsNotInRangeExclusive etc.

                  • There are voices in comments that suggest using == '-' instead of your extensions. I don't agree with that. Your extensions are much better because they make the code look clean and consistent. Having extensions for some operations and not for others would make it look dirty.

                  • I don't however agree with your decision for not using Regex. With regex it's usually simpler and easier to express complex validations. It's also easier to make your validations case-insensitive or match other patterns or use such services as regex101.com to test them. It might be enough for now but sooner or later you will need it so don't be so strict about always consider other solutions.


                  • Keep also in mind that your IsStrictlyLowerThan APIs are currently case sensitive so you might consider using IEqualityComparer<string> as the last argument and not the TComparable that doesn't offer you any additional functionality in this context. So your API should have signatures similar to this one:



                    public static bool IsLessThan(this string value, string other, IComparer<string> comaprer = default)
                    {
                    return (comaprer ?? StringComparer.Ordinal).Compare(value, other) < 0;
                    }


                    Then you can use it with other comparers if necessary, e.g.:



                    "FOO".IsLessThan("foo", StringComparer.OrdinalIgnoreCase)







                  share|improve this answer









                  $endgroup$



                  I think you've seen enough alternative solutions so I'll just review your code.




                  • You should not invent new terms for x < 0 like IsStrictlyLowerThan as this makes things unnecessarily more difficult to understand. Instead, you should stick to the well known naming. In this case that would be IsLessThan or in case of x <= 0 it would be IsLessThanOrEqual - Everyone who write code would understand these without any further explanation. Or in case of IsStrictlyNotBetween should be IsNotInRangeExclusive etc.

                  • There are voices in comments that suggest using == '-' instead of your extensions. I don't agree with that. Your extensions are much better because they make the code look clean and consistent. Having extensions for some operations and not for others would make it look dirty.

                  • I don't however agree with your decision for not using Regex. With regex it's usually simpler and easier to express complex validations. It's also easier to make your validations case-insensitive or match other patterns or use such services as regex101.com to test them. It might be enough for now but sooner or later you will need it so don't be so strict about always consider other solutions.


                  • Keep also in mind that your IsStrictlyLowerThan APIs are currently case sensitive so you might consider using IEqualityComparer<string> as the last argument and not the TComparable that doesn't offer you any additional functionality in this context. So your API should have signatures similar to this one:



                    public static bool IsLessThan(this string value, string other, IComparer<string> comaprer = default)
                    {
                    return (comaprer ?? StringComparer.Ordinal).Compare(value, other) < 0;
                    }


                    Then you can use it with other comparers if necessary, e.g.:



                    "FOO".IsLessThan("foo", StringComparer.OrdinalIgnoreCase)








                  share|improve this answer












                  share|improve this answer



                  share|improve this answer










                  answered Jan 28 at 16:32









                  t3chb0tt3chb0t

                  35k753125




                  35k753125























                      2












                      $begingroup$

                      N.B. I thought I'd answer the original test question myself in F# as a Kata, so this is a functional suggestion based on what I found.



                      I'll only talk about the versatility part of the solution, others have touched on using Linq and I think your code is already quite readable.





                      If you can divide the original requirement into a number of smaller rules, where each rule has the same "Shape".



                      This "Shape" is take a sequence of characters and return a Boolean.



                      So in C# we'd have:



                      bool LengthIsValid(string s) => s.Length >= 6 && s.Length <= 16;

                      bool Rule2(string s) => true; //e.g must start with Letter, or doesn't with with '-'


                      Now that we have each rule in the same "Shape", you can create a composite set of rules and validate them all together:



                      var rules = new List<Func<string, bool>> 
                      {
                      LengthIsValid,
                      Rule2,
                      //etc.
                      };


                      Checking the rules is then just a matter of applying those rules to the input string:



                      bool CheckAllRules (string s) =>
                      rules
                      .All(rule => rule(s));


                      With this approach you get versatility from been able to create any number of composition of rules.



                      e.g. Creating Rules where the powerUsers don't need to check the length (I can be root).



                      IEnumerable<Func<string, bool>> CreateRules(bool powerUser)
                      {
                      if (!powerUser)
                      yield return LengthIsValid;

                      yield return Rule2;
                      }



                      The F# version if you're interested in here






                      share|improve this answer











                      $endgroup$













                      • $begingroup$
                        In C# you might want to write rules.All(x => x(s)) (unless you want to evaluate all the rules without stopping with the first failed one).
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:33










                      • $begingroup$
                        It's OK, because LINQ is lazy evaluated "All" will only keep pulling whilst the predicate returns "true", as soon the result of the predicate is "false" it will stop iterating through the rules. The "select" code only executes when "All" asks for the one more item. Try changing it to .Select(rule => { Console.WriteLine("test"); return rule(s); }) and you will only see one "test" if the first rule fails.
                        $endgroup$
                        – DaveShaw
                        Jan 28 at 23:47










                      • $begingroup$
                        Yep, I worded that very poorly. The two parts of my comments were unrelated. 1) you don't need Select() and 2) unless you want to extract them all (but in that case you obviously need ToArray() or similar before All()).
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:59










                      • $begingroup$
                        Yes, I have a habit of using .Select and .Where followed by .Any/All/Single.First/etc. - it usually makes things clearer for me :) In this case, I think you're correct that just .All would suffice.
                        $endgroup$
                        – DaveShaw
                        Jan 29 at 0:07
















                      2












                      $begingroup$

                      N.B. I thought I'd answer the original test question myself in F# as a Kata, so this is a functional suggestion based on what I found.



                      I'll only talk about the versatility part of the solution, others have touched on using Linq and I think your code is already quite readable.





                      If you can divide the original requirement into a number of smaller rules, where each rule has the same "Shape".



                      This "Shape" is take a sequence of characters and return a Boolean.



                      So in C# we'd have:



                      bool LengthIsValid(string s) => s.Length >= 6 && s.Length <= 16;

                      bool Rule2(string s) => true; //e.g must start with Letter, or doesn't with with '-'


                      Now that we have each rule in the same "Shape", you can create a composite set of rules and validate them all together:



                      var rules = new List<Func<string, bool>> 
                      {
                      LengthIsValid,
                      Rule2,
                      //etc.
                      };


                      Checking the rules is then just a matter of applying those rules to the input string:



                      bool CheckAllRules (string s) =>
                      rules
                      .All(rule => rule(s));


                      With this approach you get versatility from been able to create any number of composition of rules.



                      e.g. Creating Rules where the powerUsers don't need to check the length (I can be root).



                      IEnumerable<Func<string, bool>> CreateRules(bool powerUser)
                      {
                      if (!powerUser)
                      yield return LengthIsValid;

                      yield return Rule2;
                      }



                      The F# version if you're interested in here






                      share|improve this answer











                      $endgroup$













                      • $begingroup$
                        In C# you might want to write rules.All(x => x(s)) (unless you want to evaluate all the rules without stopping with the first failed one).
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:33










                      • $begingroup$
                        It's OK, because LINQ is lazy evaluated "All" will only keep pulling whilst the predicate returns "true", as soon the result of the predicate is "false" it will stop iterating through the rules. The "select" code only executes when "All" asks for the one more item. Try changing it to .Select(rule => { Console.WriteLine("test"); return rule(s); }) and you will only see one "test" if the first rule fails.
                        $endgroup$
                        – DaveShaw
                        Jan 28 at 23:47










                      • $begingroup$
                        Yep, I worded that very poorly. The two parts of my comments were unrelated. 1) you don't need Select() and 2) unless you want to extract them all (but in that case you obviously need ToArray() or similar before All()).
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:59










                      • $begingroup$
                        Yes, I have a habit of using .Select and .Where followed by .Any/All/Single.First/etc. - it usually makes things clearer for me :) In this case, I think you're correct that just .All would suffice.
                        $endgroup$
                        – DaveShaw
                        Jan 29 at 0:07














                      2












                      2








                      2





                      $begingroup$

                      N.B. I thought I'd answer the original test question myself in F# as a Kata, so this is a functional suggestion based on what I found.



                      I'll only talk about the versatility part of the solution, others have touched on using Linq and I think your code is already quite readable.





                      If you can divide the original requirement into a number of smaller rules, where each rule has the same "Shape".



                      This "Shape" is take a sequence of characters and return a Boolean.



                      So in C# we'd have:



                      bool LengthIsValid(string s) => s.Length >= 6 && s.Length <= 16;

                      bool Rule2(string s) => true; //e.g must start with Letter, or doesn't with with '-'


                      Now that we have each rule in the same "Shape", you can create a composite set of rules and validate them all together:



                      var rules = new List<Func<string, bool>> 
                      {
                      LengthIsValid,
                      Rule2,
                      //etc.
                      };


                      Checking the rules is then just a matter of applying those rules to the input string:



                      bool CheckAllRules (string s) =>
                      rules
                      .All(rule => rule(s));


                      With this approach you get versatility from been able to create any number of composition of rules.



                      e.g. Creating Rules where the powerUsers don't need to check the length (I can be root).



                      IEnumerable<Func<string, bool>> CreateRules(bool powerUser)
                      {
                      if (!powerUser)
                      yield return LengthIsValid;

                      yield return Rule2;
                      }



                      The F# version if you're interested in here






                      share|improve this answer











                      $endgroup$



                      N.B. I thought I'd answer the original test question myself in F# as a Kata, so this is a functional suggestion based on what I found.



                      I'll only talk about the versatility part of the solution, others have touched on using Linq and I think your code is already quite readable.





                      If you can divide the original requirement into a number of smaller rules, where each rule has the same "Shape".



                      This "Shape" is take a sequence of characters and return a Boolean.



                      So in C# we'd have:



                      bool LengthIsValid(string s) => s.Length >= 6 && s.Length <= 16;

                      bool Rule2(string s) => true; //e.g must start with Letter, or doesn't with with '-'


                      Now that we have each rule in the same "Shape", you can create a composite set of rules and validate them all together:



                      var rules = new List<Func<string, bool>> 
                      {
                      LengthIsValid,
                      Rule2,
                      //etc.
                      };


                      Checking the rules is then just a matter of applying those rules to the input string:



                      bool CheckAllRules (string s) =>
                      rules
                      .All(rule => rule(s));


                      With this approach you get versatility from been able to create any number of composition of rules.



                      e.g. Creating Rules where the powerUsers don't need to check the length (I can be root).



                      IEnumerable<Func<string, bool>> CreateRules(bool powerUser)
                      {
                      if (!powerUser)
                      yield return LengthIsValid;

                      yield return Rule2;
                      }



                      The F# version if you're interested in here







                      share|improve this answer














                      share|improve this answer



                      share|improve this answer








                      edited Jan 29 at 0:07

























                      answered Jan 28 at 23:30









                      DaveShawDaveShaw

                      1214




                      1214












                      • $begingroup$
                        In C# you might want to write rules.All(x => x(s)) (unless you want to evaluate all the rules without stopping with the first failed one).
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:33










                      • $begingroup$
                        It's OK, because LINQ is lazy evaluated "All" will only keep pulling whilst the predicate returns "true", as soon the result of the predicate is "false" it will stop iterating through the rules. The "select" code only executes when "All" asks for the one more item. Try changing it to .Select(rule => { Console.WriteLine("test"); return rule(s); }) and you will only see one "test" if the first rule fails.
                        $endgroup$
                        – DaveShaw
                        Jan 28 at 23:47










                      • $begingroup$
                        Yep, I worded that very poorly. The two parts of my comments were unrelated. 1) you don't need Select() and 2) unless you want to extract them all (but in that case you obviously need ToArray() or similar before All()).
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:59










                      • $begingroup$
                        Yes, I have a habit of using .Select and .Where followed by .Any/All/Single.First/etc. - it usually makes things clearer for me :) In this case, I think you're correct that just .All would suffice.
                        $endgroup$
                        – DaveShaw
                        Jan 29 at 0:07


















                      • $begingroup$
                        In C# you might want to write rules.All(x => x(s)) (unless you want to evaluate all the rules without stopping with the first failed one).
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:33










                      • $begingroup$
                        It's OK, because LINQ is lazy evaluated "All" will only keep pulling whilst the predicate returns "true", as soon the result of the predicate is "false" it will stop iterating through the rules. The "select" code only executes when "All" asks for the one more item. Try changing it to .Select(rule => { Console.WriteLine("test"); return rule(s); }) and you will only see one "test" if the first rule fails.
                        $endgroup$
                        – DaveShaw
                        Jan 28 at 23:47










                      • $begingroup$
                        Yep, I worded that very poorly. The two parts of my comments were unrelated. 1) you don't need Select() and 2) unless you want to extract them all (but in that case you obviously need ToArray() or similar before All()).
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:59










                      • $begingroup$
                        Yes, I have a habit of using .Select and .Where followed by .Any/All/Single.First/etc. - it usually makes things clearer for me :) In this case, I think you're correct that just .All would suffice.
                        $endgroup$
                        – DaveShaw
                        Jan 29 at 0:07
















                      $begingroup$
                      In C# you might want to write rules.All(x => x(s)) (unless you want to evaluate all the rules without stopping with the first failed one).
                      $endgroup$
                      – Adriano Repetti
                      Jan 28 at 23:33




                      $begingroup$
                      In C# you might want to write rules.All(x => x(s)) (unless you want to evaluate all the rules without stopping with the first failed one).
                      $endgroup$
                      – Adriano Repetti
                      Jan 28 at 23:33












                      $begingroup$
                      It's OK, because LINQ is lazy evaluated "All" will only keep pulling whilst the predicate returns "true", as soon the result of the predicate is "false" it will stop iterating through the rules. The "select" code only executes when "All" asks for the one more item. Try changing it to .Select(rule => { Console.WriteLine("test"); return rule(s); }) and you will only see one "test" if the first rule fails.
                      $endgroup$
                      – DaveShaw
                      Jan 28 at 23:47




                      $begingroup$
                      It's OK, because LINQ is lazy evaluated "All" will only keep pulling whilst the predicate returns "true", as soon the result of the predicate is "false" it will stop iterating through the rules. The "select" code only executes when "All" asks for the one more item. Try changing it to .Select(rule => { Console.WriteLine("test"); return rule(s); }) and you will only see one "test" if the first rule fails.
                      $endgroup$
                      – DaveShaw
                      Jan 28 at 23:47












                      $begingroup$
                      Yep, I worded that very poorly. The two parts of my comments were unrelated. 1) you don't need Select() and 2) unless you want to extract them all (but in that case you obviously need ToArray() or similar before All()).
                      $endgroup$
                      – Adriano Repetti
                      Jan 28 at 23:59




                      $begingroup$
                      Yep, I worded that very poorly. The two parts of my comments were unrelated. 1) you don't need Select() and 2) unless you want to extract them all (but in that case you obviously need ToArray() or similar before All()).
                      $endgroup$
                      – Adriano Repetti
                      Jan 28 at 23:59












                      $begingroup$
                      Yes, I have a habit of using .Select and .Where followed by .Any/All/Single.First/etc. - it usually makes things clearer for me :) In this case, I think you're correct that just .All would suffice.
                      $endgroup$
                      – DaveShaw
                      Jan 29 at 0:07




                      $begingroup$
                      Yes, I have a habit of using .Select and .Where followed by .Any/All/Single.First/etc. - it usually makes things clearer for me :) In this case, I think you're correct that just .All would suffice.
                      $endgroup$
                      – DaveShaw
                      Jan 29 at 0:07











                      1












                      $begingroup$

                      About versatility my thought is a separate class to stipulate the different possible rules:



                      class ValidationRules
                      {
                      public const char HYPHEN = '-';
                      public readonly int hyphenCount;
                      public readonly bool needUpper;
                      public readonly bool needLower;
                      public readonly bool needDigit;
                      public readonly bool allowSpaces;
                      public readonly int minLength;
                      public readonly int maxLength;
                      /// <summary>
                      /// Constructor with min and max length and default rules:
                      /// needUpper = true
                      /// needLower = true
                      /// allowSpaces = false
                      /// hyphenCount = 1;
                      /// </summary>
                      public ValidationRules(int minLength, int maxLength)
                      {
                      this.minLength = minLength;
                      this.maxLength = maxLength;
                      hyphenCount = 1;
                      needLower = true;
                      needUpper = true;
                      needDigit = true;
                      allowSpaces = false;
                      }
                      /// <summary>
                      /// Constructor with min and max length and supplied rules:
                      /// </summary>
                      public ValidationRules(int minLength, int maxLength, int hyphenCount, bool needUpper, bool needLower, bool needDigit, bool allowSpaces)
                      {
                      this.minLength = minLength;
                      this.maxLength = maxLength;
                      this.hyphenCount = hyphenCount;
                      this.needLower = needLower;
                      this.needUpper = needUpper;
                      this.needDigit = needDigit;
                      this.allowSpaces = allowSpaces;
                      }
                      }


                      the method to validate is still quite simple:



                      /// <summary>
                      /// Validate string according to validation rules
                      /// </summary>
                      /// <returns></returns>
                      public static bool Validate(string input,ValidationRules rules)
                      {
                      if(input.Length < rules.minLength || input.Length > rules.maxLength)
                      {
                      return false;
                      }
                      if(!Char.IsLetter(input[0]) || input[input.Length-1] == ValidationRules.HYPHEN)
                      {
                      return false;
                      }
                      return input.Count(x => x == ValidationRules.HYPHEN) <= rules.hyphenCount && input.All(x =>
                      (rules.needUpper && char.IsUpper(x)) ||
                      (rules.needLower && char.IsLower(x)) ||
                      (rules.allowSpaces && char.IsWhiteSpace(x)) ||
                      (rules.needDigit && char.IsDigit(x)) ||
                      (x == ValidationRules.HYPHEN));
                      }





                      share|improve this answer











                      $endgroup$













                      • $begingroup$
                        I like your approach!
                        $endgroup$
                        – Ehouarn Perret
                        Jan 28 at 15:11










                      • $begingroup$
                        For an approach that targets extendability, it will quickly become a mini god class, requiring changes on several different locations. Multiple types with a single visitor would be much better off SOLID and elegance-wise.
                        $endgroup$
                        – Denis
                        Jan 28 at 15:17










                      • $begingroup$
                        @Denis actually I was thinking about a set of rules that could be built out of a builder: github.com/JeremySkinner/FluentValidation
                        $endgroup$
                        – Ehouarn Perret
                        Jan 28 at 15:27






                      • 2




                        $begingroup$
                        @EhouarnPerret That's similar to what I meant, but this implementation would add way too much overhead for such a simple task, if it were an interview question, you should also consider time. Show off some techniques but don't overdo it.
                        $endgroup$
                        – Denis
                        Jan 28 at 15:30
















                      1












                      $begingroup$

                      About versatility my thought is a separate class to stipulate the different possible rules:



                      class ValidationRules
                      {
                      public const char HYPHEN = '-';
                      public readonly int hyphenCount;
                      public readonly bool needUpper;
                      public readonly bool needLower;
                      public readonly bool needDigit;
                      public readonly bool allowSpaces;
                      public readonly int minLength;
                      public readonly int maxLength;
                      /// <summary>
                      /// Constructor with min and max length and default rules:
                      /// needUpper = true
                      /// needLower = true
                      /// allowSpaces = false
                      /// hyphenCount = 1;
                      /// </summary>
                      public ValidationRules(int minLength, int maxLength)
                      {
                      this.minLength = minLength;
                      this.maxLength = maxLength;
                      hyphenCount = 1;
                      needLower = true;
                      needUpper = true;
                      needDigit = true;
                      allowSpaces = false;
                      }
                      /// <summary>
                      /// Constructor with min and max length and supplied rules:
                      /// </summary>
                      public ValidationRules(int minLength, int maxLength, int hyphenCount, bool needUpper, bool needLower, bool needDigit, bool allowSpaces)
                      {
                      this.minLength = minLength;
                      this.maxLength = maxLength;
                      this.hyphenCount = hyphenCount;
                      this.needLower = needLower;
                      this.needUpper = needUpper;
                      this.needDigit = needDigit;
                      this.allowSpaces = allowSpaces;
                      }
                      }


                      the method to validate is still quite simple:



                      /// <summary>
                      /// Validate string according to validation rules
                      /// </summary>
                      /// <returns></returns>
                      public static bool Validate(string input,ValidationRules rules)
                      {
                      if(input.Length < rules.minLength || input.Length > rules.maxLength)
                      {
                      return false;
                      }
                      if(!Char.IsLetter(input[0]) || input[input.Length-1] == ValidationRules.HYPHEN)
                      {
                      return false;
                      }
                      return input.Count(x => x == ValidationRules.HYPHEN) <= rules.hyphenCount && input.All(x =>
                      (rules.needUpper && char.IsUpper(x)) ||
                      (rules.needLower && char.IsLower(x)) ||
                      (rules.allowSpaces && char.IsWhiteSpace(x)) ||
                      (rules.needDigit && char.IsDigit(x)) ||
                      (x == ValidationRules.HYPHEN));
                      }





                      share|improve this answer











                      $endgroup$













                      • $begingroup$
                        I like your approach!
                        $endgroup$
                        – Ehouarn Perret
                        Jan 28 at 15:11










                      • $begingroup$
                        For an approach that targets extendability, it will quickly become a mini god class, requiring changes on several different locations. Multiple types with a single visitor would be much better off SOLID and elegance-wise.
                        $endgroup$
                        – Denis
                        Jan 28 at 15:17










                      • $begingroup$
                        @Denis actually I was thinking about a set of rules that could be built out of a builder: github.com/JeremySkinner/FluentValidation
                        $endgroup$
                        – Ehouarn Perret
                        Jan 28 at 15:27






                      • 2




                        $begingroup$
                        @EhouarnPerret That's similar to what I meant, but this implementation would add way too much overhead for such a simple task, if it were an interview question, you should also consider time. Show off some techniques but don't overdo it.
                        $endgroup$
                        – Denis
                        Jan 28 at 15:30














                      1












                      1








                      1





                      $begingroup$

                      About versatility my thought is a separate class to stipulate the different possible rules:



                      class ValidationRules
                      {
                      public const char HYPHEN = '-';
                      public readonly int hyphenCount;
                      public readonly bool needUpper;
                      public readonly bool needLower;
                      public readonly bool needDigit;
                      public readonly bool allowSpaces;
                      public readonly int minLength;
                      public readonly int maxLength;
                      /// <summary>
                      /// Constructor with min and max length and default rules:
                      /// needUpper = true
                      /// needLower = true
                      /// allowSpaces = false
                      /// hyphenCount = 1;
                      /// </summary>
                      public ValidationRules(int minLength, int maxLength)
                      {
                      this.minLength = minLength;
                      this.maxLength = maxLength;
                      hyphenCount = 1;
                      needLower = true;
                      needUpper = true;
                      needDigit = true;
                      allowSpaces = false;
                      }
                      /// <summary>
                      /// Constructor with min and max length and supplied rules:
                      /// </summary>
                      public ValidationRules(int minLength, int maxLength, int hyphenCount, bool needUpper, bool needLower, bool needDigit, bool allowSpaces)
                      {
                      this.minLength = minLength;
                      this.maxLength = maxLength;
                      this.hyphenCount = hyphenCount;
                      this.needLower = needLower;
                      this.needUpper = needUpper;
                      this.needDigit = needDigit;
                      this.allowSpaces = allowSpaces;
                      }
                      }


                      the method to validate is still quite simple:



                      /// <summary>
                      /// Validate string according to validation rules
                      /// </summary>
                      /// <returns></returns>
                      public static bool Validate(string input,ValidationRules rules)
                      {
                      if(input.Length < rules.minLength || input.Length > rules.maxLength)
                      {
                      return false;
                      }
                      if(!Char.IsLetter(input[0]) || input[input.Length-1] == ValidationRules.HYPHEN)
                      {
                      return false;
                      }
                      return input.Count(x => x == ValidationRules.HYPHEN) <= rules.hyphenCount && input.All(x =>
                      (rules.needUpper && char.IsUpper(x)) ||
                      (rules.needLower && char.IsLower(x)) ||
                      (rules.allowSpaces && char.IsWhiteSpace(x)) ||
                      (rules.needDigit && char.IsDigit(x)) ||
                      (x == ValidationRules.HYPHEN));
                      }





                      share|improve this answer











                      $endgroup$



                      About versatility my thought is a separate class to stipulate the different possible rules:



                      class ValidationRules
                      {
                      public const char HYPHEN = '-';
                      public readonly int hyphenCount;
                      public readonly bool needUpper;
                      public readonly bool needLower;
                      public readonly bool needDigit;
                      public readonly bool allowSpaces;
                      public readonly int minLength;
                      public readonly int maxLength;
                      /// <summary>
                      /// Constructor with min and max length and default rules:
                      /// needUpper = true
                      /// needLower = true
                      /// allowSpaces = false
                      /// hyphenCount = 1;
                      /// </summary>
                      public ValidationRules(int minLength, int maxLength)
                      {
                      this.minLength = minLength;
                      this.maxLength = maxLength;
                      hyphenCount = 1;
                      needLower = true;
                      needUpper = true;
                      needDigit = true;
                      allowSpaces = false;
                      }
                      /// <summary>
                      /// Constructor with min and max length and supplied rules:
                      /// </summary>
                      public ValidationRules(int minLength, int maxLength, int hyphenCount, bool needUpper, bool needLower, bool needDigit, bool allowSpaces)
                      {
                      this.minLength = minLength;
                      this.maxLength = maxLength;
                      this.hyphenCount = hyphenCount;
                      this.needLower = needLower;
                      this.needUpper = needUpper;
                      this.needDigit = needDigit;
                      this.allowSpaces = allowSpaces;
                      }
                      }


                      the method to validate is still quite simple:



                      /// <summary>
                      /// Validate string according to validation rules
                      /// </summary>
                      /// <returns></returns>
                      public static bool Validate(string input,ValidationRules rules)
                      {
                      if(input.Length < rules.minLength || input.Length > rules.maxLength)
                      {
                      return false;
                      }
                      if(!Char.IsLetter(input[0]) || input[input.Length-1] == ValidationRules.HYPHEN)
                      {
                      return false;
                      }
                      return input.Count(x => x == ValidationRules.HYPHEN) <= rules.hyphenCount && input.All(x =>
                      (rules.needUpper && char.IsUpper(x)) ||
                      (rules.needLower && char.IsLower(x)) ||
                      (rules.allowSpaces && char.IsWhiteSpace(x)) ||
                      (rules.needDigit && char.IsDigit(x)) ||
                      (x == ValidationRules.HYPHEN));
                      }






                      share|improve this answer














                      share|improve this answer



                      share|improve this answer








                      edited Jan 28 at 15:32

























                      answered Jan 28 at 15:09









                      tinstaafltinstaafl

                      6,8581928




                      6,8581928












                      • $begingroup$
                        I like your approach!
                        $endgroup$
                        – Ehouarn Perret
                        Jan 28 at 15:11










                      • $begingroup$
                        For an approach that targets extendability, it will quickly become a mini god class, requiring changes on several different locations. Multiple types with a single visitor would be much better off SOLID and elegance-wise.
                        $endgroup$
                        – Denis
                        Jan 28 at 15:17










                      • $begingroup$
                        @Denis actually I was thinking about a set of rules that could be built out of a builder: github.com/JeremySkinner/FluentValidation
                        $endgroup$
                        – Ehouarn Perret
                        Jan 28 at 15:27






                      • 2




                        $begingroup$
                        @EhouarnPerret That's similar to what I meant, but this implementation would add way too much overhead for such a simple task, if it were an interview question, you should also consider time. Show off some techniques but don't overdo it.
                        $endgroup$
                        – Denis
                        Jan 28 at 15:30


















                      • $begingroup$
                        I like your approach!
                        $endgroup$
                        – Ehouarn Perret
                        Jan 28 at 15:11










                      • $begingroup$
                        For an approach that targets extendability, it will quickly become a mini god class, requiring changes on several different locations. Multiple types with a single visitor would be much better off SOLID and elegance-wise.
                        $endgroup$
                        – Denis
                        Jan 28 at 15:17










                      • $begingroup$
                        @Denis actually I was thinking about a set of rules that could be built out of a builder: github.com/JeremySkinner/FluentValidation
                        $endgroup$
                        – Ehouarn Perret
                        Jan 28 at 15:27






                      • 2




                        $begingroup$
                        @EhouarnPerret That's similar to what I meant, but this implementation would add way too much overhead for such a simple task, if it were an interview question, you should also consider time. Show off some techniques but don't overdo it.
                        $endgroup$
                        – Denis
                        Jan 28 at 15:30
















                      $begingroup$
                      I like your approach!
                      $endgroup$
                      – Ehouarn Perret
                      Jan 28 at 15:11




                      $begingroup$
                      I like your approach!
                      $endgroup$
                      – Ehouarn Perret
                      Jan 28 at 15:11












                      $begingroup$
                      For an approach that targets extendability, it will quickly become a mini god class, requiring changes on several different locations. Multiple types with a single visitor would be much better off SOLID and elegance-wise.
                      $endgroup$
                      – Denis
                      Jan 28 at 15:17




                      $begingroup$
                      For an approach that targets extendability, it will quickly become a mini god class, requiring changes on several different locations. Multiple types with a single visitor would be much better off SOLID and elegance-wise.
                      $endgroup$
                      – Denis
                      Jan 28 at 15:17












                      $begingroup$
                      @Denis actually I was thinking about a set of rules that could be built out of a builder: github.com/JeremySkinner/FluentValidation
                      $endgroup$
                      – Ehouarn Perret
                      Jan 28 at 15:27




                      $begingroup$
                      @Denis actually I was thinking about a set of rules that could be built out of a builder: github.com/JeremySkinner/FluentValidation
                      $endgroup$
                      – Ehouarn Perret
                      Jan 28 at 15:27




                      2




                      2




                      $begingroup$
                      @EhouarnPerret That's similar to what I meant, but this implementation would add way too much overhead for such a simple task, if it were an interview question, you should also consider time. Show off some techniques but don't overdo it.
                      $endgroup$
                      – Denis
                      Jan 28 at 15:30




                      $begingroup$
                      @EhouarnPerret That's similar to what I meant, but this implementation would add way too much overhead for such a simple task, if it were an interview question, you should also consider time. Show off some techniques but don't overdo it.
                      $endgroup$
                      – Denis
                      Jan 28 at 15:30











                      1












                      $begingroup$

                      Stop it. Just stop it. Unless you have some kind of unusual performance requirements, keep it simple:



                      /// <summary>
                      /// Regular expression pattern that matches strings containing only Unicode
                      /// letters, digits, and hyphens.
                      /// </summary>
                      public static Regex AllUnicodeLettersNumsHyphen = new Regex(@"^[p{L}p{Nd}-]*$");

                      public static bool IsValid(string s)
                      {
                      return (
                      s.Length >= 6 && s.Length <= 16
                      && AllUnicodeLettersNumsHyphen.IsMatch(s)
                      && s.Count(c => c == '-') <= 1
                      && Char.IsLetter(s, 0)
                      && s[s.Length - 1] != '-'
                      );
                      }


                      (Those parentheses are optional. I find them visually appealing. Follow your group's coding guidelines on style choices like that.)



                      Given the requirements you specify, there's no reason for loops, extension methods, or any of that stuff. I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. The smaller the scope the better when you're reading. It will take someone 30 seconds to fully understand these 6 lines of code. It will take 10 minutes to dive through all yours.



                      And this isn't any less flexible. In fact it's more so. You can trivially add new rules to this 6 line method. That is not the case with your code. Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes.



                      And look how I dealt with the regular expression impenetrability:




                      • Encode as many rules as possible outside the regex.

                      • Only use one fairly simple, appropriately named and documented regex for just the one check: that it matches the character classes. That and a comment are all you need.




                      If you are completely dead set against regular expressions, consider this LINQ based alternative:



                      public static bool IsValid(string s)
                      {
                      return (
                      s.Length >= 6 && s.Length <= 16
                      && s.All(c => Char.IsLetterOrDigit(c) || '-' == c)
                      && s.Count(c => c == '-') <= 1
                      && Char.IsLetter(s, 0)
                      && s[s.Length - 1] != '-'
                      );
                      }





                      share|improve this answer











                      $endgroup$













                      • $begingroup$
                        I'd tend to agree to use a single regex but in that case everything can be expressed with a single regex without any other surrounding code (including the length and the first/last character)
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:30






                      • 1




                        $begingroup$
                        Hmmmmm POV, I guess. To me a well written regex can express clearly the requirement while UnicodeLettersNumsHyphen has the disadvantages of regexes (less easy to write and read) without its advantages (concise, fast and complete). Nitpick: I'd change UnicodeLettersNumsHyphen to a meaningful name because you can't understand what the regex is for without fully decoding it.
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:38






                      • 2




                        $begingroup$
                        I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. - this is so not true. I wouldn't let this code pass to production.
                        $endgroup$
                        – t3chb0t
                        Jan 29 at 6:21






                      • 1




                        $begingroup$
                        Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes. - not that has to be tested but one that can be tested. Your code isn't testable and is so easy to make a mistake there that you wouldn't notice until something goes wrong for the client. This isn't an improvement. It's a great degradation of both readability and testability.
                        $endgroup$
                        – t3chb0t
                        Jan 29 at 6:23








                      • 3




                        $begingroup$
                        @t3chb0t This is testable...
                        $endgroup$
                        – Tvde1
                        Jan 29 at 12:44
















                      1












                      $begingroup$

                      Stop it. Just stop it. Unless you have some kind of unusual performance requirements, keep it simple:



                      /// <summary>
                      /// Regular expression pattern that matches strings containing only Unicode
                      /// letters, digits, and hyphens.
                      /// </summary>
                      public static Regex AllUnicodeLettersNumsHyphen = new Regex(@"^[p{L}p{Nd}-]*$");

                      public static bool IsValid(string s)
                      {
                      return (
                      s.Length >= 6 && s.Length <= 16
                      && AllUnicodeLettersNumsHyphen.IsMatch(s)
                      && s.Count(c => c == '-') <= 1
                      && Char.IsLetter(s, 0)
                      && s[s.Length - 1] != '-'
                      );
                      }


                      (Those parentheses are optional. I find them visually appealing. Follow your group's coding guidelines on style choices like that.)



                      Given the requirements you specify, there's no reason for loops, extension methods, or any of that stuff. I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. The smaller the scope the better when you're reading. It will take someone 30 seconds to fully understand these 6 lines of code. It will take 10 minutes to dive through all yours.



                      And this isn't any less flexible. In fact it's more so. You can trivially add new rules to this 6 line method. That is not the case with your code. Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes.



                      And look how I dealt with the regular expression impenetrability:




                      • Encode as many rules as possible outside the regex.

                      • Only use one fairly simple, appropriately named and documented regex for just the one check: that it matches the character classes. That and a comment are all you need.




                      If you are completely dead set against regular expressions, consider this LINQ based alternative:



                      public static bool IsValid(string s)
                      {
                      return (
                      s.Length >= 6 && s.Length <= 16
                      && s.All(c => Char.IsLetterOrDigit(c) || '-' == c)
                      && s.Count(c => c == '-') <= 1
                      && Char.IsLetter(s, 0)
                      && s[s.Length - 1] != '-'
                      );
                      }





                      share|improve this answer











                      $endgroup$













                      • $begingroup$
                        I'd tend to agree to use a single regex but in that case everything can be expressed with a single regex without any other surrounding code (including the length and the first/last character)
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:30






                      • 1




                        $begingroup$
                        Hmmmmm POV, I guess. To me a well written regex can express clearly the requirement while UnicodeLettersNumsHyphen has the disadvantages of regexes (less easy to write and read) without its advantages (concise, fast and complete). Nitpick: I'd change UnicodeLettersNumsHyphen to a meaningful name because you can't understand what the regex is for without fully decoding it.
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:38






                      • 2




                        $begingroup$
                        I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. - this is so not true. I wouldn't let this code pass to production.
                        $endgroup$
                        – t3chb0t
                        Jan 29 at 6:21






                      • 1




                        $begingroup$
                        Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes. - not that has to be tested but one that can be tested. Your code isn't testable and is so easy to make a mistake there that you wouldn't notice until something goes wrong for the client. This isn't an improvement. It's a great degradation of both readability and testability.
                        $endgroup$
                        – t3chb0t
                        Jan 29 at 6:23








                      • 3




                        $begingroup$
                        @t3chb0t This is testable...
                        $endgroup$
                        – Tvde1
                        Jan 29 at 12:44














                      1












                      1








                      1





                      $begingroup$

                      Stop it. Just stop it. Unless you have some kind of unusual performance requirements, keep it simple:



                      /// <summary>
                      /// Regular expression pattern that matches strings containing only Unicode
                      /// letters, digits, and hyphens.
                      /// </summary>
                      public static Regex AllUnicodeLettersNumsHyphen = new Regex(@"^[p{L}p{Nd}-]*$");

                      public static bool IsValid(string s)
                      {
                      return (
                      s.Length >= 6 && s.Length <= 16
                      && AllUnicodeLettersNumsHyphen.IsMatch(s)
                      && s.Count(c => c == '-') <= 1
                      && Char.IsLetter(s, 0)
                      && s[s.Length - 1] != '-'
                      );
                      }


                      (Those parentheses are optional. I find them visually appealing. Follow your group's coding guidelines on style choices like that.)



                      Given the requirements you specify, there's no reason for loops, extension methods, or any of that stuff. I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. The smaller the scope the better when you're reading. It will take someone 30 seconds to fully understand these 6 lines of code. It will take 10 minutes to dive through all yours.



                      And this isn't any less flexible. In fact it's more so. You can trivially add new rules to this 6 line method. That is not the case with your code. Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes.



                      And look how I dealt with the regular expression impenetrability:




                      • Encode as many rules as possible outside the regex.

                      • Only use one fairly simple, appropriately named and documented regex for just the one check: that it matches the character classes. That and a comment are all you need.




                      If you are completely dead set against regular expressions, consider this LINQ based alternative:



                      public static bool IsValid(string s)
                      {
                      return (
                      s.Length >= 6 && s.Length <= 16
                      && s.All(c => Char.IsLetterOrDigit(c) || '-' == c)
                      && s.Count(c => c == '-') <= 1
                      && Char.IsLetter(s, 0)
                      && s[s.Length - 1] != '-'
                      );
                      }





                      share|improve this answer











                      $endgroup$



                      Stop it. Just stop it. Unless you have some kind of unusual performance requirements, keep it simple:



                      /// <summary>
                      /// Regular expression pattern that matches strings containing only Unicode
                      /// letters, digits, and hyphens.
                      /// </summary>
                      public static Regex AllUnicodeLettersNumsHyphen = new Regex(@"^[p{L}p{Nd}-]*$");

                      public static bool IsValid(string s)
                      {
                      return (
                      s.Length >= 6 && s.Length <= 16
                      && AllUnicodeLettersNumsHyphen.IsMatch(s)
                      && s.Count(c => c == '-') <= 1
                      && Char.IsLetter(s, 0)
                      && s[s.Length - 1] != '-'
                      );
                      }


                      (Those parentheses are optional. I find them visually appealing. Follow your group's coding guidelines on style choices like that.)



                      Given the requirements you specify, there's no reason for loops, extension methods, or any of that stuff. I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. The smaller the scope the better when you're reading. It will take someone 30 seconds to fully understand these 6 lines of code. It will take 10 minutes to dive through all yours.



                      And this isn't any less flexible. In fact it's more so. You can trivially add new rules to this 6 line method. That is not the case with your code. Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes.



                      And look how I dealt with the regular expression impenetrability:




                      • Encode as many rules as possible outside the regex.

                      • Only use one fairly simple, appropriately named and documented regex for just the one check: that it matches the character classes. That and a comment are all you need.




                      If you are completely dead set against regular expressions, consider this LINQ based alternative:



                      public static bool IsValid(string s)
                      {
                      return (
                      s.Length >= 6 && s.Length <= 16
                      && s.All(c => Char.IsLetterOrDigit(c) || '-' == c)
                      && s.Count(c => c == '-') <= 1
                      && Char.IsLetter(s, 0)
                      && s[s.Length - 1] != '-'
                      );
                      }






                      share|improve this answer














                      share|improve this answer



                      share|improve this answer








                      edited Jan 28 at 23:39

























                      answered Jan 28 at 23:25









                      jpmc26jpmc26

                      72638




                      72638












                      • $begingroup$
                        I'd tend to agree to use a single regex but in that case everything can be expressed with a single regex without any other surrounding code (including the length and the first/last character)
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:30






                      • 1




                        $begingroup$
                        Hmmmmm POV, I guess. To me a well written regex can express clearly the requirement while UnicodeLettersNumsHyphen has the disadvantages of regexes (less easy to write and read) without its advantages (concise, fast and complete). Nitpick: I'd change UnicodeLettersNumsHyphen to a meaningful name because you can't understand what the regex is for without fully decoding it.
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:38






                      • 2




                        $begingroup$
                        I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. - this is so not true. I wouldn't let this code pass to production.
                        $endgroup$
                        – t3chb0t
                        Jan 29 at 6:21






                      • 1




                        $begingroup$
                        Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes. - not that has to be tested but one that can be tested. Your code isn't testable and is so easy to make a mistake there that you wouldn't notice until something goes wrong for the client. This isn't an improvement. It's a great degradation of both readability and testability.
                        $endgroup$
                        – t3chb0t
                        Jan 29 at 6:23








                      • 3




                        $begingroup$
                        @t3chb0t This is testable...
                        $endgroup$
                        – Tvde1
                        Jan 29 at 12:44


















                      • $begingroup$
                        I'd tend to agree to use a single regex but in that case everything can be expressed with a single regex without any other surrounding code (including the length and the first/last character)
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:30






                      • 1




                        $begingroup$
                        Hmmmmm POV, I guess. To me a well written regex can express clearly the requirement while UnicodeLettersNumsHyphen has the disadvantages of regexes (less easy to write and read) without its advantages (concise, fast and complete). Nitpick: I'd change UnicodeLettersNumsHyphen to a meaningful name because you can't understand what the regex is for without fully decoding it.
                        $endgroup$
                        – Adriano Repetti
                        Jan 28 at 23:38






                      • 2




                        $begingroup$
                        I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. - this is so not true. I wouldn't let this code pass to production.
                        $endgroup$
                        – t3chb0t
                        Jan 29 at 6:21






                      • 1




                        $begingroup$
                        Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes. - not that has to be tested but one that can be tested. Your code isn't testable and is so easy to make a mistake there that you wouldn't notice until something goes wrong for the client. This isn't an improvement. It's a great degradation of both readability and testability.
                        $endgroup$
                        – t3chb0t
                        Jan 29 at 6:23








                      • 3




                        $begingroup$
                        @t3chb0t This is testable...
                        $endgroup$
                        – Tvde1
                        Jan 29 at 12:44
















                      $begingroup$
                      I'd tend to agree to use a single regex but in that case everything can be expressed with a single regex without any other surrounding code (including the length and the first/last character)
                      $endgroup$
                      – Adriano Repetti
                      Jan 28 at 23:30




                      $begingroup$
                      I'd tend to agree to use a single regex but in that case everything can be expressed with a single regex without any other surrounding code (including the length and the first/last character)
                      $endgroup$
                      – Adriano Repetti
                      Jan 28 at 23:30




                      1




                      1




                      $begingroup$
                      Hmmmmm POV, I guess. To me a well written regex can express clearly the requirement while UnicodeLettersNumsHyphen has the disadvantages of regexes (less easy to write and read) without its advantages (concise, fast and complete). Nitpick: I'd change UnicodeLettersNumsHyphen to a meaningful name because you can't understand what the regex is for without fully decoding it.
                      $endgroup$
                      – Adriano Repetti
                      Jan 28 at 23:38




                      $begingroup$
                      Hmmmmm POV, I guess. To me a well written regex can express clearly the requirement while UnicodeLettersNumsHyphen has the disadvantages of regexes (less easy to write and read) without its advantages (concise, fast and complete). Nitpick: I'd change UnicodeLettersNumsHyphen to a meaningful name because you can't understand what the regex is for without fully decoding it.
                      $endgroup$
                      – Adriano Repetti
                      Jan 28 at 23:38




                      2




                      2




                      $begingroup$
                      I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. - this is so not true. I wouldn't let this code pass to production.
                      $endgroup$
                      – t3chb0t
                      Jan 29 at 6:21




                      $begingroup$
                      I promise you that 2 years down the line, anyone reading this code (including you!) will be much happier understanding the 5 liner than your code. - this is so not true. I wouldn't let this code pass to production.
                      $endgroup$
                      – t3chb0t
                      Jan 29 at 6:21




                      1




                      1




                      $begingroup$
                      Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes. - not that has to be tested but one that can be tested. Your code isn't testable and is so easy to make a mistake there that you wouldn't notice until something goes wrong for the client. This isn't an improvement. It's a great degradation of both readability and testability.
                      $endgroup$
                      – t3chb0t
                      Jan 29 at 6:23






                      $begingroup$
                      Your patterns require adding 20 extra lines of boilerplate that then has to be tested and presents more opportunities for mistakes. - not that has to be tested but one that can be tested. Your code isn't testable and is so easy to make a mistake there that you wouldn't notice until something goes wrong for the client. This isn't an improvement. It's a great degradation of both readability and testability.
                      $endgroup$
                      – t3chb0t
                      Jan 29 at 6:23






                      3




                      3




                      $begingroup$
                      @t3chb0t This is testable...
                      $endgroup$
                      – Tvde1
                      Jan 29 at 12:44




                      $begingroup$
                      @t3chb0t This is testable...
                      $endgroup$
                      – Tvde1
                      Jan 29 at 12:44


















                      draft saved

                      draft discarded




















































                      Thanks for contributing an answer to Code Review Stack Exchange!


                      • Please be sure to answer the question. Provide details and share your research!

                      But avoid



                      • Asking for help, clarification, or responding to other answers.

                      • Making statements based on opinion; back them up with references or personal experience.


                      Use MathJax to format equations. MathJax reference.


                      To learn more, see our tips on writing great answers.




                      draft saved


                      draft discarded














                      StackExchange.ready(
                      function () {
                      StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f212381%2fversatile-string-validation%23new-answer', 'question_page');
                      }
                      );

                      Post as a guest















                      Required, but never shown





















































                      Required, but never shown














                      Required, but never shown












                      Required, but never shown







                      Required, but never shown

































                      Required, but never shown














                      Required, but never shown












                      Required, but never shown







                      Required, but never shown







                      Popular posts from this blog

                      Can a sorcerer learn a 5th-level spell early by creating spell slots using the Font of Magic feature?

                      Does disintegrating a polymorphed enemy still kill it after the 2018 errata?

                      A Topological Invariant for $pi_3(U(n))$