Handling errors while inside a button

Question

I have a windows form that involes filling out textboxes with information and then clicking connect. I have error messages that pops up if any of the textboxes are empty but when I hit OK the program just continues and I end up getting run-time errors because there's insufficient information, and the program crashes. What I want is for the program to go back to the point before I hit "Connect" whenever any textbox is not filled out correctly.

This is the code:

private void cmdConnect_Click(object sender, EventArgs e)
    {

        if (cmdConnect.Text == "Connect")
        {
            if (txtGroup.Text == "")
            {
                txtGroup.Text = "_Group01";
            }

            if (txtItemID.Text == "")
            {
                MessageBox.Show("Please enter ItemID.", "Connect Error", MessageBoxButtons.OK, MessageBoxIcon.Error);

            }
                switch (cboServer.Text)
                {
                    case "":
                        MessageBox.Show("Please select and OPC server", "Connect Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
                        break;


                    case "RSLinx Remote OPC Server":
                        if (txtMachine.Text == "")
                        {
                            MessageBox.Show("Please enter a machine name for remote connection", "Connect Error", MessageBoxButtons.OK, MessageBoxIcon.Error);
                            break;
                        }
                        else
                        {
                            oOpcServer.Connect(cboServer.Text, txtMachine.Text);
                        }
                        break;

                    case "RSLinx OPC Server":
                        oOpcServer.Connect(cboServer.Text);
                        break;

                    default:
                        if (txtMachine.Text == "")
                        {
                            oOpcServer.Connect(cboServer.Text);
                        }
                        else
                        {
                            oOpcServer.Connect(cboServer.Text, txtMachine.Text);


                        }
                        break;
            }
            oOpcGroup = oOpcServer.OPCGroups.Add(txtGroup.Text);
            oOpcGroup.IsSubscribed = true;
            oOpcGroup.IsActive = false;
            oOpcGroup.UpdateRate = 1000;


            ClHandle = 1;
            oOpcGroup.OPCItems.DefaultAccessPath = txtAccessPath.Text;
            oOpcGroup.OPCItems.AddItem(txtItemID.Text, ClHandle);

            cmdItemWrite.Enabled = true;
            cmdItemRead.Enabled = true;
            cmdSyncWrite.Enabled = true;
            cmdSyncRead.Enabled = true;
            cmdAsyncWrite.Enabled = true;
            cmdAsyncRead.Enabled = true;
            cmdAdvise.Enabled = true;
            txtSubValue.Enabled = true;
            cboServer.Enabled = false;
            txtMachine.Enabled = false;
            txtGroup.Enabled = false;
            txtAccessPath.Enabled = false;
            txtItemID.Enabled = false;

            cmdConnect.Text = "Disconnect";
        }
        else
        {
            oOpcServer.OPCGroups.RemoveAll();
            oOpcGroup = null;
            oOpcServer.Disconnect();

            cmdConnect.Text = "Connect";
            cmdItemWrite.Enabled = false;
            cmdItemRead.Enabled = false;
            cmdSyncWrite.Enabled = false;
            cmdSyncRead.Enabled = false;
            cmdAsyncWrite.Enabled = false;
            cmdAsyncRead.Enabled = false;
            cmdAdvise.Enabled = false;
            txtSubValue.Enabled = false;
            cboServer.Enabled = true;
            txtMachine.Enabled = true;
            txtGroup.Enabled = true;
            txtAccessPath.Enabled = true;
            txtItemID.Enabled = true;
        }
        oOpcGroup.DataChange += new RsiOPCAuto.DIOPCGroupEvent_DataChangeEventHandler(oOpcGroup_DataChange);
    }

Answer 1

Simplest solution, as Dervall mentioned is adding return statements after each MessageBox.Show call. But more elegant solution is using validation and error provider to highlight incorrect input data prior to executing connect logic.

Anyway, here is some thoughts on refactoring your code.

private void cmdConnect_Click(object sender, EventArgs e)
{
    if (cmdConnect.Text == "Disconnect") 
    {
        Disconnect();
        SetControlsToDisconnectedState();
        return;
    }

    if (String.IsNullOrWhiteSpace(txtGroup.Text))
        txtGroup.Text = "_Group01";


    if (String.IsNullOrWhiteSpace(txtItemID.Text))
    {
        ShowErrorMessage("Connect Error", "Please enter ItemID.");
        return;
    }

    if (String.IsNullOrWhiteSpace(cboServer.Text))
    {
        ShowErrorMessage("Connect Error", "Please select and OPC server");
        return;
    }

    Connect(cboServer.Text, txtMachine.Text);
    DoSomethingWithGroup(txtGroup.Text, txtAccessPath.Text, txtItemID.Text);
    SetControlsToConnectedState();
}

What changed:

• It's more readable, when you verify which text on button, then which it don't have
• Method ShowErrorMessage does exactly what it says
• Verify text with IsNullOrWhiteSpace because it could be full of white spaces
• Control state changing moved to separate code
• Connecting/Disconnecting now separated from UI

Here other methods:

private void ShowErrorMessage(string title, string message)
{
    MessageBox.Show(message, title, MessageBoxButtons.OK, MessageBoxIcon.Error);
}

private void SetControlsToConnectedState()
{
    UpdateControls(true);
}

private void SetControlsToDisconnectedState()
{
    UpdateControls(false);
}

private void UpdateControls(bool isConnected)
{
    cmdConnect.Text = isConnected ? "Disconnect" : "Connect";
    cmdItemWrite.Enabled = isConnected;
    cmdItemRead.Enabled = isConnected;
    cmdSyncWrite.Enabled = isConnected;
    cmdSyncRead.Enabled = isConnected;
    cmdAsyncWrite.Enabled = isConnected;
    cmdAsyncRead.Enabled = isConnected;
    cmdAdvise.Enabled = isConnected;
    txtSubValue.Enabled = isConnected;
    cboServer.Enabled = !isConnected;
    txtMachine.Enabled = !isConnected;
    txtGroup.Enabled = !isConnected;
    txtAccessPath.Enabled = !isConnected;
    txtItemID.Enabled = !isConnected;      
}

private void Disconnect()
{
    oOpcServer.OPCGroups.RemoveAll();
    oOpcGroup = null;
    oOpcServer.Disconnect();            
}

private void Connect(string serverName, string machineName)
{
    switch (serverName)
    {
        case "RSLinx Remote OPC Server":
            if (String.IsNullOrWhiteSpace(machineName))
            {
                ShowErrorMessage("Connect Error", "Please enter a machine name for remote connection");
                return;
            }

            oOpcServer.Connect(serverName, machineName);                    
            break;

        case "RSLinx OPC Server":
            oOpcServer.Connect(serverName);
            break;

        default:
            if (String.IsNullOrWhiteSpace(machineName))            
                oOpcServer.Connect(serverName);            
            else            
                oOpcServer.Connect(serverName, machineName);            
            break;
    }           
}

private void DoSomethingWithGroup(string groupName, string accessPath, string itemID)
{
    oOpcGroup = oOpcServer.OPCGroups.Add(groupName);
    oOpcGroup.IsSubscribed = true;
    oOpcGroup.IsActive = false;
    oOpcGroup.UpdateRate = 1000;

    ClHandle = 1;
    oOpcGroup.OPCItems.DefaultAccessPath = accessPath;
    oOpcGroup.OPCItems.AddItem(itemID, ClHandle);

    oOpcGroup.DataChange += new RsiOPCAuto.DIOPCGroupEvent_DataChangeEventHandler(oOpcGroup_DataChange);
}

Answer 2

Adding a return statment after each message box would do the trick and cause the method to exit without doing the work at end.