6
\$\begingroup\$

I always end up creating a quick return statement above a code block if the simple condition of textbox null fields are found. Naturally, these tend to build up the more controls are added. Now I'm staring at this mess of code, and I'm looking for advice as to avoid this very common mistake in the future.

This is the spaghetti code of a small winform C# project that I'm sure will trigger more than a few people:

   if (txtFirst.Text == "" || txtLast.Text == "" || txtGross.Text == "" ||
        txtLessTNT.Text == "" || txtTCI.Text == "" || txtADDTI.Text == "" || txtGTI.Text == "" ||
        txtLessTE.Text == "" || txtLessPPH.Text == "" || txtLessNTI.Text == "" || txtTD.Text == "" ||
        txtTWCE.Text == "" || txtTWPE.Text == "" || txtTATW.Text == "" ||
        txtFirst.Text == " " || txtLast.Text == " " || txtGross.Text == " " ||
        txtLessTNT.Text == " " || txtTCI.Text == " " || txtADDTI.Text == " " || txtGTI.Text == " " ||
        txtLessTE.Text == " " || txtLessPPH.Text == " " || txtLessNTI.Text == " " || txtTD.Text == " " ||
        txtTWCE.Text == " " || txtTWPE.Text == " " || txtTATW.Text == " " || txtID.Text == "" ||
        txtID.Text == " " || txtTIN.Text == "" || txtTIN.Text == " " || txtFrom.Text == " " || txtFrom.Text == "" || txtTo.Text == " " || txtTo.Text == "" || txtCTC.Text == " " || txtCTC.Text == "" || txtCTC.Text == " " || txtPOI.Text == "" || txtPOI.Text == " " || txtDOI.Text == "" || txtDOI.Text == " " || txtDOI.Text == "" || txtAMT.Text == " " || txtAMT.Text == ""
    )


    { 
        MessageBox.Show("Cannot enter null values!");
        return;
    }

Surely, there is an easier way to do this that allows the code to be modular and concise that permits the addition of new textbox controls. This happens more often than I'd like to admit and I wish to get rid of this habit once and for all.

Note that, I do have another solution, it's a linq block that iterates through ALL textbox controls in a given form. But what I'm looking for is as I mentioned, code that allows for selection of the controls involved.

\$\endgroup\$
  • 3
    \$\begingroup\$ I think it doesn't qualify to be on CodeReview \$\endgroup\$ – Paweł Łukasik Jan 26 '17 at 10:42
  • 1
    \$\begingroup\$ More modular? Yes, definitely: create a subclass of the TextBox control, add your code there, and use that NonNullTextBox control class where you want this behavior instead of the built-in one. I'd have posted this as a complete solution in an answer, but I'm not sure what event you would want to put this logic in—when do you want to test that the contents are non-null? During validation? What is the purpose of this code? Also, note that this is pretty bad design. You should let the user enter whatever they want, and check it all when they try to submit the form. \$\endgroup\$ – Cody Gray Jan 26 '17 at 11:20
  • \$\begingroup\$ This does return the user after the form is submitted, not while the text is being changed. I tend to make these validations for things like updating a database or inserting into a database where the text boxes need to be filled, so I have quite a number of these spaghetti snippets. I haven't seen use of NonNullTextBox used yet, @CodyGray \$\endgroup\$ – Aroueterra Jan 26 '17 at 13:27
  • \$\begingroup\$ @PawełŁukasik Yes, I considered posting this on base StackOverflow but reading 2 or three similar answers tagged - beginner - , I figured it would be alright here as I present completed code? Let me know what you think! \$\endgroup\$ – Aroueterra Jan 26 '17 at 13:33
  • \$\begingroup\$ A while since I used WinForms. This is easier on WPF. I am surprised there is no a way to just enumerate the TextBoxes on a Form. \$\endgroup\$ – paparazzo Jan 26 '17 at 15:25
12
\$\begingroup\$

Why so complicated?

One possible approach is:

var f = new Form();
if (f.Controls.OfType<TextBox>().Any(x => string.IsNullOrWhiteSpace(x.Text)))
{
    /*your stuff*/
}

A second approach is:

var TextBox1 = new TextBox();
var TextBox2 = new TextBox();

