Setting up keyboard bindings using JSON (no reflection!)

Question

I got a lot of good feedback on my other question. So I will take another swing at it, post my revisions, and hopefully get some more.

First, the configuration hasn't changed. Here's the JSON file:

{
  "DisplaySettings": {
    "IsFullScreen": false,
    "PreferredBackBufferWidth": 800,
    "PreferredBackBufferHeight": 600
  },
  "GamePadSettings": {
    "Back": "Exit",
    "DPadDown": "MoveDown",
    "DPadLeft": "MoveLeft",
    "DPadRight": "MoveRight",
    "DPadUp": "MoveUp"
  },
  "KeyboardSettings": {
    "Escape": "Exit",
    "S": "MoveDown",
    "A": "MoveLeft",
    "D": "MoveRight",
    "W": "MoveUp"
  }
}

This is parsed into a Settings object:

public class Settings
{
    public DisplaySettings DisplaySettings = new DisplaySettings();
    public GamePadSettings GamePadSettings = new GamePadSettings();
    public KeyboardSettings KeyboardSettings = new KeyboardSettings();
}
public class DisplaySettings
{
    public bool IsFullScreen = false;
    public int PreferredBackBufferWidth = 800;
    public int PreferredBackBufferHeight = 600;
}
public class GamePadSettings
{
    public string Back = "Exit";
    public string DPadDown = "MoveDown";
    public string DPadLeft = "MoveLeft";
    public string DPadRight = "MoveRight";
    public string DPadUp = "MoveUp";
}
public class KeyboardSettings
{
    public string Escape = "Exit";
    public string S = "MoveDown";
    public string A = "MoveLeft";
    public string D = "MoveRight";
    public string W = "MoveUp";
}

I might still change these to properties, but public fields are simple and work well with the JSON parser. Main method of Program is largely the same, except now I pass level and player to the Input class:

public static class Program
{
    private static void Main( string[] args )
    {
        IFileHandler fileHandler = new JsonHandler();

        var settings = fileHandler.Read<Settings>( "settings.json" );
        var level = fileHandler.Read<Level>( "level.json" );
        var player = fileHandler.Read<Entity>( "player.json" );

        using ( var game = new MyGame( new Graphics( settings.DisplaySettings, level, player ),
                                       new Input( settings.GamePadSettings, settings.KeyboardSettings, level, player ) ) )
        {
            game.Run();
            fileHandler.Write( "player.json", player );
            fileHandler.Write( "level.json", level );
        }
    }
}

The reason for this change is MyGame doesn't handle the player actions directly anymore:

public class MyGame : Game
{
    private readonly IGraphics graphics;
    private readonly IInput input;

    public MyGame( IGraphics graphics, IInput input )
    {
        if ( graphics == null || input == null )
        {
            throw new ArgumentNullException();
        }

        this.graphics = graphics;
        this.input = input;

        graphics.Setup( this );
        input.Setup( this );
    }

    protected override void Update( GameTime gameTime )
    {
        input.Process();

        base.Update( gameTime );
    }

    protected override void Draw( GameTime gameTime )
    {
        graphics.Draw();

        base.Draw( gameTime );
    }
}

Input handles them instead:

public class Input : IInput
{
    private readonly GamePadSettings gamePadSettings;
    private readonly KeyboardSettings keyboardSettings;
    private readonly Level level;
    private readonly Entity player;
    private Game game;
    private IDictionary<Buttons, string> gamePadBindings;
    private IDictionary<Keys, string> keyboardBindings;
    private GamePadState gamePadState;
    private KeyboardState keyboardState;

    public Input( GamePadSettings gamePadSettings, KeyboardSettings keyboardSettings, Level level, Entity player )
    {
        if ( gamePadSettings == null || keyboardSettings == null || level == null || player == null )
        {
            throw new ArgumentNullException();
        }

        this.gamePadSettings = gamePadSettings;
        this.keyboardSettings = keyboardSettings;
        this.level = level;
        this.player = player;
    }

    public void Setup( Game game )
    {
        if ( game == null )
        {
            throw new ArgumentNullException();
        }

        this.game = game;

        SetupGamePad();
        SetupKeyboard();
    }

    public void Process()
    {
        ProcessGamePad();
        ProcessKeyboard();
    }

