5
\$\begingroup\$

In a Win Form I'm doing initial validation in the Form. So before saving data, I want to validate whether all the required fields are filled (Text Boxes) by the user. There are about 18 such Text Boxes in the Form. Currently I'm doing it as follows. To make the code short only three fields are shown in the code.

     private void cmbSave_Click(object sender, EventArgs e)
     {
         if(IsFilled (txtApplicationNumber.Text))
         { 
             if(IsFilled (txtEmployeeID.Text))
             {
                 if (IsFilled(txtNIC.Text))
                 Save(sender, e);
             }
         }                          
    }


    private bool IsFilled(string s)
    {
        if (s != "")
        { return true; }
        else
        { return false; }
    }

Another option would be to use the logic inside the cmbSave_Click straightaway as follows,

    private void cmbSave_Click(object sender, EventArgs e)
    {
        If (txtApplicationNumber.Text!="" & txtEmployeeID.Text!="" & txtNIC.Text="")
        Save(sender, e)
    }

Basically the code should be readable, consistent and should perform the validation properly.

Which option could be considered better? If both are not good please propose a way to do this?

\$\endgroup\$
2
  • 6
    \$\begingroup\$ Look into string.IsNullOrEmpty() or string.IsNullOrWhiteSpace(). \$\endgroup\$ Jun 18, 2014 at 14:37
  • \$\begingroup\$ @JesseC.Slicer, saw your comment right after I posted....lol \$\endgroup\$
    – Malachi
    Jun 18, 2014 at 19:24

5 Answers 5

10
\$\begingroup\$

Another option would be to handle the Validating event:

private void textBox_Validating(object sender, CancelEventArgs e)
{
    TextBox currenttb = (TextBox)sender;
    if(currenttb.Text == "")
        MessageBox.Show(string.Format("Empty field {0 }",currenttb.Name.Substring(3)));
        e.Cancel = true;
    else
    {
        e.Cancel = false;
    }
}

Adding the handler to the textboxes is easily done with a foreach loop in the form constructor:

foreach(TextBox tb in this.Controls.OfType<TextBox>().Where(x => x.CausesValidation == true))
{
    tb.Validating += textBox_Validating;
}

Now the user can't leave a textbox empty. The handler returns the focus back to the textbox. To exclude a textbox from being validated simply set the CausesValidation property to false.

\$\endgroup\$
5
  • \$\begingroup\$ In your validating(), I see the condition is wrong. Now if text is not empty, it shows the message!! \$\endgroup\$
    – CAD
    Jun 19, 2014 at 8:43
  • \$\begingroup\$ Also, even I changed the condition, still some thing missing!. Now I cant focus to next text box? \$\endgroup\$
    – CAD
    Jun 19, 2014 at 8:46
  • \$\begingroup\$ Message.show(); should be in If block not in Else. Then its ok ;) \$\endgroup\$
    – CAD
    Jun 19, 2014 at 8:59
  • \$\begingroup\$ Shouldn't you be validating data types like date or length? \$\endgroup\$
    – JustJohn
    Aug 10, 2020 at 22:38
  • \$\begingroup\$ @JustJohn - That is outside the scope of this post. The OP is concerned about making sure the fields are filled in. \$\endgroup\$
    – user33306
    Aug 12, 2020 at 13:26
6
\$\begingroup\$
 private void cmbSave_Click(object sender, EventArgs e)
 {
     if(IsFilled (txtApplicationNumber.Text))
     { 
         if(IsFilled (txtEmployeeID.Text))
         {
             if (IsFilled(txtNIC.Text))
             Save(sender, e);
         }
     }                          
}

As @Malachi pointed out, the nesting is easily avoidable here. However it's still mixed abstraction levels - "Save" is more abstract than "is any of the txtApplicationNumber.Text, txtEmployeeID.Text and txtNIC.Text values null or empty?".

Being consistent about abstraction levels makes the code much easier to read:

private void cmbSave_Click(object sender, EventArgs e)
{
    if (ValidateForm())
    {
        Save(sender, e);
    }
}

Where ValidateForm() is at a lower abstraction level and needs to know about the existence of textboxes on the form:

