Static variable representing program state accessed with multiple functions

Question

I have this method that performs certain functions based on the state that the algorithm is currently at:

static infoStruct_t info;
void first_part_of_algorithm(void){
 info.state = FIRST_STATE;
 //set other information here as well
 while(info.state < FINAL_STATE){
 switch(info.state){
 case FIRST_STATE:
 perform_first_state_function();
 break;
 case SECOND_STATE:
 perform_second_state_function();
 break;
 case THIRD_STATE:
 perform_check_needed_for_algorithm();
 break;
 default:
 //should never be reached
 break;
 }
 }
}

Now in each of these "perform" functions, the state can change. The first function always causes the state to go to the second state, the second function always makes the state go to the third state, and the third function will either reset the state to the first state, or go to the final state and leave the first part of the algorithm. Also, different parts of the info struct are set at various points in any of the functions.

I have not yet written the second part of the algorithm, but it will need to have access to the info struct

Here are the following state / check functions:

void perform_first_state_function(void){
 while(wait_for_condition()!=SUCCESS){
 sleep();
 }
 info.state = SECOND_STATE;
 sleep();
}
void perform_second_state_function(void){
 int attempts = 0;
 while(attempts < ALLOWED_ATTEMPTS){
 if(event_is_ready()){
 if(info.detection_number==0){
 //set various info properties
 ...
 info.first_detection_time = get_time();
 info.detection_number = 1;
 break;
 }
 else{
 //set other info properties
 ...
 info.second_detection_time = get_time();
 info.state = THIRD_STATE;
 break;
 }
 }
 attempts++;
 sleep();
 }
}

My questions are:

• Is this a code smell that the functions are implicitly accessing the static info variable?
• Should I explicitly pass in the info struct as a pointer? (for readability)
• Does the info struct need to be a singleton?
• Any general suggestions?

• Malachi: This looks like code that has not yet been written, we don't review theoretical questions here on Code Review.
• C.B.: @Malachi it's been written, i've just simply abstracted it for confidentiality's sake. It is literally above except with different struct, state, and function names.
• Jamal: Yes, it is a bit harder to review this with the hypothetical names. Beyond that, could you provide more of this code so that it can be determined if static and singleton is needed?
• linguamachina: @C.B., is this a single-threaded algorithm?
• C.B.: @headeronly yes

Answer 1

Minor note regarding proper type names:

Avoid having user-defined types ending in _t as they're already used by the C standard and POSIX. Also, to differentiate from variables and functions that are in camelCase or snake_case, types could be named in PascalCase.

As such, infoStruct_t can become InfoStruct.

Answer 2

Since it's a single-threaded algorithm, I think you could simplify things a bit to avoid all the state-checking. Unless you're reading the state from another thread, you don't really need to keep track of it at all. You could simply make perform_check_needed_for_algorithm return a bool:

for (;;)
{
 /* info.state = FIRST_STATE; */
 perform_first_state_function();
 /* info.state = SECOND_STATE; */
 perform_second_state_function();
 /* info.state = THIRD_STATE; */
 if (perform_check_needed_for_algorithm())
 {
 /* info.state = FINAL_STATE; */
 break;
 }
}

The commented-out state assignments are just there to show what's happening at each stage. Although a state machine's inner functions ought to be responsible for updating the state, in an example as simple as this, it's arguably more readable to update the state in the outer function.

If you need access to the state from another thread, then you'll need to update it using atomic operations, and mark it as volatile.

Afterthought: perform_second_state_function doesn't always advance the state. It fails once ALLOWED_ATTEMPTS is exceeded. However, it dumps you back into the outer loop, and dives straight back into perform_second_state_function because state.info is still SECOND_STATE. Bit of a design error there IMO - it will keep trying no matter what.