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.
3 Answers 3
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*/
}
-
5\$\begingroup\$ OP checks for whitespace as well (well a single space) so use
string.IsNullOrWhiteSpace
instead. \$\endgroup\$404– 4042017年01月26日 11:06:19 +00:00Commented Jan 26, 2017 at 11:06 -
2\$\begingroup\$ @eurotrash fixed, thx. \$\endgroup\$tym32167– tym321672017年01月26日 11:57:28 +00:00Commented Jan 26, 2017 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\$Avan– Avan2017年01月26日 13:31:41 +00:00Commented Jan 26, 2017 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– t3chb0t2017年01月26日 16:06:07 +00:00Commented Jan 26, 2017 at 16:06
-
\$\begingroup\$ @t3chb0t thx, I noticed that \$\endgroup\$tym32167– tym321672017年01月26日 16:32:56 +00:00Commented Jan 26, 2017 at 16:32
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.
-
\$\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– rossum2017年01月26日 15:12:48 +00:00Commented Jan 26, 2017 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\$Avan– Avan2017年01月28日 02:35:56 +00:00Commented Jan 28, 2017 at 2:35
-
\$\begingroup\$ Thats exactly the way to go +1 \$\endgroup\$Heslacher– Heslacher2017年01月30日 08:45:19 +00:00Commented Jan 30, 2017 at 8:45
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*/
}
-
\$\begingroup\$ You don't need to abuse the
Tag
property for this. Use theCausesValidation
property, and handle theValidating
event. \$\endgroup\$Cody Gray– Cody Gray2017年01月26日 14:20:54 +00:00Commented Jan 26, 2017 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– Malivil2017年01月26日 14:27:25 +00:00Commented Jan 26, 2017 at 14:27
TextBox
control, add your code there, and use thatNonNullTextBox
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\$