Input handling system using the command pattern

The online book Game Programming Patterns briefly describes the use of the command pattern to handle input in a game. I've attempted to write my own "one size fits all" input handling system based on the command pattern, as shown below.

• Is this a correct implementation and use of the command pattern?

• Would it be "better" (more efficient, more maintainable) to use another pattern (e.g: observer pattern)?

• How do I adapt it to handle input from multiple controllers?

input_handler.hpp

```
#ifndef INPUT_HANDLER_HPP
#define INPUT_HANDLER_HPP

#include
#include
#include
#include "character.hpp"
#include "input_constants.hpp"

class Command
{
public:
virtual ~Command() {}
virtual void execute(Character *character) = 0;
virtual InputType get_input_type() = 0;
};

class InputHandler
{
private:
// Pointers to all commands
Command *move_up;
Command *move_down;
Command *move_left;
Command *move_right;
Command *jump;

std::map commands;

// Gameplay context
std::map state_map;
std::map action_map;

bool input_mapping();
void dispatcher(std::vector &command_queue);

void keydown(SDL_Event &event);
void keyup(SDL_Event &event);

bool is_held(int key);
bool was_pressed(int key);

public:
InputHandler();
~InputHandler();
bool fill(std::vector &command_queue);
void configure(int key, Command *command);
};

class MoveUp : public Command
{
public:
void execute(Character *character) { character->move_up(); }
InputType get_input_type() { return STATE; }
};

class MoveLeft : public Command
{
public:
void execute(Character *character) { character->move_left(); }
InputType get_input_type() { return STATE; }
};

class MoveRight : public Command
{
public:
void execute(Character *character) { character->move_right(); }
InputTy

Is this a correct implementation and use of the command pattern?

Yes, I think it is good enough.


Would it be better (more efficient, more maintainable) to use another pattern?

Performance wise it is hard to tell if other solutions would be faster.
This one is quite maintainable and scalable, but I am biased in this case, since I
really appreciate the Command pattern.

One performance optimization that you might do in the future if to get rid of the
std::maps that you are using right now. You could very well use arrays, since your
map keys are just integer constants. If you take to trouble of ensuring that the enum
constants are sequential, you can replace:

std::map commands;
std::map    state_map;
std::map   action_map;

With plain arrays:

Command* commands[MAX_COMMAND_INDEX];
State    state_map[MAX_STATE_INDEX];
Action   action_map[MAX_ACTION_INDEX];

Or better still, with a C++11 array:

std::array commands;
std::array state_map;
std::array action_map;

General improvements:

You could use smart pointers to enforce a safer object ownership policy.
The C++11 std::shared_ptr would be a good choice:

typedef std::shared_ptr CommandPtr;

This ensures a Command never gets destroyed when it is passed around and frees you from
the burden of manually deleting them in the destructor of InputHandler.

In fact, you should use smart pointers for most if not all objects, this includes the
Character pointers:

typedef std::shared_ptr CharacterPtr;

So that in virtual void execute(CharacterPtr character) of Command the character pointer is always valid while to function is being executed, even if the original pointer's referenced is lost in some other thread.

Better naming:

The public function:

void configure(int key, Command *command);

of InputHandler is responsible for the binding of a given key to a command, thus,
a better name would be bind:

void bind(int key, CommandPtr command);

fill is also a vague name for a function that is responsible for generating the
input commands for a game frame:

bool fill(std::vector &command_queue);

I would recommend renaming it to:

bool generate_input(std::vector &command_queue);

or even more explicit:

bool generate_input_commands(std::vector &command_queue);

Don't be afraid of using long names. The less margin you leave for ambiguity, the better.

bool input_mapping();

Is also a somewhat poor name. Take the description commend you've written on its call
site and try to come-up with a very descriptive name of what the function does:

// converts raw input datum to an action and/or state

You could rename it to:

bool convert_inputs_to_actions();

bool map_inputs_to_actions();

bool do_input_to_action_mapping();

And the list goes on.

void dispatcher(std::vector &command_queue);

This one actually fills the command queue. The name you've chosen is good for a type
(E.g.: A Dispatcher class) but not quite proper for a function.
You could rename it to:

void dispatch_commands(std::vector &command_queue);

Or more simply:

void fill_command_queue(std::vector &command_queue);

Mind your style of control flow layout:

This one might be disregarded as personal taste, but I think it is worth mentioning.

I'd like to recommend that you mind your style of placing the return statements on
the same line of an if clause. For example, in this block:

while (SDL_PollEvent(&event))
    if (event.type == SDL_QUIT) return true;
    else if (event.type == SDL_KEYDOWN) {
        if (event.key.keysym.sym == SDLK_ESCAPE) return true;
        keydown(event);
    }
    else if (event.type == SDL_KEYUP)
        keyup(event);

I find the mixture of styles and bracing hard to read.
It would be a lot easier on the eyes if you were to reformat as:

while (SDL_PollEvent(&event))
    if (event.type == SDL_QUIT)
        return true;
    else if (event.type == SDL_KEYDOWN) {
        if (event.key.keysym.sym == SDLK_ESCAPE)
            return true;
        keydown(event);
    }
    else if (event.type == SDL_KEYUP)
        keyup(event);

But even better and safer if you added uniform bracing on all statements,
even in the single line ones:

while (SDL_PollEvent(&event)) {
    if (event.type == SDL_QUIT) {
        return true;
    } else if (event.type == SDL_KEYDOWN) {
        if (event.key.keysym.sym == SDLK_ESCAPE) {
            return true;
        }
        keydown(event);
    } else if (event.type == SDL_KEYUP) {
        keyup(event);
    }
}

With uniform bracing, if you need to add more than one command to an if result
there is no risk of doing something stupid like:

if (x == 42)
    do_something();
    do_some_other_thing();

The correct indentation fools you into thinking that both lines belong to the result
of the first if. If the programmer had bracing form the beginning, he would be immune to
this sort or erro

StackExchange Code Review Q#55365, answer score: 9