Problem
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?
Solution
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.
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.
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.
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.