# Input validation for text boxes in a Form

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?

-
Look into string.IsNullOrEmpty() or string.IsNullOrWhiteSpace(). – Jesse C. Slicer Jun 18 '14 at 14:37
@JesseC.Slicer, saw your comment right after I posted....lol – Malachi Jun 18 '14 at 19:24

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.

-
In your validating(), I see the condition is wrong. Now if text is not empty, it shows the message!! – CAD Jun 19 '14 at 8:43
Also, even I changed the condition, still some thing missing!. Now I cant focus to next text box? – CAD Jun 19 '14 at 8:46
Message.show(); should be in If block not in Else. Then its ok ;) – CAD Jun 19 '14 at 8:59
+1 for using the built-in validation mechanics! – Mat's Mug Jun 19 '14 at 11:26
@Chathuranga fixed the messagebox it should work better now. – tinstaafl Jun 19 '14 at 13:14
 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;
}


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.

-
I was waiting for you to post an answer after I read all those wonderful comments – Malachi Jun 18 '14 at 20:14
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? – CAD Jun 19 '14 at 11:14

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...

-

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.

-