views:

158

answers:

5

In the current project, there are lots of GetData() method, which get different kind of data from hand written database at run time, and set them in different fields in a class. The projects then have such methods.

void GetData(Datatype type, int& value);
void GetData(Datatype type, double& value);
void GetData(Datatype type, long& value);
void GetData(Datatype type, longlong& value);
....

There are lots of data types, so, these method are often called with a switch with lots of branches.

void GetData(Datatype type, int& value)
{
     switch(type)
     {
       Type1:
        value = GetDataFromDB1(TYPE1);
        Type2:
              value = .. //get from different source
        ...

     }   

}

void GetData(Datatype type, double& value)
....

As you see, the the GetData()s are classified according to the second param. And within each single GetData(), there are lots of branches. Is this a reasonable method to get data?

+2  A: 

To answer "best way to refactor this" would take more context. For example maybe it's appropriate to change how the data is stored in addition to how it's fetched like you are showing. I doubt that this code structure needs optimization though.

tenfour
+1  A: 

Sorry, Java programmer coming:)
I have 2 solutions here:

  1. If you want change code not much, you could put Datatypes and their relevant operations into a Map. So long switch(type) statement could be one statement map.get(type).

  2. If more decent way, use Polymorphism. You could define interface for DBOperations, each specific DBOperation could be defined in its implemented class. So GetData just needs for invoke DBOperation interface. It's rough idea.

卢声远 Shengyuan Lu
A: 

Not sure, if I understood it right, but this might help

typedef int Datatype;

template <Datatype d, class T> struct Wrap{
    Wrap(T (*p)(Datatype)) : mp(p){}
    template<class T> void GetData(T &value){
        value = (*mp)(d);
    };
    T (*mp)(Datatype);
};

int GetDataFromDB(Datatype){
    return 0;
}

int main(){
    Wrap<0, int> wi0(GetDataFromDB);
    int val;
    wi0.GetData(val);
}
Chubsdad
Once you have the type fixed in your `Warp` template anyway, you could change `GetData()` to be used more easily: `int val = w10.GetData()`. Passing it as a function argument was very likely only done to facilitate template argument deduction.
sbi
@sbi: true, however I tried to be in as much synch with the original code as possible (which returns the value by reference
Chubsdad
-1: Introducing a set of objects in 1:1 correspondence with existing functions does not constitute refactoring. That's just obfuscation.
Potatoswatter
A: 

It looks like you're outgrowing a custom SQL interface. Look into existing C++ SQL libraries. Unfortunately I can't recommend any (last time I needed one, I wrote a new one at foolish great expense), but see the related question C++ SQL database library comparison.

Also, libpqxx, the official C++ PostgreSQL client, looks very nice.

Hmm, looking around, I can't seem to find a library with generic support for structs like what I wrote. There must be some out there… you should be able to plug in your tables and headers (with a little modification, maybe) and get automatic interface code.

Potatoswatter
A: 

You might want to refactor it to make it more maintainable.

Refactoring it for optimization will likely have no benefit, or negative, unless you've shown that it is a performance problem, by profiling.

Mike Dunlavey