Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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?

share|improve this question
6  
Look into string.IsNullOrEmpty() or string.IsNullOrWhiteSpace(). –  Jesse C. Slicer Jun 18 at 14:37
    
@JesseC.Slicer, saw your comment right after I posted....lol –  Malachi Jun 18 at 19:24
add comment

4 Answers 4

up vote 3 down vote accepted

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.

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

share|improve this answer
    
I was waiting for you to post an answer after I read all those wonderful comments –  Malachi Jun 18 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? –  Chathuranga Jun 19 at 11:14
add comment

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

share|improve this answer
add comment

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.

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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