3

I have an issue in that I'm implementing approximately 5 validation checks in a form. Each of these is located in its own method Is_XXX_Valid(). I'm looking for a way to establish that each of the methods return true; otherwise an error message should be displayed.

This however is where the problem arises, I have a solution which works partially, in that it fails to run the subsequent methods if a previous method has returned false

Here is the current code which I am using:

private void Button_Click(object sender, EventArgs e)
  {
    DialogResult validation_msgbox = MessageBox.Show("Are you sure you would like to submit this form?", "Submit Form?", MessageBoxButtons.YesNo);

    // Run each validaion check

    if (IsAAAValid() && IsBBBValid())
    {
      //Continue and submit data
    }
    else
    {
      //Display the errors
      DialogResult Textbox_validation = MessageBox.Show(ErrorText, "Some errors were found.", MessageBoxButtons.OK);
    }
  }

Using the code above as an example, if IsAAAValid() returns false, then the second method is not executed and therefore the data within is not validated, leading to an incorrect dialog box if multiple errors are found.

Thanks!

3
  • 1
    && is short-circuiting, so the first false return will cause any subsequent conditionals not to execute. Simply use & instead, as it is not short-circuiting. Dec 21, 2015 at 19:28
  • 1
    Can't you just do bool variables for each method call, and do an if () over the bool variables?
    – wentimo
    Dec 21, 2015 at 19:29
  • 2
    If you truly need all methods to run, then I think your method's responsibilities are not well named/defined. I wouldn't expect a method named IsXXX to have side effects like what you are implying.
    – sstan
    Dec 21, 2015 at 19:29

3 Answers 3

8

This is known as "short-circuit evaluation," which you can read about here and is a usually-desirable feature of the C# programming language. You can get around it with something like this:

bool avalid = IsAAAValid();
bool bvalid = IsBBBValid();
if (avalid && bvalid)
{
  //Continue and submit data
}

This will guarantee that both methods get run.

As a side note, for clarity in your code I would recommend that you rename your validation methods to signify that they have side-effects. That is, they do more than simply returning the state of the data; they actually have the potential to modify state. This is why short-circuit evaluation causes problems in this instance.

6

As adv12 has answered, this is called short-circuit evaluation but there is also an alternative to the way he refactored your code.

There are two boolean and operators:

  • && - short-circuit evaluation
  • & - full evaluation

So you can simply switch to using & to call both methods regardless:

if (IsAAAValid() & IsBBBValid())
                 ^
                 |
                 +-- only one &, not two &&

Now, having said that, personally I would write the code as adv12 did as it makes for easier to read code, not as easy to spot the use of only the one & but I thought I would post the answer for completeness.

2
  • 2
    Nice answer, especially recommendation to not do that :). It is pretty much guaranteed that next person who see the code will replace & with && and call original author names... and than week later spend a day trying to figure out why code no longer reports errors correctly. Dec 21, 2015 at 19:34
  • @AlexeiLevenkov yeah, relying on side effects within a boolean expression is pretty sketchy. But breaking things up and recording results is more readable.
    – ryanyuyu
    Dec 21, 2015 at 19:36
4

You could simply call every method and sets a boolean value to false if any of the methods return false. At the end you could check this boolean value and display your error message.

bool isValid = true;
if(!IsValidA()) isValid = false;
if(!IsValidB()) isValid = false;
if(!IsValidC()) isValid = false;
if(!IsValidD()) isValid = false;
if(!IsValidE()) isValid = false;
if(!isValid)
    MessageBox.Show("Global validation error message");

However I prefer a better approach in which you use a List<string> to accumulate error messages and print them all at the end

List<string> errors = new List<string>();
if(!IsValidA()) errors.Add("Fail on IsValidA");
if(!IsValidB()) errors.Add("Fail on IsValidB");
if(!IsValidC()) errors.Add("Fail on IsValidC");
if(!IsValidD()) errors.Add("Fail on IsValidD");
if(!IsValidE()) errors.Add("Fail on IsValidE");

if(errors.Count > 0)
{
    string message = string.Join(Environment.NewLine, errors.ToArray());
    MessageBox.Show("Validation errors found:" + Environment.NewLine + message);
}

I think this is better from a user perspective because you could inform her/him of the problems found in a single message and avoid that horrible user experience that happens when you tell to your user about a single problem, the user fixes the problem only to be awarded with another error message about another problem.

3
  • 1
    Replacing the count in the first code example with a boolean, e.g. notValid, would remove the need to test for 5 and update the value if the tests change during future code maintenance. The second code example is highly recommended for communicating useful information back to the user.
    – HABO
    Dec 21, 2015 at 19:38
  • right, it will be a little more confusing but nevertheless you are correct
    – Steve
    Dec 21, 2015 at 19:55
  • Would using isValid &= IsValidA(); ... be more appealing? It doesn't short circuit.
    – HABO
    Dec 22, 2015 at 3:18

Your Answer

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

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