Alternatives to if-else on data reading

Question

I have some code on Arduino (so, written in C++) that receives a String through the UART terminal, reads the String, then decides what String to print back and how many things to print depending on the data.

I've used an if-else stack to decide on what to print back. Something like this:

    if (cmdtype == "VALCN" && canSend == true) { //canSend is a flag from above. 
      cmdparam = "00000000";
      rtnProtocols(cmdtype, cmdparam);
    }
    else if (cmdtype == "FIRMV" && canSend == true) {
      cmdparam = "00000000";  //See desktop code for reason why cmdparam is what it is.
      rtnProtocols(cmdtype, cmdparam);
    }
    else if (cmdtype == "RQSPC" && canSend == true) { //RQSPC is a command
      //Need to get serial info, date, end of device info
      cmdtype = "GVSPC";    //Send device info signal
      cmdparam = "00101201";  //cmdparam is serial number
      rtnProtocols(cmdtype, cmdparam);
      
      cmdparam = "30082022";  //cmdparam is date
      rtnProtocols(cmdtype, cmdparam);

      cmdtype = "ENSPC";    //Send device info signal
      cmdparam = "00000000";  //cmdparam is date
      rtnProtocols(cmdtype, cmdparam);
    }

There are a lot of variations to the receivable String, so I'm foreseeing this if-else stack spiralling out of control. I am wondering whether there is a way of doing this without using an if-else stack. Or even if there is a way of doing this without conditionals, since all I'm doing with the branches is having it run the same function with different inputs, different times.

I've considered a switch-case, but wasn't able to do so since switch-case can't take Strings. Please let me know if this question is not suitable here, or if more information is needed.

Answer 1

To prevent over-engineering, first boil down on what is done:

if (canSend) {
    if (cmdtype == "VALCN") {
      rtnProtocols(cmdtype, "00000000");
    }
    else if (cmdtype == "FIRMV") {
      rtnProtocols(cmdtype, "00000000");
    }
    else if (cmdtype == "RQSPC") {
      rtnProtocols("GVSPC", "00101201");
      rtnProtocols("GVSPC", "30082022");
      rtnProtocols("ENSPC", "00000000");
    }
}

This already looks very regular, but in reality irregularities happen.

The code above with named constants (and commented = explained again) would already be fine.

Finding common business rules, logic, would be nice:

if (canSend) {
    if (cmdtype == "RQSPC") { // First prepare
      rtnProtocols("GVSPC", "00101201");
      rtnProtocols("GVSPC", "30082022");
      cmdtype = "ENSPC";
    }
    rtnProtocols(cmdtype, "00000000"); // Do 000
}

Only if the code really diverges or is actually an other system component, use specialising class inheritance. Irregularities should not impact other cases, so then inheritance is best. The class hierarchy can also be just an Encoder object passed. That is the class itself need not be using inheritance. The code here is more or less doing some strategy. Details belong typical to some processing class one works with (Single Responsibility).

Above cmdtype could be an enum, enabling a switch. But it might even be superfluous when you have a child class for RSQPC, etcetera.

There is yet an other approach: declarative: the criteria as data. For instance in XML. You then have one piece of code interpreting the data.

<rtn cmdtype="VALCN">
    <protocols param="0000000"/>
</rtn>
...

This allows logging for instance, and finding uncovered cases. Also this XML is sufficiently readable for the business side. And for mass of report variants, you can store in the report case info, like a "form number" or other meta information.

Answer 2

There are some things you can do, but ultimately you may need to live with some compromise due to Arduino resource limitations.

Cleanup approaches