    private void SetupGamePad()
    {
        gamePadBindings = new Dictionary<Buttons, string>
            {
                { Buttons.Back, gamePadSettings.Back },
                { Buttons.DPadDown, gamePadSettings.DPadDown },
                { Buttons.DPadLeft, gamePadSettings.DPadLeft },
                { Buttons.DPadRight, gamePadSettings.DPadRight },
                { Buttons.DPadUp, gamePadSettings.DPadUp }
            };
    }

    private void SetupKeyboard()
    {
        keyboardBindings = new Dictionary<Keys, string>
            {
                { Keys.Escape, keyboardSettings.Escape },
                { Keys.S, keyboardSettings.S },
                { Keys.A, keyboardSettings.A },
                { Keys.D, keyboardSettings.D },
                { Keys.W, keyboardSettings.W }
            };
    }

    private void ProcessGamePad()
    {
        gamePadState = GamePad.GetState( PlayerIndex.One );

        if ( !gamePadState.IsConnected )
        {
            return;
        }

        foreach ( var binding in gamePadBindings )
        {
            if ( gamePadState.IsButtonDown( binding.Key ) )
            {
                DoAction( binding.Value );
            }
        }
    }

    private void ProcessKeyboard()
    {
        keyboardState = Keyboard.GetState( PlayerIndex.One );

        foreach ( var binding in keyboardBindings )
        {
            if ( keyboardState.IsKeyDown( binding.Key ) )
            {
                DoAction( binding.Value );
            }
        }
    }

    private void DoAction( string action )
    {
        if ( action.Equals( Actions.Exit, StringComparison.OrdinalIgnoreCase ) )
        {
            game.Exit();
            return;
        }

        if ( action.Equals( Actions.MoveDown, StringComparison.OrdinalIgnoreCase ) )
        {
            Scroll( Directions.Down );
            return;
        }

        if ( action.Equals( Actions.MoveLeft, StringComparison.OrdinalIgnoreCase ) )
        {
            Scroll( Directions.Left );
            return;
        }

        if ( action.Equals( Actions.MoveRight, StringComparison.OrdinalIgnoreCase ) )
        {
            Scroll( Directions.Right );
            return;
        }

        if ( action.Equals( Actions.MoveUp, StringComparison.OrdinalIgnoreCase ) )
        {
            Scroll( Directions.Up );
            return;
        }
    }

    private void Scroll( Vector2 direction )
    {
        // TODO: Collision detection

        player.Turn( direction );
        player.Animate();

        level.Move( direction * -1 );
    }
}

Before, I was using reflection to create a dictionary of bindings. This seemed pretty elegant, but had several problems. What I lose in elegance though I more than gain (in my humble opinion) in simplicity. i.e., the DoAction method is very easy to understand. In addition, I no longer have to worry about accidentally exposing a method the user can access via configuration, I don't have a bunch of methods that ReSharper can't find usages of, and performance should be improved.

So what do you think? Is this better than my previous attempt? What else can be improved?

Answer 1

I guess one minor thing is I might suggest refactoring DoAction to something like below.

Whether that helps improve the code I guess is a matter of opinion. To me though it does appear that you can only do one action at a time so having multiple if statements does not seem plausible to me.

To avoid the need to ToUpperInvariant on the Actions I would consider having them uppercase already.

public void DoAction(string action)
{
    // Or better move this to a class level instance variable to avoid re-creating
    // every time
    Dictionary<string, Action> actionHandler = new Dictionary<string, Action>
    {
        {Actions.Exit.ToUpperInvariant(),     () => game.Exit() },
        {Actions.MoveDown.ToUpperInvariant(), () => Scroll(Directions.Down) },
        {Actions.MoveUp.ToUpperInvariant(),   () => Scroll(Directions.Up) },
        {Actions.MoveLeft.ToUpperInvariant(), () => Scroll(Directions.Left) },
        {Actions.MoveRight.ToUpperInvariant(),() => Scroll(Directions.Right) },
    };

    actionHandler[action.ToUpperInvariant()]();
}

Or if Dictionary is overkill, I would consider using a switch:

switch(action.ToUpperInvariant()) 
{
    case Actions.Exit.ToUpperInvariant():
        game.Exit();
        break;
    case Actions.MoveDown.ToUpperInvariant():
        Scroll(Directions.Down);
        break;
    case Actions.MoveMoveUp.ToUpperInvariant():
        Scroll(Directions.Up);
        break;
    case Actions.MoveLeft.ToUpperInvariant():
        Scroll(Directions.Left);
        break;
    case Actions.MoveRight.ToUpperInvariant():
        Scroll(Directions.Right);
        break;
}

