What design/component principle did I apply? How can I further improve it?

Question

I recently refactored a program for code feasibility and maintainability; however, I am totally unaware of what software principle I did implement. I just followed my feeling.

The purpose for this post is that I want to know what software principle I did implement so that I'm capable of referencing books or websites.

Before refactoring:

int main(void)
{
    char input_cmd[ENOUGH_SIZE] = {0};
    get_input_cmd(&input_cmd, ENOUGH_SIZE);
    int input_data = get_input_data(input_cmd, strlen(input_cmd));
    int access_right = get_access(); //1 = admin; 0 = user

    if (!strcmp(input_cmd, "A_cmd"))
    {
        if (access_right == admin)
        {
            A_calculate_data_and_save_result_to_buffer(input_data);
            move_all_buffer_to_rom();
            A_show_result();
            
            return 0;
        }
        else
        {
            //error: access deny
            return -1;
        }
    }
    else if (!strcmp(input_cmd, "B_cmd"))
    {
        B_calculate_data_and_save_result_to_buffer(input_data);
        move_all_buffer_to_rom();
        B_show_result();
        
        return 0;
    }
    else if (!strcmp(input_cmd, "C_cmd"))
    {
        C_calculate_data_and_save_result_to_buffer(input_data);
        C_show_result();
        
        return 0;
    }
    else if (!strcmp(input_cmd, "D_cmd"))
    {
        D_calculate_data_and_save_result_to_buffer(input_data);
        
        return 0;
    }
    else if (!strcmp(input_cmd, "E_cmd"))
    {
        E_calculate_data_and_save_result_to_buffer(input_data);
        move_all_buffer_to_rom();
        
        return 0;
    }
    else if (!strcmp(input_cmd, "F_cmd"))
    {
        if (access_right == admin)
        {
            F_calculate_data_and_save_result_to_buffer(input_data);
            move_all_buffer_to_rom();
            
            return 0;
        else
        {
            //error: access deny
            return -1;
        }
    }
    else if (!strcmp(input_cmd, "G_cmd"))
    {
        if (access_right == admin)
        {
            G_calculate_data_and_save_result_to_buffer(input_data);
            
            return 0;
        else
        {
            //error: access deny
            return -1;
        }
    }
    else if (!strcmp(input_cmd, "H_cmd"))
    {
        if (access_right == admin)
        {
            H_calculate_data_and_save_result_to_buffer(input_data);
            H_show_result();
            return 0;
        else
        {
            //error: access deny
            
            return -1;
        }
    }
    else
    {
        //error: cmd not found
        return -2;
    }
    
    return 0;
}

After refactoring:

static bool find_cmd(char* input, uint16_t* index);
static bool check_access(void);
static void do_nothing(void);

typedef struct cmd_t cmd_t;
struct cmd_t
{
    const char* cmd_name;
    bool need_check_access;
    bool need_move_all_buffer_to_rom;
    void (*calculate_data_and_save_result_to_buffer)(int);
    void (*show_result)(void);
};

#define YES 1
#define NO 0

static cmd_t cmd_table[] =  
{
    {"A_cmd", YES, YES, &A_calculate_data_and_save_result_to_buffer, &A_show_result},
    {"B_cmd", NO,  YES, &B_calculate_data_and_save_result_to_buffer, &B_show_result},
    {"C_cmd", NO,  NO,  &C_calculate_data_and_save_result_to_buffer, &C_show_result},
    {"D_cmd", NO,  NO,  &D_calculate_data_and_save_result_to_buffer, &do_nothing},
    {"E_cmd", NO,  YES, &E_calculate_data_and_save_result_to_buffer, &do_nothing},
    {"F_cmd", YES, YES, &F_calculate_data_and_save_result_to_buffer, &do_nothing},
    {"G_cmd", YES, NO,  &G_calculate_data_and_save_result_to_buffer, &do_nothing},
    {"H_cmd", YES, NO,  &H_calculate_data_and_save_result_to_buffer, &H_show_result},
};


int main(void)
{
    char input_cmd[ENOUGH_SIZE] = {0};
    get_input_cmd(&input_cmd, ENOUGH_SIZE);
    
    uint16_t idx = 0;
    bool cmd_found = find_cmd(input_cmd, &idx);
    if (!cmd_found)
    {
        return -2;
    }
    
    if (cmd_table[idx].need_check_access)
    {
        bool access_allowed = check_access();
        if (!access_allowed)
        {
            return -1;
        }
    }
    
    int input_data = get_input_data(input_cmd, strlen(input_cmd));
    cmd_table[idx].calculate_data_and_save_result_to_buffer(input_data);
    
    if (cmd_table[idx].need_move_all_buffer_to_rom)
    {
        move_all_buffer_to_rom();
    }

    cmd_table[idx].show_result();
    
    return 0;
}

static bool find_cmd(char* input, uint16_t* index)
{
    //loop table and compare string to find cmd
}

static bool check_access(void)
{

#define allowed 1
#define denied 0

    bool rv = denied;
    
    int access_right = get_access(); //1 = admin; 0 = user

    if (access_right == admin)
    {
        rv = allowed;
    }

    return rv;
}

static void do_nothing(void)
{
    
}

The original code is definitely not that easy to read. For example, A_calculate_data_and_save_result_to_buffer might be set_pswd; A_cmd might be set_pswd, too.

It costs me a lot of time reading into each function, knowing what each function do, brainstorming, concluding that although every function's name is different, but all functions in fact are doing the same thing on concept level, and finally forming the struct members and deciding to use a function pointer to represent all the functions.

By now, I'm still not figuring out what principle I did apply.

As title, what design/component principle did I apply? How can I further improve it?

Answer 1

Candied Orange is correct in that this is an application of Replace Conditional with Polymorphism. But I don't think that quiet explains why your result from this approach is so powerful.

It might be hard to see but the dispatch table is a fairly crude DSL (Domain Specific Language) which clearly expresses the problem and desired resolution without detailing how to do it. The how to do it part is implemented generically below by the consuming functions, a crude interpreter of sorts.

So why this is so powerful is that this refactoring has lifted the problem up a language level. Its Declarative Programming.

Not all applications of this refactoring will result in a Declarative Program. You could have applied the same technique and arrived at class based polymorphism which would have had the same knowledge spread throughout the code base, as the if else chain had it spread throughout the function.

The safety would have improved as the compiler could spot check some more of the implementation details (like ensuring functions are implemented), and it would have made adding/adjusting behaviour easier too without having to update each call site. But it would not have summarised the behaviour in such a simple problem specific view as the solution you arrived at.

Answer 2

Believe it or not this is a refactoring called Replace Conditional with Polymorphism. It might be hard to see that because you didn't use classes or objects to get your polymorphism. You used an array of structs. But that's still polymorphism.

This refactoring can be driven by a desire to reduce duplication or by a desire for flexibility. You have reduced duplication but since each struct resides in the same source file you haven't achieved the flexibility you might if adding a new command only required adding one new file. If that's important, you could still do that.

If you're fine editing and compiling this file every time a new command is needed then this dispatch table is fine as is. It has a much more readable form than the nested if-else structure.