2
\$\begingroup\$

I'm using this Game Messenger as an instant messaging tool in my game. Just wondering peoples on thoughts on it.

Thank you!

    enum class eGameMessageType
{
    AddItemGUI = 0,
    RemoveItemGUI,
    UpdateSelectionBoxGUI,
    UpdateItemQuantityGUI,
    DestroyCubeSetPosition, 
    DestroyCubeReset,
    SelectedCubeSetPosition,
    SelectedCubeSetActive,
    SpawnPickup,
    PlayerDisgardPickup,
    AddToPlayerInventory,
    Max = AddToPlayerInventory
};

struct Listener : private NonCopyable
{
    Listener(const std::function<void(const void*)>& fp, const void* ownerAddress)
        : m_listener(fp),
        m_ownerAddress(ownerAddress)
    {
        assert(ownerAddress);
    }
    Listener(Listener&& rhs) noexcept
        : m_listener(rhs.m_listener),
        m_ownerAddress(rhs.m_ownerAddress)
    {
        rhs.m_listener = nullptr;
        rhs.m_ownerAddress = nullptr;
    }
    Listener& operator=(Listener&& rhs) noexcept
    {
        m_listener = rhs.m_listener;
        m_ownerAddress = rhs.m_ownerAddress;

        rhs.m_listener = nullptr;
        rhs.m_ownerAddress = nullptr;

        return *this;
    }

    std::function<void(const void*)> m_listener;
    const void* m_ownerAddress;
};

class GameMessenger : private NonCopyable, private NonMovable 
{
public:
    static GameMessenger& getInstance()
    {
        static GameMessenger instance;
        return instance;
    }

    template <typename GameMessage>
    void subscribe(const std::function<void(const GameMessage&)>& gameMessage, const void* ownerAddress)
    {
        auto& listeners = m_listeners[static_cast<int>(GameMessage::getType())];
        assert(!isOwnerAlreadyRegistered(listeners, GameMessage::getType(), ownerAddress));

        listeners.emplace_back(reinterpret_cast<std::function<void(const void*)> const&>(gameMessage), ownerAddress);
    }

    template <typename GameMessage>
    void unsubscribe(const void* ownerAddress)
    {
        auto& listeners = m_listeners[static_cast<int>(GameMessage::getType())];
        assert(isOwnerAlreadyRegistered(listeners, GameMessage::getType(), ownerAddress));

        auto iter = std::find_if(listeners.begin(), listeners.end(), [ownerAddress](const auto& listener)
        {
            return listener.m_ownerAddress == ownerAddress;
        });

        assert(iter != listeners.end());
        listeners.erase(iter);
    }

    template <typename GameMessage>
    void broadcast(GameMessage gameMessage)
    {
        const auto& listeners = m_listeners[static_cast<int>(GameMessage::getType())];
        for (const auto& listener : listeners)
        {
            reinterpret_cast<std::function<void(const GameMessage&)> const&>(listener.m_listener)(gameMessage);
        }
    }

private:
    GameMessenger() {}
    std::array<std::vector<Listener>, static_cast<size_t>(eGameMessageType::Max) + 1> m_listeners;

    bool isOwnerAlreadyRegistered(const std::vector<Listener>& listeners, eGameMessageType gameMessageType, const void* ownerAddress) const
    {
        assert(ownerAddress != nullptr);

        if (!listeners.empty())
        {
            auto result = std::find_if(listeners.cbegin(), listeners.cend(), [ownerAddress](const auto& listener)
            {
                return listener.m_ownerAddress == ownerAddress;
            });

            return result != listeners.cend();
        }
        else
        {
            return false;
        }
    }
};
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Is this program made using a game engine? \$\endgroup\$ – Aryan Parekh Dec 24 '20 at 12:38
  • \$\begingroup\$ It's from a little Minecraft clone that I made a while ago. It's only a pet project - am a student currently. \$\endgroup\$ – Ryan Swann Dec 24 '20 at 14:37
  • 1
    \$\begingroup\$ The thing is we don't have the definition of the class you derive from \$\endgroup\$ – Aryan Parekh Dec 24 '20 at 16:43
2
\$\begingroup\$

Move struct Listener into class GameMessenger

The struct Listener is just an implementation detail of GameMessenger, you can move it into class GameMessenger like so:

class GameMessenger: ...
{
    struct Listener: ...
    {
         ...
    };

    ...
};

Now that it is private, I would also keep Listener very simple, and just have it hold the two member variables, without adding any member functions.

Pass messages as const references

The actual message handler functions take a const reference to the message, but your GameMessenger::broadcast() takes it by value, making an unnecessary copy. Change it to:

void broadcast(const GameMessage &gameMessage) {...}

Naming things

Some names could be improved. For example, struct Listener has a member variable m_listener. Combined with the std::vector<Listener>, you get the word listener repeated a lot, like in GameMessenger::broadcast(), where there's even a listener.m_listener. Consider renaming m_listener to m_handler or m_callback.

Some names are a bit longer than necessary. I suggest these changes:

- `typename GameMessage` -> `typename Message`
- `isOwnerAlreadyRegistered()` -> `isRegistered()`

Make class GameMessenger a template

With void pointers you throw type-safety overboard, and requires you to reinterpret_cast<>() things. I can't say much about m_ownerAddress, however the main problem here is that you have a non-templated class GameMessenger. Apart from the constructor, all the public member functions are template <typename GameMessage>. Instead of having one GameMessenger object being responsible for all possible message types, consider making the class templated itself, and only be responsible for one specific message type.

If you implement it this way, there will be one GameMessenger object per message type. That means there is no longer a need to have a std::array for each of the possible message types, and in fact enum eGameMessageType is no longer necessary.

Here is an example of how it might look:

