Calling all the 3 functions while using or operator even after returning true as a result.

Question

I am calling three functions in my code where i want to validate some of my fields. When I tries to work with the code given below. It checks only for first value until it gets false result.

I want some thing like that if fisrt function returns true then it should also call next function and so on. What can be used instead of Or Operator to do this.

    if (IsFieldEmpty(ref txtFactoryName, true, "Required") ||
        IsFieldEmpty(ref txtShortName, true, "Required") ||
        IsFieldEmpty(ref cboGodown, true, "Required"))
    { }

EDIT

public bool IsFieldEmpty(ref TextBox txtControl, Boolean SetErrorProvider,string msgToShowOnError)
{
    ErrorProvider EP = new ErrorProvider();
    if (txtControl.Text == string.Empty)
    {
        EP.SetError(txtControl, msgToShowOnError);
        return true;
    }
    else
    {
        EP.Clear();
        return false;
    }
}

Please comment, Is this method fine using ref variable as one of the parameter.

I am checking for validation onSubmit event in winform.

Answer 1

You can use the single | for OR:

 if (IsFieldEmpty(ref txtFactoryName, true, "Required") |
    IsFieldEmpty(ref txtShortName, true, "Required") |
    IsFieldEmpty(ref cboGodown, true, "Required"))
{ }

The double-pipe || is performing short-circuit evaluation, the single version | does full evaluation.
The same for && and &.

See the MSDN reference.

Response to the Edit:

1. There is no need for the 'ref' in front of txtControl, and removing that would go a long way in addressing the criticism on your approach here. IsFieldEmpty makes no changes to txtControl. You could rename to CheckFieldEmpty to improve it a little further.
2. It is strange that you create an ErrorProvider instance inside this method, does that work at all? There should normally be one (permanent) instance on the Form. You probably want this method to be independent of the Form, so just add an EP as a parameter. It can replace the SetErrorProvider, you can check the EP parameter for null. O, and replace EP.Clear(); with Ep.SetErrortxtControl, "");

Answer 2

Make it explicit what you're doing:

bool isFactoryNameEmpty = IsFieldEmpty(ref txtFactoryName, true, "Required");
bool isShortNameEmpty = IsFieldEmpty(ref txtShortName, true, "Required");
bool isGodownEmpty = IsFieldEmpty(ref cboGodown, true, "Required");
if (isFactoryNameEmpty || isShortNameEmpty || isGodownEmpty)
{
    // ...
}

(Also, I assume you need to call all three functions because they have side-effects? In which case IsFieldEmpty is a really bad name.)

Answer 3

What you are seeing is called short circuiting in C#. If the first expression fails, then it won't bother trying the next ones, because the final result has already been determined.

http://johnnycoder.com/blog/2006/08/02/short-circuit-operators-in-c/

You should you | instead of || to get your result.

 if (IsFieldEmpty(ref txtFactoryName, true, "Required") |
        IsFieldEmpty(ref txtShortName, true, "Required") |
        IsFieldEmpty(ref cboGodown, true, "Required"))

C# operators http://msdn.microsoft.com/en-us/library/6a71f45d.aspx

|| operator. http://msdn.microsoft.com/en-us/library/6373h346.aspx

| operator. http://msdn.microsoft.com/en-us/library/kxszd0kx.aspx

Answer 4

Why would you need it to? The only reason I can think of is that your "IsFieldEmpty" function is also doing some calculations or changes to the data, and that worries me. A function named "IsFieldEmpty" really shouldn't be doing anything else.

In that case, from a usability/maintainability viewpoint, you'd be better off with:

SomeFieldMaintenance(ref txtFactoryName, true, "Required")
SomeFieldMaintenance(ref txtShortName, true, "Required")
SomeFieldMaintenance(ref cboGodown, true, "Required")
if (IsFieldEmpty(txtFactoryname) ||
    IsFieldEmpty(txtShortName) ||
    IsFieldEmpty(cboGodown))
{ }

Or something of that ilk.

Answer 5

The answers so far have assumed that you want to validate all the fields, even after one of them fails. This assumption is not explicit in your original question. So, if you don't mind stopping validation when one field fails, then the simplest solution is to use the && operator instead of ||. This will achieve your stated goal: "if first function returns true then it should also call next function and so on". However, if the first function returns false, then none of the other functions will be called, which may not be what you want.