tags:

views:

615

answers:

7

Whilst refactoring some legacy C++ code I found that I could potentially remove some code duplication by somehow defining a variable that could point to any class method that shared the same signature. After a little digging, I found that I could do something like the following:

class MyClass
{
protected:
    bool CaseMethod1( int abc, const std::string& str )
    {
     cout << "case 1:" << str;
     return true;
    }

    bool CaseMethod2( int abc, const std::string& str )
    {
     cout << "case 2:" << str;
     return true;
    }

    bool CaseMethod3( int abc, const std::string& str )
    {
     cout << "case 3:" << str;
     return true;
    }

public:
    bool TestSwitch( int num )
    { 
     bool ( MyClass::*CaseMethod )( int, const std::string& );

     switch ( num )
     {
      case 1: CaseMethod = &MyClass::CaseMethod1;
        break;
      case 2: CaseMethod = &MyClass::CaseMethod2;
        break;
      case 3: CaseMethod = &MyClass::CaseMethod3;
        break;
     }

     ...

     bool res = CaseMethod( 999, "hello world" );

     ...

     reurn res;
    }
};

My question is - is this the correct way to go about this? Should I consider anything that Boost has to offer?

Edit...

Ok, my mistake - I should be calling the method like so:

bool res = ( (*this).*CaseMethod )( 999, "Hello World" );
+1  A: 

You can certainly do it, although the CaseMethod call isn't correct (it's a pointer to member function, so you have to specify the object on which the method should be called). The correct call would look like this:

bool res = this->*CaseMethod( 999, "hello world" );

On the other hand, I'd recommend boost::mem_fn - you'll have less chances to screw it up. ;)

Paulius Maruška
+7  A: 

What you have there is a pointer-to-member-function. It will solve your problem. I am surprised that your "TestSwitch" function compiles, as the calling syntax is slightly different to what you might expect. It should be:

bool res = (this->*CaseMethod)( 999, "hello world" );

However, you might find a combination of boost::function and boost::bind makes things a little easier, as you can avoid the bizarre calling syntax.

boost::function<bool(int,std::string)> f=
    boost::bind(&MyClass::CaseMethod1,this,_1,_2);

Of course, this will bind it to the current this pointer: you can make the this pointer of the member function an explicit third parameter if you like:

boost::function<bool(MyClass*,int,std::string)> f=
    boost::bind(&MyClass::CaseMethod1,_1,_2,_3);

Another alternative might be to use virtual functions and derived classes, but that might require major changes to your code.

Anthony Williams
A: 

There's nothing intrinsically wrong with the localised example you've given here, but class method pointers can often be tricky to keep 'safe' if you use them in a wider context, such as outside the class they're a pointer of, or in conjunction with a complex inheritance tree. The way compilers typically manage method pointers is different to 'normal' pointers (since there's extra information beyond just a code entry point), and consequently there are a lot of restrictions on what you can do with them.

If you're just keeping simple pointers the way you describe then you'll be fine, but fore more complex uses you may want to take a look at a more generalised functor system such as boost::bind. These can take pointers to just about any callable code pointer, and can also bind instanced function arguments if necessary.

Nik
+1  A: 

I don't see the difference between your call and simply calling the method within the switch statement.

No, there is no semantic or readability difference.

The only difference I see is that you are taking a pointer to a method and so forbids to the compiler to inline it or optimizes any call to that method.

Vincent Robert
+3  A: 

You could also build a lookup (if your key range is reasonable) so that you end up writing:

this->*Methods[num]( 999, "hello world" );

This removes the switch as well, and makes the cleanup a bit more worthwhile.

Simon Steele
+1  A: 

Without wider context, it's hard to figure out the right answer, but I sew three possibilities here:

  • stay with normal switch statement, no need to do anything. This is the most likely solution

  • use pointers to member function in conjunction with an array, as @Simon says, or may be with a map. For a case statement with a large number of cases, this may be faster.

  • split t he class into a number of classes, each carrying one function to call, and use virtual functions. This is probably the best solution, buy it will require some serious refatoring. Consider GoF patterns such as State or Visitor or some such.

Arkadiy
A: 

There are other approaches available, such as using an abstract base class, or specialized template functions.

I'll describe the base class idea.

You can define an abstract base class

class Base { virtual bool Method(int i, const string& s) = 0; };

Then write each of your cases as a subclass, such as

class Case1 : public Base { virtual bool Method(..) { /* implement */; } };

At some point, you will get your "num" variable that indicates which test to execute. You could write a factory function that takes this num (I'll call it which_case), and returns a pointer to Base, and then call Method from that pointer.

Base* CreateBase(int which_num) { /* metacode: return new Case[which_num]; */ }
// ... later, when you want to actually call your method ...
Base* base = CreateBase(23);
base->Method(999, "hello world!");
delete base;  // Or use a scoped pointer.

By the way, this application makes me wish C++ supported static virtual functions, or something like "type" as a builtin type - but it doesn't.

Tyler