Validation function code review

Question

Any suggestions on cleaning up this code:

 public override bool Validate(Control control, object value)
    {
      bool res = false;
      string str = string.Empty;

      try
      {
        if (((string)value).Length > 0)
          str = value.ToString();
      }
      catch (Exception e)
      {
        res = false;
        this.ErrorText = "Bad values " + e.Message;
        this.ErrorType = ErrorType.Critical;
      }

      //Is it empty?
      if (String.IsNullOrEmpty(str))
      {
        res = false;
        this.ErrorType = ErrorType.Critical;
      }

      Regex RegNrShortRegexDarm = new Regex(@"([F][A][D][-][Z]\d{3})");
      Regex RegNrLongRegexDarm = new Regex(@"([F][A][D][-][Z]\d{3}[-][0-9]+$)");
      Regex RegNrShortRegexBrust = new Regex(@"([F][A][B][-][Z]\d{3})");
      Regex RegNrLongRegexBrust = new Regex(@"([F][A][B][-][Z]\d{3}[-][0-9]+$)");

      if (selectedItem == Constants.Item1)
      {
        this.ErrorText = "The correct form is: FAD-Zxxx oder FAD-Zxxx-x !";

        //has the form FAD-Zxxx
        if (RegNrShortRegexDarm.IsMatch(str) && str.Length == 8)
        {
          // Number is valid
          res = true;
        }

        //has the form FAD-Zxxx-x
        if (RegNrLongRegexDarm.IsMatch(str) && str.Length >= 10 && str.Length <= 12)
        {
          // Number is valid
          res = true;
        }
      }
      else if (selectedItem == Constants.Item2)
      {
        this.ErrorText = "The correct form is: FAB-Zxxx oder FAB-Zxxx-x !";

        //has the form FAB-Zxxx
        if (RegNrShortRegexBrust.IsMatch(str) && str.Length == 8)
        {
          // Number is valid
          res = true;
        }

        //has the form FAB-Zxxx-x
        if (RegNrLongRegexBrust.IsMatch(str) && str.Length >= 10 && str.Length <= 12)
        {
          // Number is valid
          res = true;
        }
      }

      return res;
    }

Later Edit:

I've reached the following solution:

public override bool Validate(Control control, object value)
{
  var str = value as string;
  var letter = (selectedItem == Constants.Item1) ? "D" : "B";
  var errText = String.Format("The correct form is: FA{0}-Zxxx or FA{0}-Zxxx-x !", letter);

  if (string.IsNullOrWhiteSpace(str))
  {
    this.ErrorType = ErrorType.Critical;
    this.ErrorText = errText;
    return false;
  }

  string regExString = String.Format(@"FA{0}-Z\d{{3}}", letter);
  Regex shortRegEx = new Regex(regExString);
  Regex longRegEx = new Regex(regExString + @"-\d+$");

  bool isMatch = ((shortRegEx.IsMatch(str) && IsValidShortLength(str)) || (longRegEx.IsMatch(str) && IsValidLongLength(str)));

  if (!isMatch)
  {
    this.ErrorText = errText;
  }
  return isMatch;

}

private static bool IsValidShortLength(string str)
{
  return str.Length == 8;
}

private static bool IsValidLongLength(string str)
{
  return (str.Length >= 10 && str.Length <= 12);
}

Answer 1

The first check in the try block is not needed at all. It just checks if the value is string, and it's unlikely to be something else. If this validation is attached to a control, who's value is not a string, then this is an error, and I would throw an exception "Bad values", so the code is corrected.

Then, if the value is null or empty, you have an error, why would you inspect all the other possibilities?

Also - where you use the control itself? Why you pass it, if you do not use it?

Anyway, if I was to check the string for valid formatting, I would do this:

        var str = value as String;

        var letter = (selectedItem == Constants.Item1) ? "D" : "B";
        var errText = String.Format("The correct form is: FA{0}-Zxxx oder FA{0}-Zxxx-x !", letter);

        if (str == null 
            || str.Length < 8 
            || !(str.Length >= 10 && str.Length <=12))
        {
            this.ErrorType = ErrorType.Critical;
            this.ErrorText = errText;
            return false;
        }

        var regExString = @"([F][A][" + letter + @"][-][Z]\d{3})";
        Regex shortRegEx = new Regex(regExString);
        Regex longRegEx = new Regex(regExString + @"[-][0-9]+$)");

        var isMatch = (shortRegEx.IsMatch(str) || longRegEx.IsMatch(str));
        if (!isMatch)
        {
            this.ErrorText = errText;
        }

        return isMatch;

Answer 2

