HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

XNA KeyboardStateManager

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
xnakeyboardstatemanagerstackoverflow

Problem

This class allows for easy management of the KeyboardState of an XNA game.

It provides a simple, easy-to-use API for interacting with the Keyboard and getting key-presses, via a few (trivial) events.

As far as I can test, it works as intended.

GitHub Link (for future reference) as of this version: KeyboardStateManager.

```
///
/// This provides an easy-to-use class to interact with the Keyboard in an XNA game.
///
public class KeyboardStateManager
{
private KeyboardState _kPrev;
private KeyboardState _kNow;

///
/// Gets the KeyboardState from the previous update.
///
public KeyboardState KeyStatePrevious { get { return _kPrev; } }
///
/// Gets the KeyboardState from the current update.
///
public KeyboardState KeyStateNow { get { return _kNow; } }

///
/// Updates the internal KeyboardState and fires relevant events.
///
/// The new KeyboardState.
public void Update(KeyboardState kState)
{
_kPrev = _kNow;
_kNow = kState;

List keysDownNow = GetPressedKeys(_kNow);
List keysDownPrev = GetPressedKeys(_kPrev);
List keysPressed = GetPressedKeys();

foreach (Evbpc.Framework.Windows.Forms.Keys keyDownNow in keysDownNow)
if (!keysDownPrev.Contains(keyDownNow))
OnKeyDown(new KeyEventArgs(keyDownNow));

foreach (Evbpc.Framework.Windows.Forms.Keys keyDownPrev in keysDownPrev)
if (!keysDownNow.Contains(keyDownPrev))
OnKeyUp(new KeyEventArgs(keyDownPrev));

foreach (Evbpc.Framework.Windows.Forms.Keys pressedKey in keysPressed)
OnKeyPress(new KeyPressEventArgs(GetKeyChar(pressedKey)));
}

///
/// Gets the char value represented by the that was sent.
///
/// The enumeration value to examine.
/// A char that represents the ASCII value of the key.
public static char GetKeyChar(Evbpc.Framework.Windows.Forms.Keys key)
{
if ((key & E

Solution

Use foreach when you don't need indexes

In the main loop in GetPressedKeys,
it seems to me you don't need the index variable i,
and instead of this:

for (int i = 0; i < pressedKeys.Count; i++)
    {


You could rewrite the loop using foreach:

foreach (Framework.Windows.Forms.Keys keys in pressedKeys)
    {


You can do likewise in IsKeyDown and IsKeyUp too.

Avoid duplicated logic

The implementations of IsKeyDown and IsKeyUp look very very similar.
You could avoid duplicating logic by generalizing:

private static bool IsPressedState(List pressedKeys, Framework.Windows.Forms.Keys key, bool state)
{
    for (int i = 0; i  pressedKeys, Framework.Windows.Forms.Keys key)
{
    return IsPressedState(pressedKeys, key, true);
}

private static bool IsKeyUp(List pressedKeys, Framework.Windows.Forms.Keys key)
{
    return IsPressedState(pressedKeys, key, false);
}


Avoid too similar variable names

While it's admirable that you used perfectly self-descriptive variable names here,
it's really easy to get lost in the references to keyDownNow, keysDownNow, keyDownPrev, keysDownPrev:

foreach (Evbpc.Framework.Windows.Forms.Keys keyDownNow in keysDownNow)
        if (!keysDownPrev.Contains(keyDownNow))
            OnKeyDown(new KeyEventArgs(keyDownNow));

    foreach (Evbpc.Framework.Windows.Forms.Keys keyDownPrev in keysDownPrev)
        if (!keysDownNow.Contains(keyDownPrev))
            OnKeyUp(new KeyEventArgs(keyDownPrev));


I suggest to simplify the loop variables (the singular names, keyDownNow and keyDownPrev) to simply key.

Simplify to ^= and |=

Instead of this:

key = key ^ Framework.Windows.Forms.Keys.Shift;


I believe you can write more compactly as:

key ^= Framework.Windows.Forms.Keys.Shift;


Likewise, instead of this:

result[i] = result[i] | Framework.Windows.Forms.Keys.Shift;


You could write:

result[i] |= Framework.Windows.Forms.Keys.Shift;


Reduce nesting

When a brach in a conditional returns,
you can reduce nesting by removing else if and else statements.
For example instead of this:

if ((int)key >= 0x41 && (int)key = 0x30 && (int)key <= 0x39)
            return (char)key;
        else
        {
            switch (key)
            {
                // ...
            }


You can write like this, especially notice the reduced indentation of the switch:

if ((int)key >= 0x41 && (int)key = 0x30 && (int)key <= 0x39)
            return (char)key;

        switch (key)
        {
            // ...
        }

Code Snippets

for (int i = 0; i < pressedKeys.Count; i++)
    {
foreach (Framework.Windows.Forms.Keys keys in pressedKeys)
    {
private static bool IsPressedState(List<Framework.Windows.Forms.Keys> pressedKeys, Framework.Windows.Forms.Keys key, bool state)
{
    for (int i = 0; i < pressedKeys.Count; i++)
        if (pressedKeys[i] == key)
            return state;

    return !state;
}

private static bool IsKeyDown(List<Framework.Windows.Forms.Keys> pressedKeys, Framework.Windows.Forms.Keys key)
{
    return IsPressedState(pressedKeys, key, true);
}

private static bool IsKeyUp(List<Framework.Windows.Forms.Keys> pressedKeys, Framework.Windows.Forms.Keys key)
{
    return IsPressedState(pressedKeys, key, false);
}
foreach (Evbpc.Framework.Windows.Forms.Keys keyDownNow in keysDownNow)
        if (!keysDownPrev.Contains(keyDownNow))
            OnKeyDown(new KeyEventArgs(keyDownNow));

    foreach (Evbpc.Framework.Windows.Forms.Keys keyDownPrev in keysDownPrev)
        if (!keysDownNow.Contains(keyDownPrev))
            OnKeyUp(new KeyEventArgs(keyDownPrev));
key = key ^ Framework.Windows.Forms.Keys.Shift;

Context

StackExchange Code Review Q#101303, answer score: 7

Revisions (0)

No revisions yet.