var textboxes = new[] { TextBox1, TextBox2 };
if (textboxes.Any(x => string.IsNullOrWhiteSpace(x.Text)))
{
    /*your stuff*/
}
| improve this answer | |
\$\endgroup\$
  • 5
    \$\begingroup\$ OP checks for whitespace as well (well a single space) so use string.IsNullOrWhiteSpace instead. \$\endgroup\$ – 404 Jan 26 '17 at 11:06
  • 2
    \$\begingroup\$ @eurotrash fixed, thx. \$\endgroup\$ – tym32167 Jan 26 '17 at 11:57
  • \$\begingroup\$ The first one is similar to what I mentioned I had, the problem is that it considers that all of the textboxes in the form are necessary for the code, when I only want a select few. The second one though, is looking more like what I want, though I wonder if, since I have about 15 textbox controls, whether that would qualify as bad code as well from the mess of initializations? I don't think you can avoid that though. \$\endgroup\$ – Aroueterra Jan 26 '17 at 13:31
  • 1
    \$\begingroup\$ OP writes: Note that, I do have another solution, it's a linq block that iterates through ALL textbox controls in a given form. \$\endgroup\$ – t3chb0t Jan 26 '17 at 16:06
  • \$\begingroup\$ @t3chb0t thx, I noticed that \$\endgroup\$ – tym32167 Jan 26 '17 at 16:32
7
\$\begingroup\$

While the suggested approach with enumerating and checking all text-boxes works as an easy workaround, it's a horrible solution from the user point of view because he either gets noticed about some missing inputs when he hits submit or he will constantly be notified with annoying message-boxes when he leaves a text-box. He also won't know which field he left empty.

The right thing to do is to use the Validating event connected with the ErrorProvider that will give instant hints about what's wrong.


Here's a small example.

You create an error-provider by dropping it to the form/user-control from the toolbox.

ErrorProvider errorProvider = new ErrorProvider();

Then you create a method to validate the text and to set the error message.

bool ValidateIsNotNullOrWhitespace(TextBox textBox)
{
    if (string.IsNullOrWhiteSpace(textBox.Text))
    {
        // textBox.Tag - use this to get a custom message for this text-box
        var message = "Please provide your Name."
        errorProvider.SetError(textBox, message);
        return true;
    }
    errorProvider.SetError(textBox, string.Empty); // clears error message
    return false;
}

If you don't want to use a generic message like This field must not be empty. then you can use the Tag property to specify the key for the string:

textBox.Tag = "age"; // this can be an enum, string, int or anything

Finally you wire each text-box'es Validating event with a call to the validation method above. Setting Cancel to true will prevent the user from changing the focus to another control until he enters a valid value.

textBox.Validating += (sender, e) => 
{
    e.Cancel = ValidateIsNotNullOrWhitespace((TextBox)sender);
};

When a text-box contains an invalid value an exclamation mark will be shown and hovering over it will display the message.

Text-Box validation

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Another approach would be to keep the Submit button disabled until all the required fields are filled. That would mean indicating what fields are required, perhaps with an asterisk: "Zip Code*" and "Address 4". \$\endgroup\$ – rossum Jan 26 '17 at 15:12
  • \$\begingroup\$ @rossum, that's one way to do it, surely. I'll test t3chb0t's solution when I get back on my work machine. It looks pretty modular. \$\endgroup\$ – Aroueterra Jan 28 '17 at 2:35
  • \$\begingroup\$ Thats exactly the way to go +1 \$\endgroup\$ – Heslacher Jan 30 '17 at 8:45
2
\$\begingroup\$

I was going to add this as a comment to @tym32167's answer, but I am brand new and don't have enough rep to do that.

What you can do, building on @tym32167's answer, is filter the controls by another property. Each of the controls has a Tag property that you can set any string value to, such as "Required". From there, you can find all text boxes whose Tag property matches that value, like so:

var f = new Form();
if (f.Controls.OfType<TextBox>().Where(x => (string)x.Tag == "Required").Any(x => string.IsNullOrWhiteSpace(x.Text)))
{
    /*your stuff*/
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ You don't need to abuse the Tag property for this. Use the CausesValidation property, and handle the Validating event. \$\endgroup\$ – Cody Gray Jan 26 '17 at 14:20
  • \$\begingroup\$ @CodyGray Similar to t3chb0t's answer? That's probably the better way to go, especially with the UI feedback. I was just trying to expand upon tym32167's answer minimally because Aroueterra mentioned he originally tried something similar but wanted to filter it more. \$\endgroup\$ – Malivil Jan 26 '17 at 14:27

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.