removing duplicate code cause redundant operation?

user_15017591 0
Viewed 59
void caller()
{
    int var;

    var = setvar();
    if (var == 1)
        func1();
    else if (var == 3)
        func3();
    else if (var == 8)
        func8();
}

void func1()
{
    common();
    //do something case 1 specific...
}

void func3()
{
    common();
    //do something case 3 specific...
}

void func8()
{
    common();
    //do something case 8 specific...
}

In this case, I feel very uncomfortable because I have duplicate function common() which could be written inside the caller() function to remove duplicate. So I can change above code like this:

void caller()
{
    int var;

    var = setvar();
    if (var == 1 || var == 3 || var == 8)
        common();
    if (var == 1)
        func1();
    else if (var == 3)
        func3();
    else if (var == 8)
        func8();
}

void func1()
{ //do something case 1 specific... }

void func3()
{ //do something case 3 specific... }

void func8()
{ //do something case 8 specific... }

However, I feel very uncomfortable in this case too, because caller() function now check var's value twice.

I don't have much experience of coding, so I don't know what to consider to choose which one is better. What is better code and why? What do I have to consider?

refactoring c
3 Answers

If common is only meant to be called by func1, func3, and func8, then the first option is better - there's no reason for caller to know about common, much less call it directly.

Ideally, caller should not care what func1, func3, and func8 do internally; it should only care about what parameters they take and what values they return. Suppose you need to add another function (we'll call it func5). Does it need to call common as well? Or does it not? What if you need to change the behavior of func3 so that it no longer calls common at all?

With the first option, you're localizing the knowledge about common to the functions that actually use it, which will make maintenance and debugging easier.

user_1345540

I'd go with the second approach.

First I would improve this code by separating the validation function (improves debugging) and using switch case to determine what function to run based on value of var.

Pros:

  • You can keep track with ease of where the function common() is used just by looking to the isAllowed numbers.
  • Don't need to write common() within every single function you use it. It'd be redundant if you use it in hundreds of func's and you might forget one.
  • Easily visualize when common() will be applied

Cons:

  • You have to add or remove cases in switch/case (use if/else wheter you prefer) depending on the isAllowed defined integers. It may lead to fire default if you forgot to add a case or if a bug happen.
  • You double check the var's value.
int isAllowed(int var){
    return var == 1 || var == 3 || var == 8;
}

void caller()
{
    int var = setVar();
    if (isAllowed(var)){
        common();
        switch(var){
            case 1:
                func1();
                break;
            case 3:
                func3();
                break;
            case 8:
                func8();
                break;
            default:
                break;
        }
    }
}
user_120969220

You could use a function pointer:

void caller()
{
    int var = setvar();
    void (*func)() = NULL;

    if (var == 1)
        func = func1;
    else if (var == 3)
        func = func3;
    else if (var == 8)
        func = func8;
    else return; // or error handling

    common();
    func();
}

If a special common is used for a specific case, then one could use two function pointer, one for common part another one for func part.

void caller()
{
    int var = setvar();
    void (*func)() = NULL;
    void (*fcommon)() = common; // default common

    if (var == 1) {
        func = func1;
    } else if (var == 3) {
        func = func3;
    } else if (var == 8) {
        fcommon = common_for_8;
        func = func8;
    } else {
        return; // or error handling
    }

    fcommon();
    func();
}
user_49894510
Related