4
\$\begingroup\$

I'm working on a small winforms app, the goal of which is to capture data and write to xml. I'm still a child where programming is concerned so could you guys please take a look and suggest changes as I'm pretty sure that this is not a good approach. I'm using a masked-textbox for the invoice number which should only contain numbers. I'm trying not to use a numupdown for obvious reasons.

InputVars is a public class with empty string properties (string value {get;set;})

    private bool TestVal(string testedVal)
    {
        if (!String.IsNullOrEmpty(testedVal))
        {
            return true;
        }
        else
        {
            return false;
        }
    }



    private InputVars SetSemVars()
    {
        InputVars inVars = new InputVars();
        if (TestVal(txtCapt.Text))
        {
            inVars.captured = txtCapt.Text;
        }
        else
        {
            MessageBox.Show("Please insert your name here");
            txtCapt.Focus();
        }
        if (TestVal(mtxtInvoiceNumber.Text))
        {
            inVars.invoice = mtxtInvoiceNumber.Text;
        }
        else
        {
            MessageBox.Show("Please insert the invoice number here");
            mtxtInvoiceNumber.Focus();
        }
        if(TestVal(cmbNetwork.SelectedIndex.ToString()))
        {
            inVars.network = cmbNetwork.SelectedIndex.ToString();
        }
        else
        {
            MessageBox.Show("Please select the network ");
            cmbNetwork.Focus();
        }
        if(TestVal(cmbRegion.SelectedIndex.ToString()))
        {
            inVars.region = cmbRegion.SelectedIndex.ToString();
        }
        else
        {
            MessageBox.Show("Please select your office code here");
            cmbRegion.Focus();
        }
        if(TestVal(cmbSupplier.SelectedIndex.ToString()))
        {
            inVars.supplier = cmbRegion.SelectedIndex.ToString();
        }
        else
        {
            MessageBox.Show("Please select the supplier here");
            cmbSupplier.Focus();
        }
        return inVars;
    }

I was playing around earlier and got this to work , just thought I'd share

        foreach (TextBox box in this.panel1.Controls.OfType<TextBox>())
        {
            if (String.IsNullOrWhiteSpace(box.Text))
            {
                box.Text = "Enter value here";
                //or do a messagebox etc.
                box.Focus();
                return;
            }

        }

that`s loads and loads shorter and easier to read and makes sure that I don't forget a value... me gusta

\$\endgroup\$
2
\$\begingroup\$

Your testval method is completely unnecessary, just do your validation right in the method. I would create an eventhandler for each type of control's onchanged event, then you'd have one for combo boxes and one for text boxes. Have each control's tag property hold the invalid message for it to be displayed. Just set all text and combo boxes you want to validate for non-null/whitespace to have the same event handler to avoid code duplication.

Something like so:

private void OnTextChanged(object sender, EventArgs e)
{
  TextBox textBoxToValidate = sender as TextBox;
  if (string.IsNullOrWhiteSpace(textBoxTovalidate.Text))
  {
    MessageBox.Show(textBoxToValidate.Tag.ToString());
    textBoxToValidate.Focus();
  }
}

private void OnSelectedIndexChanged(object sender, EventArgs e)
{
  ComboBox comboBoxToValidate = sender as ComboBox;
  if (string.IsNullOrWhiteSpace(comboBoxTovalidate.SelectedIndex.ToString()))
  {
    MessageBox.Show(comboBoxToValidate.Tag.ToString());
    comboBoxToValidate.Focus();
  }
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I'll give it a shot but I'm pretty noobish \$\endgroup\$ – Dani Aug 15 '11 at 13:18
  • \$\begingroup\$ @Dani Just use the properties window in visual studio, select the control and scroll through properties to find the Tag property which you will put your error text in, and the SelectedIndexChanged event which you will put "OnSelectedIndexChanged" or whatever you name your event handler into and same for the text box, you just put the name of the method which has this signature public void MethodName(object sender, EventArgs e) into the event in the Properties window. \$\endgroup\$ – Jimmy Hoffa Aug 15 '11 at 14:32
  • \$\begingroup\$ I'll definitely try it out \$\endgroup\$ – Dani Aug 16 '11 at 10:15
3
\$\begingroup\$
  1. Don't use String.IsNullOrEmpty to validate required values.
  2. You can create a method

    private bool IsEmpty(string value, string errorMessage, Control controlToValidate)
    {
        if ((value ?? string.Empty).Trim().Length == 0)
        {
            MessageBox.Show(errorMessage);
            controlToValidate.Focus();
            return true;
        }
        else
        {
            return false;
        }
    }
    

    instead of TestVal.

  3. Consider using Validating event for validating your controls and the Tag property to associate error messages with the controls.
  4. Consider using ErrorProviders.
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ any specific reason for not using IsNullOrEmpty? I kinda figured that that's what it's for \$\endgroup\$ – Dani Aug 15 '11 at 9:40
  • \$\begingroup\$ IsNullOrEmpty will return false for a string containing only blank spaces. In general, a blank space is not a valid value for a reqired field. \$\endgroup\$ – Ivan Aug 15 '11 at 9:52
  • 1
    \$\begingroup\$ how about using isNullOrWhiteSpace though? That gets around the blank space issue doesn't it. \$\endgroup\$ – dreza Aug 15 '11 at 10:27
  • 2
    \$\begingroup\$ @dreza: Yes, if it is a .Net 4 project then IsNullOrWhiteSpace is definitely the right solution. \$\endgroup\$ – Ivan Aug 15 '11 at 10:39
  • \$\begingroup\$ I kinda dislike the fact that a method named IsEmpty has to wait for a message box to be dismissed before returning the result. \$\endgroup\$ – Sorin Comanescu Aug 19 '11 at 10:38

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.