Does this code "stink"

I've been reading up on "code smells" and found one that seemed to hit home. So i wondered what you guys think and if there's a good way to rewrite this or if it will be OK as it is. I think as this project is finished that it won't get fixed here, but maybe I do things differently next time.

The smell was that you shouldn't use a switch/case to sequence procedural code. So that's exactly what I did. The intention was that functions like this one could be called from the main loop via a function pointer. Most of these functions require user input and I didn't want them to block the other things being called from loop. The idea is that the function will run and will either return false if it isn't done and wants to be called again or should return true to indicate that the job is done and the function pointer can be pointed at something else. I thought originally about writing many individual functions for each step, but I liked it better having the whole process in one place where it was obvious what was going on.

Most of these are being chosen from a menu, so the code gets called when the user selects it and continues to be called over and over each loop until the user has finished, in this case entering the time and date, at which point the function returns true and control goes back to code displaying the menu.

The functions all look similar to this:

boolean inputTime(time_t& var) {

    static int state = 0;
    static tmElements_t tmElem;
    breakTime(var, tmElem);

    switch (state) {

    case 0: {      // get set up
        encoderOn();
        buttonOn();
        state++;
        break;
    }
    case 1: {  // input the hours
        if (checkButton()) {  // done with hours go to minutes
            state++;
            break;
        }
        int setHour = tmElem.Hour;   // Have to do this for wraparound
        useRotaryEncoder(setHour, 0, 23);  // Otherwise this uses unsigned vars.
        tmElem.Hour = setHour;
        setCursor(0, 1);
        break;
    }
    case 2: {  // input the minutes
        if (checkButton()) {
            state++;
            break;
        }
        int setMinute = tmElem.Minute;
        useRotaryEncoder(setMinute, 0, 59);
        tmElem.Minute = setMinute;
        setCursor(0, 4);
        break;
    }
    case 3: {   // input the year
        if (checkButton()) {
            state++;
            break;
        }
        useRotaryEncoder(tmElem.Year, 1);
        setCursor(1, 9);
        break;
    }
    case 4: {  // input the month

        if (checkButton()) {
            state++;
            break;
        }
        useRotaryEncoder(tmElem.Month, (byte) 1, (byte) 12); // OK to use unsigned vars since 0 will wrap around.
        setCursor(1, 1);
        break;
    }
    case 5: {  // input the day

        if (checkButton()) {
            state++;
            break;
        }
        byte monthDays[] = { 31, 28, 31, 30, 31, 30, 31, 31, 30, 31, 30, 31 };
        if (LEAP_YEAR(tmElem.Year)) {
            monthDays[1] = 29;
        }
        useRotaryEncoder(tmElem.Day, (byte) 1, monthDays[tmElem.Month - 1]);
        setCursor(1, 4);
        break;
    }
    case 6: { // wrap it up and let the caller know we're done with his variable. var should still be OK from the last time we touched it.
        state = 0;
        encoderOff();
        buttonOff();
        showCursor(false);
        return true;
    }

    }  // end of switch (state)

    var = makeTime(tmElem);

    displayTime(var);

    if (reblMenu.isCancelled()) {
        state = 0;
        encoderOff();
        buttonOff();
        showCursor(false);
        return true;
    }


    return false;
}

The idea is that this is a state machine within a state machine. It works and works well within the context of this one project. But is this a bad idea?

If you need more code see my github. It's pretty long.

I've used a similar structure in one of my programs. Works fine for me too. Even with millis() in most of the cases. The only minor criticism I'd make is that you haven't included a default case in case state becomes more than 6 (very unlikely, I know, but just in case!).

The smell was that you shouldn't use a switch/case to sequence procedural code.

Ugh. I've read that crap too. The author fails to balance that criticism against the full extent of the proposed alternative. Any other way of structuring that code is larger; both in lines-of-code-written and size-of-code-generated. The simple fact is that, all other things being equal, more code equals more bugs.

So, you, as our protagonist, has to decide if "small and smelly but less chance of bugs" is better than "large and perfumed but more chance of bugs". For embedded programming, I nearly always favour a path that leads to less bugs.

There is a point where a switch/case like that is most certainly worth restructuring. A friend wrote a PID algorithm that was a few thousand lines of code. There were four major sections. Each major section was composed of a giant switch/case. The thing was a nightmare. It was "smelly" to the point of being rancid. But your example is clearly on the other end of the spectrum.