3

I'm trying to find a refactoring solution to the following scenario. I have many duplicated functions, each doing exactly the same thing except calling a different member function on a class instance available only at the inner-most scope.

struct MyObject
{
    void Function1() {};
    void Function2() {};
    void Function3() {};
    void Function4() {};
};

std::vector<MyObject *> objects;

void CommandHandler1()
    {
    if (true) // lots of preconditions
        {
        size_t id = 0;
        if (true) // lots of lookup code for id
            {
            MyObject *myObject = objects.at(id);
            myObject->Function1();
            }
        // refresh code
        }
    // cleanup code
    }

// another CommandHandler2, CommandHandler3, CommandHandler4, etc. doing the same thing but for a different member function.

I'd like to avoid this duplication of multiple CommandHandlerx because they're so very similar. What I've tried is to pass a std::function to work on any instance, because I can't bind to a specific instance. An answer to this question implies that the following mini-program should compile.

#include <functional>

void CommandHandler(std::function<void(MyObject *)> func)
{
    if (true) // lots of preconditions
        {
        size_t id = 0; 
        if (true) // lots of lookup code for id
            {
            MyObject *myObject = objects.at(id);
            func(myObject);
            }
        }
    // cleanup code
}

void MenuHandler(int chosen)
    {
    switch (chosen)
        {
        case 0: CommandHandler( &MyObject::Function1); break;
        case 1: CommandHandler( &MyObject::Function2); break;
        case 2: CommandHandler( &MyObject::Function3); break;
        case 3: CommandHandler( &MyObject::Function4); break;
        }
    }

int main()
{
    MyObject *obj1 = new MyObject;
    MyObject *obj2 = new MyObject;
    objects.push_back(obj1);
    objects.push_back(obj2);

    MenuHandler(1);

    delete obj1; delete obj2;
    return 0;
}

This unfortunately doesn't build. I presume because I'm not binding against a specific this pointer:

C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\functional(506): error C2664: 'void std::_Func_class<_Ret,MyObject *>::_Set(std::_Func_base<_Ret,MyObject *> *)' : cannot convert argument 1 from '_Myimpl *' to 'std::_Func_base<_Ret,MyObject *> *'

My question is either (1) am I doing something wrong with the above std::function attempt, or (2) is there any other pattern for refactoring this scenario?

Community
  • 1
  • 1
acraig5075
  • 10,588
  • 3
  • 31
  • 50
  • 1
    Your code should work. Probably MSVC bug (I know that they completely rewrote `` for VS2015, and heard that their original version had some difficulties with member pointers). Have you tried passing a pointer-to-member-function directly? (Make the parameter `void (MyObject::* func)()`, and call as `(myObject->*func)()`) – T.C. Sep 02 '15 at 09:52

1 Answers1

3

Your code works with GCC, so it can very well be a peculiarity of MSVC's implementation of std::function. I'll offer some workarounds.

If the functions 1–4 really all follow the same signature, you don't need std::function at all and a plain old pointer to member function will do:

void CommandHandler(void (MyObject::*func)())
{
    if (true) // lots of preconditions
        {
        size_t id = 0; 
        if (true) // lots of lookup code for id
            {
            MyObject *myObject = objects.at(id);
            (myObject->*func)();
            }
        }
    // cleanup code
}

If you do need an opaque callable (e.g. because you're binding some parameters), you should be able to use std::mem_fn to obtain a suitable initialiser for std::function:

void MenuHandler(int chosen)
    {
    switch (chosen)
        {
        case 0: CommandHandler( std::mem_fn(&MyObject::Function1)); break;
        case 1: CommandHandler( std::mem_fn(&MyObject::Function2)); break;
        case 2: CommandHandler( std::mem_fn(&MyObject::Function3)); break;
        case 3: CommandHandler( std::mem_fn(&MyObject::Function4)); break;
        }
    }
Angew is no longer proud of SO
  • 167,307
  • 17
  • 350
  • 455