current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 2 down vote favorite

In input validation I'm using this validation method in my all Forms. Here I have a single handler for all TextBoxes in my Form and if I have other controllers like ComboBoxes I would use a separate handler for all ComboBoxes and so on.

My only doubt in this approach is how users would feel about this as you can't focus on other text boxes while keeping the current one empty if it's not an optional field!

Could you please review this and provide your feedback?

public partial class frmForm : Form
    {
    public frmForm()
            {
                InitializeComponent();

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



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

                    else
                    {
                        e.Cancel = false;
                    }
                }

            }


            private enum OptionalFields
            {
                txtPolicy, txtRemarks
            }
    }
share|improve this question
1  
Personally I think users would rather be reminded of an omission right away rather than waiting until all the rest of the inputs are done. One thing you can do is put a label with an * beside the required fields. This a common practice for input forms. This way the user knows what is expected. –  tinstaafl Jun 19 at 13:22
1  
You could use the ErrorProvider control to do what @tinstaafl suggests. –  Dan Lyons Jun 19 at 16:59
add comment

1 Answer

up vote 2 down vote accepted

Just a few random observations:

foreach (TextBox tb in this.Controls.OfType<TextBox>().Where(x => x.CausesValidation == true))

tb is a bad name; without the explicit TextBox type one would have to read the entire part after the in, to figure out that we're talking about a TextBox. And it's not uncommon for loops to use implicit types, like this:

foreach (var tb in this.Controls.OfType<TextBox>().Where(x => x.CausesValidation == true))

Now in this part:

.Where(x => x.CausesValidation == true)

You're comparing a bool to true - that's a no-no. Write it like this instead:

.Where(x => x.CausesValidation)

It's probably also more readable to iterate over an identifier instead of an expression; I'd split it like this:

var textBoxes = Controls.OfType<TextBox>()
                        .Where(textBox => textBox.CausesValidation);

foreach (var textBox in textBoxes)
{
    // ...
}

Kudos for IsNullOrWhiteSpace:

if (string.IsNullOrWhiteSpace(currenttb.Text))

However, currenttb should be camelCase, and would be more readable as currentTextBox.

Careful with extra spaces:

string.Format("Empty field {0 }"

I don't think this is legal, should be "Empty field {0}".

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.