0

I have this simple piece of code (Command pattern) that doesn't work as expected.

Usually a polymorphic type would work when it's manipulated by either a pointer or a reference. So if Command is an interface and m_command is a reference, then SetCommand should work for different concrete types of Command (eg, CommandLightOn and CommandGarageDoorOpen) ? It turns out not the case. Once SimpleRemote is instantiated with CommandLightOn, the SetCommand has completely no effect. In this example, attempt to change the underlying object to CommandGarageDoorOpen has no errors, but no effect either.

It works fine if m_command in SimpleRemote is changed to pointer type. So the question is why reference type fails to work in this case?

class SimpleRemote
{
public:
    SimpleRemote( Command& command ) : m_command{ command } {}

    void SetCommand( Command& command )
    {
        m_command = command;  //<-- broken
    }

    void ButtonPressed()
    {
        m_command.execute();
    }

private:
    Command& m_command;
};

Output:

Light is on

Light is on #<-- expect that it prints "Garage door is up"

Full code of this example (simple Command pattern):

#include <iostream>
using std::cout;

class Light
{
public:
    void On()
    {
        cout << "Light is on\n";
    }

private:
};

class GarageDoor
{
public:
    void Up()
    {
        cout << "Garage door is up\n";
    }

private:
};


// the Command interface
class Command
{
public:
    virtual void execute() = 0;
};


class CommandLightOn : public Command
{
public:
    CommandLightOn( Light light ) : m_light{ light }{}

    void execute() override
    {
        m_light.On();
    }   

private:
    Light m_light;
};


class CommandGarageDoorOpen : public Command
{
public:
    CommandGarageDoorOpen( GarageDoor door ) : m_door{door} {}  

    void execute() override
    {
        m_door.Up();
    }

private:
    GarageDoor m_door;
};


class SimpleRemote
{
public:
    SimpleRemote( Command& command ) : m_command{ command } {}

    void SetCommand( Command& command )
    {
        m_command = command;
    }

    void ButtonPressed()
    {
        m_command.execute();
    }

private:
    Command& m_command;
};


int main()
{
    Light light;
    CommandLightOn light_on( light );

    SimpleRemote remote( light_on );
    remote.ButtonPressed();

    GarageDoor door;
    CommandGarageDoorOpen door_open( door );
    remote.SetCommand( door_open );
    remote.ButtonPressed();
}
artm
  • 17,291
  • 6
  • 38
  • 54
  • 3
    Reference can be initialized only once, after that you cannot change referenced object. Here `m_command = command; //<-- broken` you invoke assignment operator of `Command` class. – rafix07 Apr 30 '21 at 08:43
  • @rafix07 thanks. So reassignment reference is the problem, not polymorphic type. Wondering why there isn't an error for such mistake though. – artm Apr 30 '21 at 09:25

2 Answers2

1

You have two problems here. Firstly - references in C++ are not rebindable like pointers. Which means that code like this:

int a = 1;
int& ref = a; // ref refers to a
int b = 2;
ref = b; // ref STILL referes to a

leaves you with a == 2 and b == 2. It's just how it works.

Second problem is so called object slicing. If you think about it it's only logical that since references work with polymprphism code like this is legal:

class A{
public:
    virtual void foo() = 0;
};

class B : public A {
public:
    void foo() override {}
};

class C : public A {
public:
    void foo() override {}
};

int main() {
    auto c = C{};
    A& a = c;
    auto b = B{};
    a = b;
}

What is going on here? Well, the A part of object c is being replaced wtih A part of object B according to A::operator= that is generated by default by the compiler. You can put some output in A::operator= to confirm what I am saying here.

Put these two things together and you have your problem. How to avoid it? First - if you want to rebind reference either use std::reference_wrappper or plain old pointer. Second - consider declaring operator=() deleted for your interfaces/abstract classes. This will make compiler generate error when trying to slice objects by mistake.

Some of your code after proposed fixes:

class Command
{
public:
    Command& operator=(const Command&) = delete;
    virtual void execute() = 0;
    virtual ~Command() = default; // <-- this is good practice
};
////............... rest of code here
class SimpleRemote
{
public:
    SimpleRemote(Command& command) : m_command{ command } {}

    void SetCommand(Command& command)
    {
        m_command = command;
    }

    void ButtonPressed()
    {
        m_command.get().execute();
    }

private:
    std::reference_wrapper<Command> m_command;
};
bartop
  • 9,971
  • 1
  • 23
  • 54
0

References must be initialized but cannot be updated. Unfortunately, compiler will not warn you! You can see an explanation here.

You must use a pointer instead of a reference:

class SimpleRemote
{
public:
    SimpleRemote( Command& command ) : m_command{ &command } {}

    void SetCommand( Command& command )
    {
        m_command = &command;
    }

    void ButtonPressed()
    {
        m_command->execute();
    }

private:
    Command* m_command;
};
Victor
  • 460
  • 3
  • 11