template <typename Message>
class GameMessenger: private NonCopyable, private NonMovable 
{
    struct Listener {
        const std::function<void(const Message &)> m_handler;
        const void *m_ownerAddress;
    };

public:
    static GameMessenger& getInstance()
    {
        static GameMessenger instance;
        return instance;
    }

    void subscribe(const std::function<void(const Message&)>& handler, const void* ownerAddress)
    {
        assert(!isRegistered(ownerAddress));
        listeners.emplace_back(handler, ownerAddress);
    }

    template <typename Message>
    void unsubscribe(const void* ownerAddress)
    {
        auto iter = std::find_if(m_listeners.begin(), m_listeners.end(), [ownerAddress](const auto& listener)
        {
            return listener.m_ownerAddress == ownerAddress;
        });

        assert(iter != listeners.end());
        listeners.erase(iter);
    }

    void broadcast(const Message &message)
    {
        for (const auto& listener: m_listeners)
        {
            listener.m_handler(message);
        }
    }

private:
    std::vector<Listener> m_listeners;

    bool isRegistered(const void* ownerAddress) const
    {
        assert(ownerAddress != nullptr);

        auto result = std::find_if(listeners.cbegin(), listeners.cend(), [ownerAddress](const auto& listener)
        {
            return listener.m_ownerAddress == ownerAddress;
        });

        return result != listeners.cend();
    }
};

So how do you use it? Instead of writing:

void handleSpawnPickup(const SpawnPickupMessage &message) {...}
void *ownerAddress = ...;

auto messenger = GameMessenger.getInstance();
messenger.subscribe(handleSpawnPickup, ownerAddress);

You replace the last two lines with:

auto messenger = GameMessenger<SpawnPickupMessage>.getInstance();
messenger.subscribe(handleSpawnPickup, ownerAddress);

If you want to avoid having to explicitly mention the type when getting the messenger instance, consider writing an out-of-class function that subscribes to the right instance for you:

template<typename Message>
void subscribeToMessenger(const std::function<void(const Message&)>& handler, const void* ownerAddress)
{
    GameMessenger<Message>::getInstance().subscribe(handler, ownerAddress);
}

So that you can just write:

subscribeToMessenger(handleSpawnPickup, ownerAddress);
\$\endgroup\$
1
  • \$\begingroup\$ Hi there, thank you! I have responded with a follow up question. \$\endgroup\$ – Ryan Swann Dec 27 '20 at 14:19
0
\$\begingroup\$

Thank you @G. Sliepen I have rectified the code but now have an issue where I have the public functions in GameMessenger that I'd like to make private or remove entirely - so the caller can only use the functions outside of class.

Just wondering the best way to do this? Thanks.

 template <typename Message>
    class GameMessenger : private NonCopyable, private NonMovable
    {
        struct Listener
        { 
            Listener(const std::function<void(const Message&)>& callback, const void* ownerAddress)
                : callback(callback),
                ownerAddress(ownerAddress)
            {}
    
            std::function<void(const Message&)> callback;
            const void* ownerAddress;
        };
    
    public:
        static GameMessenger<Message>& getInstance()
        {
            static GameMessenger<Message> instance;
            return instance;
        }
    
        void subscribe(const std::function<void(const Message&)>& callback, const void* ownerAddress)
        {
            assert(!isRegistered(ownerAddress));
            m_listeners.emplace_back(callback, ownerAddress);
        }   
    
        void unsubscribe(const void* ownerAddress)
        {
            assert(isRegistered(ownerAddress));
            auto listener = std::find_if(m_listeners.begin(), m_listeners.end(), [ownerAddress](const auto& listener)
            {
                return listener.ownerAddress == ownerAddress;
            });
            assert(listener != m_listeners.cend());
            m_listeners.erase(listener);
        }
    
        void broadcast(const Message& message) const
        {
            assert(!m_listeners.empty());
            for (const auto& listener : m_listeners)
            {
                listener.callback(message);
            }
        }
    
    private:
        GameMessenger() {}
    
        std::vector<Listener> m_listeners;
    
        bool isRegistered(const void* ownerAddress) const
        {
            auto listener = std::find_if(m_listeners.cbegin(), m_listeners.cend(), [ownerAddress](const auto& listener)
            {
                return listener.ownerAddress == ownerAddress;
            });
    
            return listener != m_listeners.cend();
        }
    };
    
    template <typename Message>
    void subscribeToMessenger(const std::function<void(const Message&)>& callback, const void* ownerAddress)
    {
        GameMessenger<Message>::getInstance().subscribe(callback, ownerAddress);
    }
    
    template <typename Message>
    void unsubscribeToMessenger(const void* ownerAddress)
    {
        GameMessenger<Message>::getInstance().unsubscribe(ownerAddress);
    }
    
    template <typename Message>
    void broadcastToMessenger(const Message& message)
    {
        GameMessenger<Message>::getInstance().broadcast(message);
    }
\$\endgroup\$
3
  • 1
    \$\begingroup\$ You can make everything in GameMessenger private, and add the following: friend substribeToMessenger<>(const std::function<void(const Message&)>& callback, const void* ownerAddress); And similar friend function definitions for unsubscribeToMessenger() and broadcastToMessenger(). This will allow the out-of-class functions to access the private member fucntions. \$\endgroup\$ – G. Sliepen Dec 27 '20 at 15:08
  • \$\begingroup\$ Doesn't seem to work for some reason doing it like that. I can't access them with 'GameMessenger<Message>::getInstance().m_listeners' either. \$\endgroup\$ – Ryan Swann Dec 27 '20 at 15:28
  • 1
    \$\begingroup\$ It might be best to ask this on StackOverflow with a minimum reproducible example of what you tried. \$\endgroup\$ – G. Sliepen Dec 27 '20 at 15:37

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.