I folks, I have a question for the C/C Syntax / Coding-Style Gurus out there:

The situation: I am working on an STM32 Microcontroller and in some functions I have to do a lot of bit shifting, logical operations (and, or, xor) on bits, set/clear bits and writing / reading this code is really painfull.

An example: lets asume I had to turn an device on with a digital Output, and I have 5 sensors to check, where each of the sensors has its own dedicated digital Input. Some of them need to be TRUE, others need to be FALSE

A (Pseudo-)Code would eventually look like this: Note: this code is obviously nonsense/nothing really functional, it's just for explaining the principle.

#define SENSOR1 0
#define SENSOR2 1
#define SENSOR3 2
#define SENSOR4 3
#define SENSOR5 4

void DeviceOn(void) {
   uint8_t DIOPort = ReadDIOPort();
   if((DIOPort & (1 << SENSOR1)) && (!(DIOPort & (1 << SENSOR2))) && (DIOPort & (1 << SENSOR3))) {
      turnOnDevice();
   } else {
      turnOffDevice();
   }
}

As you see, the if-condition becomes pretty ugly and if there are more signals involved it becomes more and more unreadable.

My idea was to define macros inside the function, only used by / within that function, to make the code more readable. This would look like this.

void DeviceOn(void) {
       #define __POWER_ISOK (DIOPort & (1 << SENSOR1))
       #define __SAFETY_ISOK (!(DIOPort & (1 << SENSOR2)))
       #define __LIGHT_ISON (DIOPort & (1 << SENSOR3))
       #define __COFFEEMUG_ISFULL (DIOPort & (1 << SENSOR4))
       uint8_t DIOPort = ReadDIOPort();
       if(__POWER_ISOK && __SAFETY_ISOK && __LIGHT_ISON && __COFFEEMUG_ISFULL) {
         turnOnDevice();
       } else {
         turnOffDevice();
       }
    }

IMHO the if-condition is much more readable as the first example.

The question is: is this considered to be "good coding style" or is that an absoulte no-go and other developers will start to avoid me and consider me to be the worst coder in the whole universe?

I want the macros to be defined within the function, because they are ONLY used in this one function, and it makes documentation easier: the one who has to read this code just has to scroll up to the start of the function and does not have to search in the C-File or even the Header-File for the definition. The Definitions of the bits in the Digital Input Status Variable are defined gobally (at the beginning of the C-File) as they are used in other functions, too.

Of course I'd define any macros gobally, that are used in multiple functions. Of course I could also define all macros gobally, but I think that it makes the code more readable, if those who are used only in one function, are defined in that function.

Technically I don't see any issues, as the preprocessor should replace the macros in the build process, or am I wrong?

CodePudding user response：

Marcos don't have scopes, and their definition won't end at the end of the function.

Defining macros inside of functions such that they remain defined after the function is a bad practice because programmers expect definitions to end when the scope ends. You could undefine the macros at the end of the function. That is a good practice when the macros is needed in the first place which isn't often.

Unnecessary use of macros is a bad practice. In this case, it is a better practice to use constant variables instead. Variables have a type system, and scope, both of which make it easier to write a correct program.

P.S. Names that contain two consecutive underscores are reserved to the language implementation. Don't define them yourself.

CodePudding user response：

Alternate solution:

#include <stdbool.h>

void DeviceOn(void)
{
    uint8_t DIOPort = ReadDIOPort();

    bool PowerIsOK       =    DIOPort & 1 << SENSOR1;
    bool SafetyIsOK      = ! (DIOPort & 1 << SENSOR2);
    bool LightIsOn       =    DIOPort & 1 << SENSOR3;
    bool CoffeeMugIsFull =    DIOPort & 1 << SENSOR4;

    if (PowerISOK && SafetyIsOK && LightIsOn && CoffeeMugIsFull)
        turnOnDevice();
    else
        turnOffDevice();
}