Why defining enums in the function bloc could be a bad practice?

user_10734452 0
Viewed 253

I've coded a stateflow handler and to reduce the risk of using the state flow enums outside the stateflow handler, I've defined the stateflow enums inside the function bloc.

My code looks like:

static void RequestHandler(bool isThisRequestANewRequest)
{
    typedef enum
    {
        STATE_NEW_REQUEST,
        STATE_1,
        STATE_2,
        STATE_ERROR,
    } States;

    static States  state = STATE_ERROR;

    if(isThisRequestANewRequest == true)
    {
        state = STATE_NEW_REQUEST;
    }

    switch(state)
    {    
        case STATE_NEW_REQUEST:
            //init request flags
            state = STATE_1;
            //lint -fallthrough
        case STATE_1:
            //do something
            break;
        case STATE_2:
            //do something else
            break;
        case STATE_ERROR:
        default:
            //do something in case of error
            break;
     }
}

Is this can be considered as a good practice? Is there any risk? Is there any cons? (maintenance, reading, ...)

One collegue of mine told me that this was not, but I'm waiting for fact based answers, not just raw opinions.

Note: My question applies to both monothreaded and multithreaded tasks.

embedded enums c
1 Answers

Generally, reducing scope as much as possible is good practice. If you want some sort of canonical reference to that, the closest I can come up with is MISRA-C:2012, rule 8.9 which recommends that objects that are only used by a single function should be declared at block scope. I don't see why the same wouldn't apply to types.

It is however bad practice to rely on fall-through in switch statements, since that blocks static analysers (like Lint in this case) from finding real bugs caused by missing break. It also makes the code harder to read and maintain - I would personally consider fall-through switches much worse practice than code repetition.

If you are to execute multiple states per function call, consider using simple if statements:

if(state == STATE_NEW_REQUEST)
{
  ...
  state = STATE_1;
}
if(state == STATE_1)
{
  ...
}

Otherwise, if you are only executing one state per function call, you can use a switch. In general, the need to "execute several states per state" is a hint that the broader program design could be improved.

user_5845180
Related