2
\$\begingroup\$

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
            }
    }
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – tinstaafl Jun 19 '14 at 13:22
  • 1
    \$\begingroup\$ You could use the ErrorProvider control to do what @tinstaafl suggests. \$\endgroup\$ – Dan Lyons Jun 19 '14 at 16:59
2
\$\begingroup\$

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

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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