5
\$\begingroup\$

I've created an SDL input manager for my 3D OpenGL applications. It's not finished yet, but I would like to know if I'm doing it the right way.

I really tried to optimise and compact my code, as it is for an OpenGL library.

InputHandler.h

#pragma once
#include <SDL2/SDL.h>
#include <vector>
#include <glm/glm.hpp>
#include "SDL_Handle.h"

#define TheInputHandler InputHandler::Instance() //Yes... Macros aren't good... but we can't ude typdef, or anything else :)

class InputHandler
{
    public:
        static InputHandler& Instance(){
            static InputHandler instance;
            return instance;
        }

        void Update();
        inline bool ExitRequested(){ return _hasExit; }
        inline glm::uvec2 GetMousePosition(){ return _mousePosition; }
        inline bool KeyDown(SDL_Scancode key){
            return _keyStates != 0 ? _keyStates[key] : false;
        }

        ///disable assignemant operators
        InputHandler(InputHandler const&) = delete;
        InputHandler& operator=(InputHandler const&) = delete;
    private:
        InputHandler();/// singleton pattern
        virtual ~InputHandler();
        SDL_Handle _handle;
        bool _hasExit = false;
        glm::uvec2 _mousePosition;
        const Uint8* _keyStates = nullptr;
};

InputHandler.cpp

InputHandler::InputHandler(){
    _handle = SDL_Handle(); /// init SDL : uses RAII to avoid initializing twice !
}
InputHandler::~InputHandler(){}

void InputHandler::Update(){
    _keyStates = SDL_GetKeyboardState(0);
    SDL_Event event;
    while(SDL_PollEvent(&event))
    {
        switch(event.type)
        {
            case SDL_QUIT:
                _hasExit = true;
                break;
            case SDL_MOUSEMOTION:
                _mousePosition = glm::uvec2(event.motion.x, event.motion.y);
                break;
        }
    }
}
\$\endgroup\$
4
\$\begingroup\$

Right so you're using a Singleton pattern because you find it reasonable that there will only ever be one input handler. You correctly implement the singleton pattern in C++, which is good. The bad thing is that the pattern in of itself is bad.

A better design is to make InputHandler an interface with virtual functions. Then create a concrete instance SDLInputHandler which implements InputHandler. Then, wherever you need to parse inputs you pass a pointer to a InputHandler. In your main class you can simply have an instance of SDLInputHandler which you pass pointers to.

An added benefit of this is that you can implement an InputHandler that can generate a fixed sequence of inputs, this is very useful for testing your code or for playing back a sequence of user inputs, like a recorded game for example.

If you at this point are thinking:

No way, Jose! I'm not going to be passing that pointer all over my application!

Then that indicates a bigger issue you're having in your overall design. The parts of your application that are directly dealing with input should be quite limited and small. If you indeed are having this phenomena, you should really look into the Command Pattern. It will make your life easier.

edit

If I'm not mistaken, all names that start with an underscore are reserved for the implementation and all your member variable names are technically not allowed.

\$\endgroup\$
  • \$\begingroup\$ Actually, I could pass it around by ref. because I'm using a game state managment system, but I'm creating a library, so I want to give the user the ability to use this class anywhere, outside of the gamstate managment : by the way, the assimp library's log manager is a singleton... Singleton's are bad, but one can't harm :) \$\endgroup\$ – MattMatt Jun 18 '15 at 12:50
  • \$\begingroup\$ 2) also, is it problematic that I used a #def ? I know that it is not advised to use them in modern C++... \$\endgroup\$ – MattMatt Jun 18 '15 at 12:51
  • 1
    \$\begingroup\$ Names starting with _ are only reserved in all scopes if the _ is followed by an uppercase letter. I usually refer to this thread. But yes, personally I'd also not recommend using the underscore prefix because of the many not well known rules and caveats about them. \$\endgroup\$ – glampert Jun 19 '15 at 20:54
  • 1
    \$\begingroup\$ @MattMatt, this is a very frequently unknown rule, mainly because that naming notation is just "reserved for future expansion" by library and compiler, so compilers can't really output an error if you use it. Then most programmers are not aware of it, but is is a practice that makes your code less portable and less robust against future library/language versions. \$\endgroup\$ – glampert Jun 21 '15 at 18:14
  • 1
    \$\begingroup\$ @MattMatt your code would be slightly non-conformant. You risk getting a name collision with something in some standard library implementation now or in a future version of the tool-chain you're using. At best you get compile errors, at worst you're silently shadowing. But the bottom line is: If you run into any problem from this, it is your fault and not a fault of the tool-chain you're using; so you cannot file any bugs on it. And people will remark on it when you put your code up on CodeReview ;) \$\endgroup\$ – Emily L. Jun 23 '15 at 9:51
3
\$\begingroup\$

The most important thing about the singleton pattern is to learn when NOT to use it.

Maybe you only need one input manager in your application. Perhaps the one-instance requirement is justifiable. But think about this: suppose you were to add input support for a gamepad/controller. With this architecture, everything would have to go inside InputHandler. Mouse handling, keyboard, gampepad. It would become a massive singleton that tries to handle every possible input device combination. Rather, if you had other classes, such as GamePadInput and MouseKeyboardInput your code would be much better organized and easier to maintain.

Before settling with a all-in-one global point of access, consider other approaches first. Read the link above. @EmilyL's answer also covers that in good detail.


The global point of access:

I've seen this used many times in combination with the lazy-initialization singleton:

#define TheInputHandler InputHandler::Instance()

I personally don't like it. Not because of the macro usage per-se, but because it obfuscates the code. When I call TheInputHandler it will seem like I'm referencing a global variable. However, if I have to track down the declaration, surprise, surprise! It is actually calling a function.

I like code to be as explicit as possible. From my experience, it makes for more robust code. If sticking to a singleton, I would not provide that macro. Don't lie to users of your class. Let them know they are calling into a singleton class.

Other options include: making every method and data static, to avoid the need for a Instance() function, or declaring TheInputHandler as an actual global variable.


Two superfluous things in your class:

  • inline for methods that already declared inline (no pun intended) inside the class body in the header file. In this case they keyword adds nothing and may be removed.

  • A virtual private destructor for a class that is not inherited from. The very fact that the destructor is private will ensure that no instance of the class is ever directly deleted, so even if you were to inherit from it, the virtual destructor would still be unnecessary. Also see When to use virtual destructors? on SO.

\$\endgroup\$
  • \$\begingroup\$ Thank you :) But the problem is that sdl does not like more then one input handler. In fact it crashes input receiving \$\endgroup\$ – MattMatt Jun 19 '15 at 9:38
  • 1
    \$\begingroup\$ @mattmatt the user creating more than one InputManager is a user error. You do not need to go to excessive length to prevent this. Just make sure you fail early and clearly if it happens. \$\endgroup\$ – Emily L. Jun 19 '15 at 19:08
  • \$\begingroup\$ Maybe I should have a static variable, counting the number of instances, and then, I could throw an exception ! \$\endgroup\$ – MattMatt Jun 21 '15 at 11:44
  • \$\begingroup\$ @MattMatt, Yep, that's a good approach. You could also make _handle static and check that it is null before assigning a new value. \$\endgroup\$ – glampert Jun 21 '15 at 18:09

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.