Just to pick on the regexes (since other people are picking up some of the other stuff I'd have commented):

  Regex RegNrShortRegexDarm = new Regex(@"([F][A][D][-][Z]\d{3})");
  Regex RegNrLongRegexDarm = new Regex(@"([F][A][D][-][Z]\d{3}[-][0-9]+$)");
  Regex RegNrShortRegexBrust = new Regex(@"([F][A][B][-][Z]\d{3})");
  Regex RegNrLongRegexBrust = new Regex(@"([F][A][B][-][Z]\d{3}[-][0-9]+$)");

Just ditch the classes for single characters: [F] is overkill. And be consistent about the class for digits. Also, there's no point using a group for the entire expression.

  Regex RegNrShortRegexDarm = new Regex(@"FAD-Z\d{3}");
  Regex RegNrLongRegexDarm = new Regex(@"FAD-Z\d{3}-\d+$");
  Regex RegNrShortRegexBrust = new Regex(@"FAB-Z\d{3}");
  Regex RegNrLongRegexBrust = new Regex(@"FAB-Z\d{3}-\d+$");

And then given the way you actually use the things it would be more sensible to have

  Regex RegNrDarm = new Regex(@"FAD-Z\d{3}(-\d{1,3})?");
  Regex RegNrBrust = new Regex(@"FAB-Z\d{3}(-\d{1,3})?");

Answer 3

I can't believe I'm saying this, but you have too much error handling going on. The first 15 lines or so all deals with converting value to a string.

just do something like...

string str = value as string;
if(string.IsNullOrWhiteSpace(str))
{        
    this.ErrorType = ErrorType.Critical;
    return false;
}

Also, return false immediately when you encounter an error condition. You continue to attempt to process the string even though you know the string is in an invalid state.

Name your variables according to the C# guidelines. All variables should be camel-cased, not Pascal-cased.

Use the alias string rather than String.

Answer 4

Other people have commented on your try/catch and whether or not it is even applicable, but I don't believe it has been mentioned that you shouldn't catch Exception, but rather something you can actually handle and might expect. Given your code, I might expect to run into a NullReferenceException or an InvalidCastException. The fact is you can avoid both of those, but assuming you could not, those would be the types you would want to catch. Other exceptions should bubble up.

Beyond that, I see a long method. Break that down into smaller, descriptive parts.

Another thing I see is

if (selectedItem == x)
{
    // several lines of code
}
else if (selectedItem == y)
{
    // several lines of code
}

The bodies of these blocks at the very least are strong candidates to be refactored out. Perhaps ValidateForItem1, ValidateForItem2, or something along those lines. Just an idea to get you started.

I see that in an upper section of code you set res = false when str is null or empty. But later you can set res to true. Whether or not it is even possible for later sections of code to override the initial value, I don't want to even have to think about it. If it's OK to override, that's one thing. If it isn't (even if it isn't even possible), don't make me have to perform the mental gymnastics to confirm to myself that the value will not change.

My (hopefully) final observation is I see this in multiple places

// Number is valid
res = true;

You know what this comment tells me? It tells me that res is a terrible name for the variable. res tells me nothing. Rename the variable to something that actually tells me what information it is supposed to convey, and then you can get rid of useless comments.

isValidNumber = true; 

OK, so that directly leads me to another point.

//Is it empty?       
if (String.IsNullOrEmpty(str)) 

Ya think?

I don't need a comment to tell me what the line of code tells me. Save your comments for things that actually do need to be explained. Perhaps comments are useful for regular expressions, because who wants to think about those. Provide high level comments for tricky pieces of code, or say why you chose algorithm X over Y, if you feel the need. Don't provide comments for what the code already tells me (and refactor the code when the code should be able to tell me what I need to know, as in the case of res).

Answer 5

After combining some of the other answers and some very minor stylistic tweaks we get:

 public override bool Validate(Control control, object value)
 {
    var str = value as string;
    var letter = (selectedItem == Constants.Item1) ? "D" : "B";
    var errText = String.Format("The correct form is: FA{0}-Zxxx oder FA{0}-Zxxx-x !", letter);

    if (string.IsNullOrEmpty(str))
    {
        this.ErrorType = ErrorType.Critical;
        this.ErrorText = errText;
        return false;
    }

    string pattern = @"(FA" + letter + @"-Z\d{3}(-\d{1,3})?";
    bool isMatch = Regex.IsMatch(str, pattern);

    if (!isMatch)
    {
          this.ErrorText = errText;
    }
    return isMatch;
}

As part of the combining, I switched to using the static Regex.IsMatch method, because we got down to only one RegEx, and creating an object for it seemed like overkill.

I might consider using a format string for regExString, although I think that would make readibility worse, so I would probably skip it.

I would avoid mixing languages in my error message. The English word is "or", not "oder" which is German. It might be best to internationalize error messages, which implies fetching that format string from a resource file, rather than hardcoding it.

The above is probably otherwise getting pretty close to optimal. This function is also not quite the same as the original though, since it makes the assumption that selectedItem is always either Constants.Item1 or Constants.Item2. If that change is not acceptable, the method above should be changed appropriately.