4

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.

  • 14
    A good start would be to remove all those == true. You don't need them, assuming they are boolean values. if (usart_error.CRCError || usart_error.DMATransferError || ... – Robert Harvey Feb 10 at 15:56
  • 5
    For this kind of code, using bit masks is probably the most elegant way, as you can check the presence of any error bit quickly. If you just don't want to use that, you could at least get rid of the superfluous == true tests. If your error bits are boolean it's highly unlikely that anything but true or false is found there. – Hans-Martin Mosner Feb 10 at 15:59
  • 1
    I actually use == true if I'm testing variables and just do 'if (errorFound())' if it's a method, so it just looks nicer and is less abstracted, thanks for all the help. – NoOne Feb 10 at 16:33
  • 6
    @hunterzzpro Seriously, get rid of == true. Two major reasons: 1) The == true serves no purpose and is simple noise. 2) You will eventually type = true (i.e. assignment) by accident - debugging the issue (and finding the sole = true in a sea of == true) is not a fun exercise – CharonX Feb 11 at 11:33
  • 2
    The == true issue is totally minor. I'd remove it here, too, but it's not important enough to worry about. One thing, though: I don't think that accidental assignment has been an issue for the last 20 years or so. Practically all C and C++ compilers in use today issue a diagnostic message for that. That's also why the true == x trick isn't really used anymore today; it just looks strange and provides no advantages. – Christian Hackl Feb 12 at 5:56
10

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.

| improve this answer | |
  • The one and only style for looong or’s. Know if Apple could fix Xcode for Swift to keep this layout intact... – gnasher729 Feb 10 at 17:22
  • Nice strategic use of our severely depleted whitespace reserve. – Deduplicator Feb 11 at 14:31
  • 1
    Putting || at the start of the line, rather than the end, is pure evil in my view :D – David Arno Feb 11 at 16:29
  • 1
    @DavidArno I love it, because I can on first sight distinguish between the two most common cases (long list of || or long list of &&). – gnasher729 Feb 11 at 22:23
6

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().

| improve this 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))
{
    //...
}

| improve this answer | |
0

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.

| improve this answer | |
  • 1
    As an aside, normalizing a boolean value is normally done with !!, not the conditional operator. Also, the return-type of main() is int. Still, kudos for a different approach. – Deduplicator Feb 11 at 16:24
  • I worked mostly with C89, and it was common to use int main if it returned a value and void main if it didn't. I now see that this is no longer considered valid, so that will be the last time I do that. Thanks. (And I've now corrected the above code.) – Ray Butterworth Feb 11 at 17:36
  • @Deduplicator, and now that I think about it, I'm pretty sure that lint would complain if an int main called exit and "fails to return value", though maybe that predates C89, which in theory eliminated the need for lint. – Ray Butterworth Feb 11 at 17:55
  • Have fun with byte ordering, implementation defined behaviour, and undefined behaviour. – gnasher729 Feb 11 at 22:25
  • 1
    @gnasher729, how would any of those affect this example? – Ray Butterworth Feb 11 at 23:39
0

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) ...
| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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