One thing is to separate the input parsing from handling of the different cases. If there are a limited number of possible commands, you can represent them as enums (or simply #defined values) and convert from string to the appropriate enum using a table. That allows you to use a switch statement, which might be enough for your case.

You could also expand this table to include a function pointer and possibly arguments, so the reaction to each command would be a separate function that just handles that command. This is a pretty natural approach to interpreting commands in languages where you have function pointers.

Style suggestions

One thing that you should certainly do is to distinguish between inputs and outputs. You seem to be using cmdtype as an input to your sequential if statement but you modify it within the cases, which is bad style.

In your short excerpt, canSend is checked for each case - that's an opportunity to move that check to an outer if and avoid it in the inner cases.

Answer 3

The obvious answer here is a dispatch table, i.e. something like std::map<string, function_type> (assuming you have the STL available, if you don't you can probably fake this up usefully enough with an array).

My C++ is horribly, horribly rusty but this seems to compile and work...

#include <iostream>
#include <map>
#include <string>

typedef void (*action_fn)(void);

void valcn(void)
{   
    std::cout << "Processing VALCN\n";
}

void firmv(void)
{   
    std::cout << "Processing FIRMV\n";
}

int main(int argc, char **argv)
{   
    std::map<std::string, action_fn> dispatch;

    dispatch["VALCN"] = valcn;
    dispatch["FIRMV"] = firmv;
    // ...

    dispatch["FIRMV"]();

    return 0;
}

Answer 4

Command pattern

The command pattern might help you.

You could define one function for each cmdtype, and call the corresponding function when you receive some input via terminal.

This approach was also suggested by @PhilipKendall. The problem is that Arduino-C++ isn't really a complete C++, and std::map<std::string, action_fn> isn't available.

Arduino Strings should be avoided, too, because they use too much heap.

Interactive shell

I wrote a small Arduino library for a similar use-case, and asked a related question on Codereview.

The corresponding functions have either no argument, one string argument or one integer argument (e.g. for booleans or serial numbers).

Here's a modified version, for your use-case:

#include "command_invoker.h" // https://github.com/EricDuminil/arduino_interactive_shell/tree/master/arduino_interactive_shell

/*
 * Define your logic in those functions:
 */

bool sendDisabled = true;

void allowSend(int32_t allowed) {
  Serial.print("Sending data is now ");
  Serial.println(allowed ? "allowed" : "disallowed");
  sendDisabled = !allowed;
}

bool cannotSend() {
  if (sendDisabled) {
    Serial.println("Not sending anything. Sorry. Call SEND 1 first.");
  }
  return sendDisabled;
}

void valcn() {
  if (cannotSend()) return;
  Serial.println("Let's call rtnProtocols with VALCN!");
}

void firmv() {
  if (cannotSend()) return;
  Serial.println("Let's call rtnProtocols with FIRMV!");
}

void rqspc(int32_t serial_number) {
  if (cannotSend()) return;
  Serial.print("Let's call RQSPC with ");
  Serial.print(serial_number);
  Serial.println("!");
}

/**
 * Setup
 */

void setup() {
  Serial.begin(115200);

  // Assign commands here:

  command_invoker::defineIntCommand("SEND", allowSend, F(" 0/1 (Allow SEND or not)"));
  command_invoker::defineIntCommand("RQSPC", rqspc, F(" 123456 (doc here)"));
  command_invoker::defineCommand("VALCN", valcn, F(" (doc here)"));
  command_invoker::defineCommand("FIRMV", firmv, F(" (doc here)"));

  Serial.println(F("Console is ready!"));
  Serial.print(F("> "));
}

/*
 * Saves bytes from Serial.read() until enter is pressed, and tries to run the corresponding command.
 *   http://www.gammon.com.au/serial
 */
void processSerialInput(const byte input_byte) {
  static char input_line[MAX_COMMAND_SIZE];
  static unsigned int input_pos = 0;
  switch (input_byte) {
  case '\n': // end of text
    Serial.println();
    input_line[input_pos] = 0;
    command_invoker::execute(input_line);
    input_pos = 0;
    Serial.print(F("> "));
    break;
  case '\r': // discard carriage return
    break;
  case '\b': // backspace
    if (input_pos > 0) {
      input_pos--;
      Serial.print(F("\b \b"));
    }
    break;
  default:
    // keep adding if not full ... allow for terminating null byte
    if (input_pos < (MAX_COMMAND_SIZE - 1)) {
      input_line[input_pos++] = input_byte;
      Serial.print((char) input_byte);
    }
    break;
  }
}

/**
 * Loop and wait for serial input.
 */

void loop() {
  while (Serial.available() > 0) {
    processSerialInput(Serial.read());
  }
  delay(50);
}

The code is a bit long, but most of it is generic (setup/loop/processSerialInput), and it wouldn't get more complex with more functions. You'd simply add functions and callbacks inside setup.

Example

The above code seems to work fine on my ESP8266. As a bonus, you can get a list of every defined command by typing an unknown one (e.g. 'help'), and you can use hex numbers as parameter (e.g. 'RQSPC 0xFF00FF'):

Console is ready!
> help
No argument
'help' not supported. Available commands :
  FIRMV (doc here).
  RQSPC 123456 (doc here).
  SEND 0/1 (Allow SEND or not).
  VALCN (doc here).
> FIRMV
No argument
Calling : FIRMV()
Not sending anything. Sorry. Call SEND 1 first.
> SEND 1
Calling : SEND(1)
Sending data is now allowed
> RQSPC 12345
Calling : RQSPC(12345)
Let's call RQSPC with 12345!
> FIRMV
No argument
Calling : FIRMV()
Let's call rtnProtocols with FIRMV!
> VALCN
No argument
Calling : VALCN()
Let's call rtnProtocols with VALCN!
> SEND 0
Calling : SEND(0)
Sending data is now disallowed
> VALCN
No argument
Calling : VALCN()
Not sending anything. Sorry. Call SEND 1 first.

Answer 5

At least to me, this looks like a fairly simple case for a simple map to handle. I'd start by defining a type to hold the command type and command param for a call to rtnProtocols:

struct params {
    std::string cmdtype;
    std::string cmdparam;
};

I'd probably also write a wrapper for rtnProtocols to accept one of these directly:

void rtnProtocols(params const &p) {
    rtnProtocols(p.cmdtype, p.cmdparam);
}

Then I'd create a map (or unordered_map) with the input string as the key, and a vector of params as the associated value:

std::map<std::string, std::vector<params> responses {
    { "VALCN", {{ "VALCN", "00000000"}}},
    { "FIRMV", {{ "FIRMV", "00000000"}}},
    { "RQSPC", {{"GVSPC", "00101201"},
                {"GVSPC", "30082022" },
                {"ENSPC", "00000000" } 
               }
    }
};

Then when we get some input, we look it up in the map, find the vector of associated values, and process them accordingly:

if (canSend) { 
    auto *resp = responses.find(cmdtype);
    if (resp == responses.end()) {
       error("unrecognized command: ", cmdtype);
    }
    auto const &response_params = resp->second;
    for (auto const &p : response_params) {
        rtnProtocols(p);
    }
}

At least assuming the other results follow roughly the same pattern as the ones you've shown, dealing with more inputs should only involve adding entries for each additional input, and the parameters you want to pass to rtnProtocols when you receive each.