views:

131

answers:

6

Hi ,

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.

A: 

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

FatherStorm
+9  A: 

Hi,

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!

Doug
+2  A: 

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()
}
zabulus
A: 

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.

Alexander Rafferty
Please don't EVER consider using `#define` for constants in C++ code. Be thankful that I am not sitting in the same room you are, I would have bashed you on the head.
Matthieu M.
A: 

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(){
}
Chubsdad
dogetdata() needs to be protected in the base class (public would technically work as well but I'm sure that's not what you want).
gregg
@gregg: Yes, it could be protected if I wanted my derived classes to call it. That would basically depend on other design considerations e.g.
Chubsdad
A: 
Manoj R