CODEWITHSUNDEEP
c++refactoring
TopCoder
TopCoder

Reputation: 4296

Generalizing / Refactoring the code

My code is something like this :

if(country == china)
{
getCNData();

}

else {
getDefaultDataForallCountries();

}

Now I need to add similar logic as CN for some other country say US . The option available to me is to add one more country check in if condition and make it like 

if(country ==china && country==US){
getCNandUSData();

}

else {
getDefaultDataForallCountries();


}.

1) I am somehow not comfortable to go for this solution as this is not generic . What if tomorrow I need to have same CN logic applied to another country say france . Can you please suggest me how can I make my code better and more generic.

2) Also I am not comfortable with the naming conventions. If say I go with my approach of adding US in If condition , Should I refactor the class name and function name to getCNAndUSData () ?

I am not sure here of what is the correct way to deal with such existing code. Appreciate your comments.

Upvotes: 2

Views: 246

Answers (6)

Manoj R
Manoj R

Reputation: 3247

As suggested the case structure can be used here for better. In you case the code will be

switch (country)
case China:
GetCNData();
Break;
case US:
GetUSData();
Break;
default:
GetGenericData();

In case you think the GetUSData and GetCNData are same, then you can simply call GetCNData() from GetUSData().

GetUSData()
{
GetCNData();
}

If you want to have compile time independence, then use virtual inheritance. Create a base class named Country and have the method GetData() in it. Now Create other country classes by inheriting this and by changing the GetData() method appropriately.

This can also be done using the templates, not sure how.

Upvotes: 0

Chubsdad
Chubsdad

Reputation: 25537

Here is a very very broad skeletal outline based on LSP/Strategy Design Pattern which should give you the idea.

struct country{
    void getdata(){return dogetdata();}
    virtual ~country() = 0;
private:
    virtual void dogetdata() = 0;
};

country::~country(){}

void country::dogetdata(){
    // logic for default data for all countries
}

struct china : country{
private:
    void dogetdata(){return ;}
public:
    ~china(){}
};

struct xyz : country{
private:
    void dogetdata(){return ;}
public:
    ~xyz(){}
};

void get(country *p){
    p->getdata();
}

int main(){
}

Upvotes: 0

Alexander Rafferty
Alexander Rafferty

Reputation: 6233

You could bitwise comparison. This means assigning each country a bit:

#define China 1
#define US 2
#define France 4

Then you can compare like this:

if (country&(US|China)) {
  // ...
}

And about your naming problem, you could just use a generic name like getSpecificData(). Though I don't really understand your context.

Upvotes: 0

zabulus
zabulus

Reputation: 2523

use enums.

enum Country
{
    China,
    USA
};

your code will be refactored to this:

switch(country)
{
        case China:
        case USA:
           getCNAndUSData () ;
           break;
        ... //Here can be another countries
        default:
           getDefaultDataForallCountries()
}

Upvotes: 2

Doug
Doug

Reputation: 5328

This type of issue (overuse of "if" and "switch" statements) is neatly handled by implementing a strategy pattern with a abstract factory. Basically you want to change the algorithm without changing your implementation and duplicating the code over and over and over.

Enjoy!

Upvotes: 10

FatherStorm
FatherStorm

Reputation: 7183

You most likely would be better off using a "switch" statement..

switch ( ) {
case this-value:
Code to execute if == this-value
break;
case that-value:
Code to execute if == that-value
break;
...
default:
Code to execute if does not equal the value following any of the cases
break;
}

Read more: http://discuss.itacumens.com/index.php?topic=22753.0#ixzz11OXV6caP

Upvotes: 0

Related Questions