2
\$\begingroup\$

I have a function which contains two switches, which check for Command Code and Response Code after that updating UI elements. The code is working fine.

How I can reduce or improve those switch cases?

private void port_DataReceived(object sender, SerialDataReceivedEventArgs e)
{
    try
    {
        byte[] buffer = new byte[serialPort.BytesToRead];

        serialPort.Read(buffer, 0, buffer.Length);


        if (buffer.Length > 1)
        {
            #region FRAME VALIDATOR
            FrameResponseValidator frameResponseValidator = new FrameResponseValidator();
            bool isFrameValid = frameResponseValidator.ValidateFrame(buffer);
            #endregion FRAME VALIDATOR


            if (isFrameValid)
            {
                int commandCode = buffer[1];
                int responesCode = 0;
                switch (commandCode)
                {
                    case Constants.HANDSHAKE_COMMAND_CODE:
                        responesCode = buffer[3];
                        _counter++;
                        break;
                    case Constants.ERASE_COMMAND_CODE:
                        responesCode = buffer[2];
                        _counter++;
                        break;
                    case Constants.NO_OF_PACKET_COMMAND_CODE:
                        responesCode = buffer[2];
                        _counter++;
                        break;
                    case Constants.WRITE_COMMAND_CODE:
                        responesCode = buffer[2];
                        _counter++;
                        break;
                    case Constants.DATA_COMMAND_CODE:
                        responesCode = buffer[2];
                        _counter++;
                        break;
                    case Constants.VERIFY_COMMAND_CODE:
                        responesCode = buffer[2];
                        _counter++;
                        break;
                    case Constants.CHECKSUM_COMMAND_CODE:
                        responesCode = buffer[2];
                        _counter++;
                        break;
                    case Constants.READ_VERSION_COMMAND_CODE:
                        this.DisplayCurrentVersion(buffer);
                        responesCode = buffer[1];
                        _counter++;
                        break;
                    case Constants.CONNECTION_ALIVE:
                        responesCode = buffer[1];
                        break;
                    default:
                        break;
                }

                switch (responesCode)
                {
                    case Constants.HANDSHAKE_AND_WRITE_ACK_SUCCESS:

                        if (_counter == 1)
                        {
                            timer.Stop();
                            connectionTimer.Start();
                            this.Dispatcher.Invoke(() =>
                            {

                                lblConnectionStatus.Content = "Connected";
                                lblConnectionStatus.Background = Brushes.Green;
                            });
                        }
                        else if (_counter == 4)
                        {
                            _noOfPackets = packetList.Count;
                            this.PacketToSendInfoCommand(_noOfPackets);
                        }

                        // For Temp Purpose {Will be refactored as per new protocol}
                        else if (_counter == _noOfPackets + 7)
                        {
                            VerifyCommandWithCheckSum(_totalCheckSum);
                        }
                        else if (_counter != 1 || _counter != 4)
                        {
                            // Check the isFileSelected if the file is selected
                            if (isFileSelected && _packetCounter != packetList.Count)
                            {
                                this.Dispatcher.Invoke(() =>
                                {
                                    double percentage = Math.Round(((double)_packetCounter / _noOfPackets) * 100);
                                    lblStatus.Content = "Writing data... " + _packetCounter + "/" + _noOfPackets + " & Perce: " + percentage + "%";
                                    pbProcess.Value = percentage;
                                });

                                //Thread.Sleep(10);
                                this.SendPacket(packetList[_packetCounter]);
                            }
                            else
                            {
                                isFileSelected = false;
                            }
                        }
                        break;
                    case Constants.ERASE_SUCCESS:
                        this.Dispatcher.Invoke(() =>
                        {
                            pbProcess.Value = 50;
                            lblStatus.Content = "Erase successfull!";
                        });
                        this.AllowToWriteCommand();
                        break;

                    case Constants.PACKET_WRITE_SUCCESS:
                        this.Dispatcher.Invoke(() =>
                        {
                            pbProcess.Value = 70;
                            lblStatus.Content = "Write Successfull!";
                        });
                        this.VerifyCommand();
                        this.Dispatcher.Invoke(() =>
                        {
                            pbProcess.Value = 90;
                            lblStatus.Content = "Verfiying...";
                        });
                        break;
                    case Constants.VERIFY_FAILED:
                        MessageBox.Show("Final checksum verification failed! Try again");
                        this.Dispatcher.Invoke(() =>
                        {
                            pbProcess.Value = 0;
                            lblStatus.Content = "Execution aborted! Try again";
                        });
                        break;
                    case Constants.VERIFY_SUCCESS:
                        this.Dispatcher.Invoke(() =>
                        {
                            pbProcess.Value = 100;
                            lblStatus.Content = "Verify Success!";
                            MessageBox.Show("Verify Successs!");
                        });

                        DisposePort();
                        break;
                    case Constants.PACKET_FAILED_RESPONSE:
                        MessageBox.Show("Failed to write a frame!");
                        this.SendPacket(packetList[_packetCounter]);
                        break;
                    case Constants.READ_VERSION_SUCCESS:
                        break;
                    case Constants.CONNECTION_ALIVE:
                        // this will be refactor later
                        connectionTimer.Interval = 70000;
                        break;
                    default:
                        MessageBox.Show("Unknown response!");
                        break;
                }
            }
            else
            {
                var sdsds = buffer;
                MessageBox.Show("Invalid Response");
            }
        }
    }
    catch (Exception ex)
    {
        ExceptionHandler exceptionHandler = new ExceptionHandler();
        exceptionHandler.HandleException(ex);
    }
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Fall through switch cases

  • Identical case blocks. You can have many clauses fall through to the same block. /* Example A * /

  • Share code in similar cases. You can have two blocks with the first falling through to the next via goto case , thus share some of the code. /* Example B * /

  • The default clause means the code is 0, I dont know if that is in Constants but it feels like it might fall through to the default in the second switch where you show an error. /* Example C */

eg Note the two possible exceptions

int responesCode = 0;
switch (buffer[1]) {
    case Constants.HANDSHAKE_COMMAND_CODE:
        responesCode = buffer[3];         // could be IndexOutOfRangeException
        _counter++;
        break;
    case Constants.ERASE_COMMAND_CODE:     /* Example A */
    case Constants.NO_OF_PACKET_COMMAND_CODE:
    case Constants.WRITE_COMMAND_CODE:
    case Constants.DATA_COMMAND_CODE:
    case Constants.VERIFY_COMMAND_CODE:
    case Constants.CHECKSUM_COMMAND_CODE:
        responesCode = buffer[2];       // could be IndexOutOfRangeException
        _counter++;
        break;
    case Constants.READ_VERSION_COMMAND_CODE: 
        this.DisplayCurrentVersion(buffer);
        _counter++;
        goto case Constants.CONNECTION_ALIVE;  /* Example B */
    case Constants.CONNECTION_ALIVE:  /* Example B */
        responesCode = buffer[1];       
        break;
    default:
        MessageBox.Show("Unknown response!"); /* Example C */
        return;
}

Code complexity.

Try to keep complexity down where possible.

Examples


  1. There is no need to cast to a double then call Math.Round (BTW rounding will show 100% before it is 100%) in the percentage calculation (Assuming pbProcess.value is an int) The value is floored in integer calculations so you only get 100% when it is 100%

You have something like

double percentage = Math.Round(((double)_packetCounter / _noOfPackets) * 100);
lblStatus.Content = "Writing data... " + percentage + "%";
pbProcess.value = percentage;

Can be

pbProcess.value = _packetCounter * 100 / _noOfPackets;  // Could be div by zero????
lblStatus.Content = "Writing data... " + pbProcess.value + "%";

  1. In the second switch in the first clause the last else has a redundant if statement. The conditions have already been meet.

eg.

} else if (_counter != 1 || _counter != 4) {

// should be just the else
} else {

Poor error handling

You have two points that look like errors where you invoke the messageBox for "Unknown response!", and "Invalid response!" and just continue execution.

You also have generic catch. It looks like a cover all solution and that you are unsure as to the range of possible errors. You are using the catch to cover poor quality code.

\$\endgroup\$
4
  • \$\begingroup\$ Thanks! It would be better if you suggest more about second switch case and more about poor error handling \$\endgroup\$ Mar 23, 2019 at 16:02
  • 1
    \$\begingroup\$ @PrashantPimpale The second switch case is ok. There is not much more I can add regarding the error handling as the source of likely errors are in the various methods you call, it would be bad advice for me to guess. All I can add is...Check each call for all the possible errors, Can you handle the errors in within the call. Can you preempt the errors and avoid them. Are the errors critical (must exit/reset state), Can you avoid needing the try catch altogether. If not consider creating custom errors that are more meaningful within the context of your code. Don't use errors to control flow. \$\endgroup\$
    – Blindman67
    Mar 23, 2019 at 16:23
  • 2
    \$\begingroup\$ @Blindman67: In C# your Example B for the switch statement is not allowed like it is in C/C++. \$\endgroup\$
    – user73941
    Mar 23, 2019 at 19:39
  • \$\begingroup\$ I was thinking to combine both switch cases into one hence the role of _counter will be vanished. \$\endgroup\$ Mar 24, 2019 at 1:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.