How to deal with a lot of conditions in If statement in an elegant manner

Question

Is there a more syntactically beautiful/simply better way to write the following (without major abstraction)?:

 if (usart_error.CRCError == true || usart_error.DMATransferError == true ||
     usart_error.FramingError == true || usart_error.NoiseError == true ||
     usart_error.OverrunError == true || usart_error.ParityError == true )
 {
    //...
 }

I am using the OOP only aspect of C++ for my embedded system if that makes any difference.

Note: usart_error is my class so I can adjust it if needed.

I have had it suggested to use a bit mask on an array and check if it has a 1 etc. but this abstracts it too far for my liking.

Answer 1

If you refuse to abstract or change usart_error then consider using whitespace to take mercy on my eyes.

if  (  usart_error.CRCError
    || usart_error.DMATransferError
    || usart_error.FramingError
    || usart_error.NoiseError
    || usart_error.OverrunError 
    || usart_error.ParityError
    )
{
    //...
}

This is reminiscent of Haskell style. It's best to avoid the need for this in the first place but this has the advantage of making the logical operation between each line noticeable at a glance, it's easy to confirm that usart_error is used throughout, and each error property is presented in vertical list form.

Now sure, it eats up lines of code but I find fluffy code more digestible then compacted code. Sparse over dense, as the Agile Manifesto says.

This form is compatible with those living under the tyranny of tabs. There are other alignments.

If you were willing to abstract it (please do) behind a good descriptive name (please please do) the complexity is still likely to show up somewhere else, sticking you with nearly the same problem. In those cases I use something like this:

return usart_error.CRCError
    || usart_error.DMATransferError
    || usart_error.FramingError
    || usart_error.NoiseError
    || usart_error.OverrunError 
    || usart_error.ParityError
;

It is worth taking a moment to ask yourself if a design decision isn't forcing you to write code this complex. If this can be broken down or avoided in someway it's worth your time to find a simpler way to handle this problem. At first glance I wonder if there isn't a type hierarchy hiding in this code.

But if you're going to do it this way, please make it easy on the eyes.

Answer 2

If usart_error is your own class, you should add methods with descriptive names to it so that you can write these conditions not just shorter, but more intellegibly.

For instance, .transient_error() and .nonrecoverable_error() might do the trick, or maybe .transfer_error() as opposed to content_error() - or maybe just is_error().

Answer 3

This is exactly what a function is for. Extract the condition into a function. I'm using a free function here, but you could equally put it into a member function on UsartError.

bool is_fatal_error(const UsartError & usart_error) {
    return usart_error.CRCError
        || usart_error.DMATransferError 
        || usart_error.FramingError
        || usart_error.NoiseError
        || usart_error.OverrunError
        || usart_error.ParityError
    ;
}

Then the main code looks like this:

if (is_fatal_error(usart_error))
{
    //...
}

Answer 4

There's no need to test all the individual error flags individually.

For the kind of test you are doing (i.e. was there any error), you could use C's bit fields:

#include <stdio.h>
#include <stdlib.h>

static union {
    unsigned int value;
    struct {
        unsigned int CRC:1;
        unsigned int DMATransfer:1;
        /* ... */
        unsigned int Overrun:1;
        unsigned int Parity:1;
    };
} usart_error;

    int
main(void) {
    usart_error.value = 0;
    /* ... */
    if (usart_error.Overrun)
        printf("Overrun\n");
    return usart_error.value ? 1 : 0;
}

That example will print "Overrun" if there was an overrun error, and exit(1) if there was an error of any kind.

Answer 5

I'm pretty lost as to how a bit-mask qualifies as "abstraction" (in fact, it's generally just the opposite--it's much closer to how most typical hardware represents things.

But, if insist on avoiding it anyway, I'd probably put the bools into an array or vector, and have the names of the various errors as subscripts into the array/vector. This way you can pretty easily write a loop to detect whether any of a given set of values is true:

enum { ERROR_FIRST, 
       CRCError = ERROR_FIRST, 
       DMATransferError, 
       FramingError, 
       NoiseError, 
       OverrunError, 
       ParityError, 
       ERROR_LAST};

bool usart_errors[ERROR_LAST];

bool error_occurred = false;

for (int i=ERROR_FIRST; i<ERROR_LAST; i++)
    error_occurred |= usart_errors[i];

if (error_occurred) ...