-1

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?

7
  • please tell me what defects with this post rather than just vote it down WITHOUT any suggestion
    – Andy Lin
    Dec 6, 2020 at 7:35
  • 1
    On this site, it is quite normal that each question, regardless of being good or bad, gets at least one or two downvotes with no comment. I suspect some of the active community members to suffer from serious communication problems, maybe Asperger syndrome, since they seem to be totally immune to any request of behaving more constructive and leaving comments to their downvotes. So don't take these downvotes personal.
    – Doc Brown
    Dec 6, 2020 at 8:25
  • 2
    ... said that, I think questions where people have to work themselves through a bunch of code first to understand what is asked are not really a good fit to this site. The question would have been way easier to read if you would have taken the time to write a short summary of what you changed during this refactoring and the main aspect of this change you are lacking a name for,
    – Doc Brown
    Dec 6, 2020 at 8:35
  • 1
    @Alexander: last year I tried to encourage some change in voting behaviour, but I think it failed. 90% of all questions asked each day get downvotes, most often uncommented ones, even if those questions are clearly on-topic and not too badly written. What I wrote above is simply a conclusion from my long-term observation of this community, there is a small part who seem to have lost the ability to see how their behaviour harms the site.
    – Doc Brown
    Dec 7, 2020 at 14:13
  • 1
    ... and don't get me wrong, I don't think explanations for downvotes should be mandatory - if questions are really showing no research effort, are obviously off-topic, or spam, or "fix-my-code" requests, then a downvote without a comment is fine.
    – Doc Brown
    Dec 7, 2020 at 14:19

2 Answers 2

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.

3

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.

2
  • May you explain in more detail about "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." I did not get it.
    – Andy Lin
    Dec 7, 2020 at 6:55
  • 1
    @AndyLin a design that allowed you to specify each command in it's own source file would mean you could add new commands without rewriting old source files. Dec 7, 2020 at 7:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.