Rewriting a specific if-statement-CodePudding

I am currently involved in a C language based embedded project where we have the following logic for component control:

input1	input2	output
OFF	OFF	NO_MOVEMENT
OFF	ON	DIRECTION_B
ON	OFF	DIRECTION_A
ON	ON	ERROR

Currently, in the existing code it is implemented like this:

//input1 and input2 are unsigned int and output is enum

if((input1 == 0u) && (input2 == 0u)) {
    output = NO_MOVEMENT;
}
else if((input1 == 1u) && (input2 == 0u)) {
    output = DIRECTION_A;
}
else if((input1 == 0u) && (input2 == 1u)) {
    output = DIRECTION_B;
}
else {
    output = ERROR;
}

I am wondering how to rework this piece of code to make it more optimal and elegant. My proposal is:

if(input1 == 0u)
{
    output = (input2 == 0u) ? NO_MOVEMENT : DIRECTION_B; 
}
else
{
    output = (input2 == 0u) ? DIRECTION_A : ERROR; 
}

Is it fine or maybe there is some better option?

CodePudding user response：

The current code looks quite clear and readable to me when all you have is two variables with two distinct values - I wouldn't change it for the alternative you suggested, it looks perfectly ok. If anything, maybe use a lookup table.

static const int input_output_table[2][2] = {
      [OFF][OFF] = NO_MOVEMENT,
      [OFF][ ON] = DIRECTION_B,
      [ ON][OFF] = DIRECTION_A,
      [ ON][ON] =  ERROR
   };

output = input_output_table[input1][input2];

(do any sanity checks if you need to ensure input1/input2 can be something other than 0 or 1. Alternatively coerce them to booleans as output = input_output_table[!!input1][!!input2];

CodePudding user response：

In my opinion, the original approach is fine enough. For easy reading, there is also possibility to use switch ... case with a small adjustment.

AltInput = (input1 << 1) | input2;

Then we can define an enum like following:

enum:
NO_MOVEMENT = 0,
DIRECTION_B,
DIRECTION_A,
ERROR

Then later you could use:

switch (AltInput)
{
case NO_MOVEMENT:
...
break;
case DIRECTION_B:
...
break;
case DIRECTION_A:
...
break;
default:
...
break;
}

CodePudding user response：

If the reason for rewriting this is performance, then note that the output values is a binary sequence and you can just merge input1 and input2 to get a number. Consider this solution:

typedef enum
{
  NO_MOVEMENT,
  DIRECTION_B,
  DIRECTION_A,
  ERROR,
  MASK = 0x3 // just defensive programming, you might not need it
} output_t;

output_t get_output (unsigned int input1, unsigned int input2)
{
  return (input1<<1 | input2) & MASK;
}

Benchmarked on gcc/x86, it's 4 instructions branch free (and a very likely candidate for inlining):

get_output:
        add     edi, edi
        or      edi, esi
        mov     eax, edi
        and     eax, 3
        ret

Without defensive programming:

typedef enum
{
  NO_MOVEMENT,
  DIRECTION_B,
  DIRECTION_A,
} output_t;

output_t get_output (unsigned int input1, unsigned int input2)
{
  return input1<<1 | input2;
}

Disassembly:

get_output:
        lea     eax, [rdi rdi]
        or      eax, esi
        ret

You won't beat this performance even with a LUT.