Keyboard input class

A little bit of background

There are two basic device input models that most applications use. Games will most certainly use one of these two or both. I'll briefly elaborate on them to give you some ideas.

Polled input:

Your application constantly queries the system/device to check if new input has occurred. Usually the devices have a buffer/queue of the recent input that accumulate until your program checks them. In such a setup, you would have functions like isKeyDown(Key) and isKeyUp(key) that your applications calls whenever it needs to receive some input. For this approach, it doesn't really matter much if you tie keys to modifiers, since you can simply check for both the key and modifier at the same time. Assume we have a fictional Player on a first person game where we move forward with W and begin to run with Shift:

if (isKeyDown(Key::W) && isKeyDown(Key::Shift))
{
    player.setState(Player::Running);
}

This approach is very straightforward and easy to implement. This is more often then not the approach used in video games because of its simplicity.

Event or interrupt based input:

In this setup, the application registers some callback with the system/device that will be fired whenever a new input arrives. There are variants of this when the application might need to call some library function to generate the events, e.g.: SDL_PollEvent(), this is a sort of hybrid between polled and event-based input.

Your approach is clearly event based, since the users of Input register callback functions that get run whenever you have new input. For this approach, having the key modifier makes more sense, since each event can only refer to one key at a time. Taking the W/Shift example from before, it will either fire a W or a Shift event callback in sequence, but it can't fire both callbacks simultaneously, so you need the modifier to tell if Shift was down at the same time of W.

This approach is more complicated and takes more code to set up. The advantage is being able to remap keys to callbacks very easily, even at runtime if needed be. Though you can also do that with the simpler polled input, only you have to approach the problem at a higher level instead (I.e. perform the mapping inside the Player class).

Okay, enough chat, lets take a look at your current implementation and see what can be objectively improved. Points roughly in order of appearance in the code:

• Don't use identifiers in your code containing double underscore (like __INPUT_H__), that naming convention is reserved for compiler and standard library uses.

• Prefer using PascalCase for types and camelCase for variable and function names, to distinguish both kinds of things. It would be better to name key as Key, with the big K, so you can for example declare: Key key = ....

• Key struct doesn't need a copy constructor. It has only Plain Old Data (POD) members, so the compiler will provide a default bit-wise copy constructor and assignment operator.

• Always initialize your member data in the constructor's initializer list. There is no difference in the generated code for native/POD types, but it will avoid redundant work for complex objects. If you init a member string, for instance, with the assignment operator, it will first go thru the default constructor to then re-assign it in the sequence. Keep your code uniform and always use the initializer list to avoid duplicate work when dealing with objects.

  Key() 
      : code(SDL_SCANCODE_UNKNOWN)
      , kmod(KMOD_NON)
  { }
  

• operator < in Key is arguable. I don't recall even comparing a key for ordering. However, I believe you added this to make it compatible with the standard map, so in that case it is understandable. Another option might be providing the map with a comparator functor/struct that you can make private, so Key doesn't expose the unexpected operator.

• For both Key operators, using std::tie seems to only obfuscate the code. I actually had to lookup in the reference to remember what std::tie was for. This isn't a function used often I'm afraid, so most programmers are likely unfamiliar with it. In this case, since you only deal with two variables, using plain comparisons would be much better, in my opinion:

  bool operator == (const key& rhs) const 
  {
      return code == rhs.code && kmod == rhs.kmod;
  }
  // Code gets shorter too!
  

• I find it better to place the public interface of a class first in the header file, to give it more emphasis. That's usually the part you and users of your code will be looking at more often. protected and private are implementation details, so shouldn't need to stand out as much.

• NUM_KEYCODES is not a "real" constant, but just a const member integer, so all instances of Input will have an instance of that variable. To make it a true compile-time constant, declare it as static and initialize it inline:

  // inside class Input 
  static const int NUM_KEYCODES = 256;
  

  Note that you can (and arguably should) use constexpr if your target is C++11 and above.

• No need to HeldKeys.clear() in the constructor. The vector is initially empty!

• return at then of a void function is unnecessary and only makes for more verbosity.

• The necessity of HeldKeys is not quite clear to me. Aren't the key maps enough? If you could get rid of it, it would save you some memory and extra indirections.

• Use auto to cleanup things like std::map<key,std::function<void()>>::iterator, or at lest typedef std::map<key,std::function<void()>> if you can't use C++11.

I'm running out of time now, so I can't dig further into your implementation and architecture, so I'll leave you with a reference implementation you can look at to get more ideas from. Take a look at the Key Input class used by the Doom 3 game. It is somewhat similar to yours, but uses a polling approach instead.