Database Programming Data Validation

Question

I tried using the symbol || which represents or. However, when I tried it in my program it still shows me the error message where I would actually put it in a message box and display it to the user to let them know that they type it wrong, however, even if I type it right it still shows me that message. Below is my code. Hope there will be someone that cold help me.

private void SaveChanges()

{

//declare variable for store code integer            
int storeCode = 0;


//check if first and last name are empty

if (txtName.Text == "")

        {

            MessageBox.Show("Fill in First Name", "Missing Data");
            txtName.Focus();
        }
        else if(txtLastname.Text == "")
        {
            MessageBox.Show("Fill in Last Name", "Missing Data");
            txtLastname.Focus();
        }
        else if(txtJobCode.Text == "" || txtJobCode.Text != "SEC" || txtJobCode.Text != "MGR" && 
            txtJobCode.Text != "GEN")
        {
            MessageBox.Show("Fill in Job Code with SEC, MGR or GEN", "Missing Data");
            txtJobCode.Focus();
        }
        else if(int.TryParse(txtStoreCode.Text, out storeCode) || txtStoreCode.Text == "")
        {
            if (storeCode <= 1 && storeCode >= 5)
            {
                MessageBox.Show("Fill in Store Code 1-5", "Missing Data");
                txtStoreCode.Focus();
            }

            MessageBox.Show("Fill in Store Code", "Missing Data");
            txtStoreCode.Focus();
        }
        else
        {
            this.eMPLOYEEBindingSource.EndEdit();
            this.tableAdapterManager.UpdateAll(this.empDataSet);
        }
    }//end SaveChanges

Answer 1

I'm assuming you're getting the error message at the line

else if(txtJobCode.Text == "" || txtJobCode.Text != "SEC" || txtJobCode.Text != "MGR" && 
            txtJobCode.Text != "GEN")
        {
            MessageBox.Show("Fill in Job Code with SEC, MGR or GEN", "Missing Data");
            txtJobCode.Focus();
        }

The reason is this will never evaluate to 'false' and skip the statement, plus you have an && for GEN. As soon as you successfully meet any of the first || statements you're going to fail the &&. It can't be one of those and GEN. Let's say you put GEN in.

== "" (false) != "SEC" (true) boom! (short circuited to error message)

Let's say you put SEC

== "" (false) != "SEC" (false) != "MGR" (true) boom! (short circuited to error message)

With or operators || the code will continue to check UNTIL it finds a true value. When it does it returns true from that point and doesn't look further. Therefore it reads if(true).. so it performs the if statement.

With and operators && the will continue to check UNTIL it finds a false. When it does it returns false and doesn't look any further. Therefore it read if(false)... and DOES NOT perform the if statement.

Now of course if no || statements are true it will return false and if no && values are false it will return true.

You need to write this I believe:

IF jobCode == "" OR (jobCode != SEC AND jobCode != MGR AND jobCode != GEN) then ShowMessage()

Now type SEC

== "" (false) || ( != SEC (false) ) - result false... Good! No message.

Now type GEN

== "" (false) || ( != SEC (true) && != MGR (true) && != GEN (false) ) - result false... Good! No message.

Now type ""

== "" (true) ... Good! We get a message.

Now try HELLO

== "" (false) || ( != SEC (true) && != MGR (true) && != GEN (true) )... Good! We get a message.

I would also like to add that nesting if statements and multiple and / or operators, although sometimes it can't be completely avoided, isn't the cleanest way because even looking at it myself it took me a moment to sort through it. I would re-write that logic to make better use if possible, even put the different tests in their own method if needed and test the method itself for success. Anything to make it easier to read and debug.

Looking to simplify it, with that line of code, since you have the three constants, just check to see if it DOES equal any of them.

You also have the range check backwards which will never be true. It can't be <= 1 and >= 5.

Here's the cleanup that should work; the way I would do it. (not tested, just scratched up on the spot.) Even if you prefer not to use it maybe you can read it easier and see the mistakes. Being easier to read makes it easier to debug and manage so hopefully I'm helping in more than one way with this.

    public void SaveChanges()
    {
        var isValid = validateFirstName()
                   && validateLastName()
                   && validateJobCode()
                   && validateStoreCode();

        if (isValid)
        {
            this.eMPLOYEEBindingSource.EndEdit();
            this.tableAdapterManager.UpdateAll(this.empDataSet);
        }
    }

    private bool validateFirstName()
    {
        var isValidFirstName = !string.IsNullOrEmpty(txtName.Text);

        if (!isValidFirstName)
        {
            MessageBox.Show("Fill in First Name", "Missing Data");
            txtName.Focus();
        }

        return isValidFirstName;
    }

    private bool validateLastName()
    {
        var isValidLastName = !string.IsNullOrEmpty(txtLastname.Text);

        if (!isValidLastName)
        {
            MessageBox.Show("Fill in Last Name", "Missing Data");
            txtLastname.Focus();
        }

        return isValidLastName;
    }

    private bool validateJobCode()
    {
        var isValidJobeCode = txtJobCode.Text == "SEC"
                           || txtJobCode.Text == "MGR"
                           || txtJobCode.Text == "GEN";

        if (!isValidJobeCode)
        {
            MessageBox.Show("Fill in Job Code with SEC, MGR or GEN", "Missing Data");
            txtJobCode.Focus();
        }

        return isValidJobeCode;
    }

    private bool validateStoreCode()
    {
        var storeCode = 0;
        var isValidStoreCode = int.TryParse(txtStoreCode.Text, out storeCode)
                               && storeCode >= 1 
                               && storeCode <= 5;

        if (!isValidStoreCode)
        {
            MessageBox.Show("Fill in Store Code 1-5", "Missing Data");
            txtStoreCode.Focus();
        }

        return isValidStoreCode;
    }
}