NOTE: Using ToUpper() may not be the fully correct way as discussed here but I think from the OP solution it would probably be sufficient.

Answer 2

There are two design issues I can see:

• Game class stands for two things, game as a whole and game logic component.

I don't understand this part.

When I say "game-as-whole," I mean it is a top level component that has an aggregation (HAS-A) relationship with subcomponents. When I read Program and MyGame:

public class MyGame : Game
{
    private readonly IGraphics graphics;
    private readonly IInput input;

This class is, or is like, the aggregate root of all the components.

Since I'm using XNA, I don't think I can get around the requirement for MyGame to extend XNA's Game class and to implement Update and Draw methods. Where else would I do this?

They could stay in MyGame. But where would, for example, the code that determines whether is over (an example of game logic) go?

I try to hand those tasks off as best I can by having Input handle key presses (for example), but then Input is doing game logic - why is this bad?

If you separate your components at an inappropriate boundary their surfaces (the sum of the public interface[s] they expose and the public interface[s] of the objects they depend on) become too large. More concretely, since Input is the component that knows about player and level any collision detection method (an example of game logic) would want to be there. If you put them there, other methods that depend on collision (e.g. score keeping) may want to go there, and so on. The name Input implies no such intention.

And what exactly do you mean by "game logic" anyways?

When I say "game logic component," I mean the top level object that encapsulates objects that are related to the "model" part of the software, such as player, level and methods that act on them such as DoAction.

The best analogy I can think of now is a desktop computer:

class Program
{
    void Main()
    {
        var motherboard = new MotherBoard();
        var networkCard = new NetworkCard();
        var graphicsCard = new GraphicsCard();
        var computer = assembleInACase( motherboard, networkCard, graphicsCard );

        computer.pressStartButton();
    }

    IComputer assembleInACase( MotherBoard motherboard, NetworkCard networkCard, GraphicsCard graphicsCard )
    {
        IComputer computer = new ComputerCase( motherboard, networkCard, graphicsCard );

        // Wire up cards
        networkCard.pciExpressEvent += motherboard.pciExpressEventHandler;
        graphicsCard.agpEvent += motherboard.agpEventHandler;

        return computer;
    }
}

In the above example ComputerCase:IComputer is analogous to "game-as-whole" and MotherBoard is analogous to "game logic." Frameworks like XNA are infrastructure, and you should isolate them from the game logic. MyGame:Game is analogous to ComputerCase:AC110_220VDevice or ComputerCase:DC12VDevice. The motherboard doesn't need to know about the power inlet, but the computer as a whole does.

• Input does game logic, which is partly caused by the first.

From the snippet below:

public class Input : IInput
{
    private readonly GamePadSettings gamePadSettings;
    private readonly KeyboardSettings keyboardSettings;
    private readonly Level level;
    private readonly Entity player;
    private Game game;
    private IDictionary<Buttons, string> gamePadBindings;
    private IDictionary<Keys, string> keyboardBindings;
    private GamePadState gamePadState;
    private KeyboardState keyboardState;

Input knows about level and player, which is a case of inappropriate intimacy. [EDIT: Because what Input class does with them is not related strictly to input. These fields are internals of MyGame class. A class named Input should not have to change because I decide to use LevelV2 instead of Level, Some.Other.Namespace.Level, or no Level at all. Inappropriate intimacy is a violation of the single responsibility principle. In this particular case Input is an adapter. It separates game logic, which only needs to know users' intended actions (move left, move up), from actual keystrokes. In the code from the original post however we see it knows too much about MyGame, therefore it doesn't separate game logic as well as expected. An ideal separator has very low coupling with both components it separates.]

Input depends on Game, and even if this would not automatically mean a circular dependency if Game were an interface, it is a violation of the interface segregation principle. Instead an input class would depend on a specific interface (e.g. IInputHandler) and Game class would implement it in addition to other interfaces it exposes to other components.

In the code provided gamePadState and keyboardState fields are not used, and it seems they would not need to be used in the code as a whole. They could be local variables.

gamePadSettings and keyboardSettings are not used after setup (object creation time). Since they are not used and Input depends on gamePadBindings and keyboardBindings during its lifetime it should depend on them only (get them as constructor args).

In the method private void DoAction( string action ), action parameter can only take a value from a fixed set: {"Exit", "MoveDown", ...}. This is a case of primitive obsession. This concept can better be implemented as an enum : enum Actions {Exit, MoveDown, ...}.

You can now decouple Game and Input by declaring an event handler:

public enum Actions {Exit, MoveLeft, ...}

public delegate void ActionHandler( Actions action );

public interface IInput {