private bool ValidateForm()
{
    var textBoxes = groupBox3.Controls.Cast<Control>()
                             .OfType<TextBox>()
                             .OrderBy(control => control.TabIndex);

    foreach(var textBox in textBoxes)
    {
        if (string.IsNullOrWhiteSpace(textBox.Text))
        {
           textBox.Focus();

           // remove "txt" prefix:
           var fieldName = textBox.Name.SubString(3);
           MessageBox.Show(string.Format("Field '{0}' cannot be empty.", fieldName));

           return false;
        }
    }

    return true;
}

Just a note about this snippet:

If (txtApplicationNumber.Text!="" & txtEmployeeID.Text!="" & txtNIC.Text="")

& makes a bitwise AND, which I really doubt you intend to be doing here. As @Malachi pointed out, use && to perform logical AND.

"" works. string.Empty works just as well, and explicitly communicates your intent of verifying against an empty string. However whenever you start typing someString != "" or someString != string.Empty, I'd warmly recommend using IsNullOrEmpty(someString) instead. Making this a habit will prevent having to debug some unexpected NullReferenceException later, under different circumstances: remember that a string is a reference type, which means it's allowed to be null.

\$\endgroup\$
2
  • \$\begingroup\$ I was waiting for you to post an answer after I read all those wonderful comments \$\endgroup\$
    – Malachi
    Jun 18, 2014 at 20:14
  • \$\begingroup\$ What would you think about @tinstaafl's answer? I feel implementation and maintenance would be very easy in this way as we use single Handler for all TextBoxes. So we could have separate handlers for ComboBoxes, MaskedTextBoxes etc. Is this is a good practice? If we compare two methods what would be the most suitable option? \$\endgroup\$
    – CAD
    Jun 19, 2014 at 11:14
4
\$\begingroup\$

You have some extra stuff here that you don't need.

like 3 if statements worth of nesting. reduce it like this

     private void cmbSave_Click(object sender, EventArgs e)
     {
         if(IsFilled (txtApplicationNumber.Text) && IsFilled (txtEmployeeID.Text) && IsFilled(txtNIC.Text))
         { 
             Save(sender, e);
         }                       
    }

That makes the code more readable, straight to the point. I don't think it will give you any performance though as it is probably the same exact thing that you have.

BTW.Normal: you should have && in your conditionals and not &


And this piece of code can be changed a little bit as well

private bool IsFilled(string s)
{
    if (s != "")
    { return true; }
    else
    { return false; }
}

to something like this

private bool IsFilled(string s)
{
    if (s.isNullOrEmpty())
    {
         return false;
    }
    return true;
}

no need to write the extra else statement, because if the string is empty or null it will return false and not execute the rest of the code anyway.


I looked at your Answer as well, and I think you can get rid of some nesting there as well.

private void cmbSave_Click(object sender, EventArgs e)
{
    foreach (Control c in groupBox3.Controls.Cast<Control>().OrderBy(c => c.TabIndex))
    {
        if (c is TextBox && string.IsNullOrWhiteSpace(c.Text))
        {
            MessageBox.Show(string.Format("Empty field {0}",c.Name.Substring(3)));
            c.Focus();
            return;                
        }
    }
    Save(sender, e);
}

just a little bit cleaner...

\$\endgroup\$
1
\$\begingroup\$

I would rather go for a simple approach to do this, which makes your code cleaner, more readable and more maintainable:

private void cmbSave_Click(object sender, EventArgs e)
    {
        var controls = new[] { txtApplicationNumber.Text, txtEmployeeID.Text, txtNIC.Text };
        if (controls.All(x => string.IsNullOrEmpty(x)))
            Save(sender, e);
    }

I have kept all the values in a string array for which I have to check for null or empty, and used a simple extension method All to return true only when all the members of the string array satisfied the condition.

\$\endgroup\$
1
\$\begingroup\$

I would rather use a generic function that would be passed each control on the form and based on the type of the control, separate validation logic would be written. in case extra validation like restricting validation to some of the controls only, that would be decided at the function calling level.

\$\endgroup\$

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