    // Change the name of the event to something that makes sense to you
    event ActionHandler ActionRequested;

    // Setup method can be removed

DoAction can then be moved to another class, making Input follow the single responsibility principle more closely.

This class now encapsulates user input and converts them to game actions and does nothing else:

public class Input : IInput
{
    // See how much smaller the surface is now
    public event ActionHandler ActionRequested;

    private IDictionary<Buttons, Actions> gamePadBindings;
    private IDictionary<Keys, Actions> keyboardBindings;

    public Input( IDictionary<Buttons, Actions> gamePadBindings,
                  IDictionary<Keys, Actions> keyboardBindings )
    {
        // ...
    }

    // This does one thing, although Process is too generic a name
    public void Process()
    {
        ProcessGamePad();
        ProcessKeyboard();
    }

    private void ProcessGamePad()
    {
        var actions = GetPressedButtons().Select( button => GetActionFor(button) );
        foreach ( var action in actions )
            ActionRequested( action );
    }

    private void ProcessKeyboard()
    {
        // As above...
    }
}

This class encapsulates game logic. It does not depend on infrastructure classes as much as possible:

// Rename me
class GameLogic
{
    // Relieve Input from the burden of these
    private readonly Level level;
    private readonly Entity player;

    public void DoAction( Actions action )
    {
        // ...
    }

    // This is now a good place to hang new behavior
    // Test whether game ended, keep scores, etc.
}

This class encapsulates subcomponents and enables separation of infrastructure and game logic:

class MyGame : Game
{
    private readonly IGraphics graphics;
    private readonly IInput input;

    public MyGame( IGraphics graphics, IInput input )
    {
        // Disallowing null dependencies makes testing code harder
        // Let's not put it until ABSOLUTELY necessary

        this.graphics = graphics;
        this.input = input;

        // Nothing other than field assignment
        // graphics.Setup() etc. belongs to a Factory or Main method
    }

    protected override void Update( GameTime gameTime )
    {
        // No change
    }

    protected override void Draw( GameTime gameTime )
    {
        // No change
    }
}

By moving the setup code to Program, it enables further refactorings and extraction of factory methods/classes:

class Program
{
    void Main()
    {
        IFileHandler fileHandler = new JsonHandler();

        var settings = fileHandler.Read<Settings>( "settings.json" );

        var level = fileHandler.Read<Level>( "level.json" );
        var player = fileHandler.Read<Entity>( "player.json" );

        // Move SetupGamePad method to a public method and rename it
        // so it can be tested and also keeps Input class more focused.
        var gamePadBindings = CreateGamePadBindings( settings.GamePadSettings );

        // Do the same for SetupKeyboard method
        // var keyboardBindings = ...

        var inputHandler = new Input( gamePadBindings, keyboardBindings );

        using ( var game = new MyGame( new Graphics(...), inputHandler ) )
        {
            // TODO game set up: move to a method

            // Taking the wiring out of constructors keeps constructors super simple,
            // makes testing easier and enables us to extract Factory classes as needed.
            inputHandler.ActionRequested += gameLogic.DoAction;

            // Also by using an event we broke the circular dependency
            // The same can be done from the graphics below
            graphics.Setup( game )

            game.Run();

            // TODO game tear down: move to a method
        }
    }
}

EDIT

Should GameLogic have a dependency on Game? When the escape key is pressed, I need to call Game.Exit(), which would be in DoAction().

As I said, I wouldn't make model depend on infrastructure types as much as possible. If pressing Esc is an action in your game then your input handler (Input) would generate an action event, and your model (GameLogic) would then generate some GameEnded event after doing something with the model (e.g. save&exit) MyGame which listens to events would call Exit().

I can't instantiate Graphics without an instance of Game. This is a limitation of the framework and the reason graphics.Setup( this ) was called in the Game constructor.

It should be called, as I did in the example, by the method that calls the new MyGame() constructor. If they should always be together, you can put the two calls in a factory method such as createMyGame() in Program.

Calling graphics.Setup(this) on is bad OOP practice, because:

1. It leaks this
2. You are doing more than field assignment in constructor.
3. Disallows passing null as graphics parameter to the constructor. (Google this one yourself.) That you cannot test pressing Esc causes Game.Exit method to be called without instantiating a graphics object indicates your code isn't modular enough.