4
\$\begingroup\$

I created a class to prevent the default delay which happens when holding down the key on Windows Forms. It seems like the class is working correctly but I don't know if I'm using the best practice.

KeyboardController class;

class KeyboardController
{
    public event EventHandler kbEvent;
    private DispatcherTimer timer;
    private HashSet<Key> keyPressed;

    public KeyboardController(Window c, EventHandler function)
    {
        c.KeyDown += WinKeyDown;
        c.KeyUp += WinKeyUp;
        kbEvent = function;
        keyPressed = new HashSet<Key>();

        timer = new DispatcherTimer();
        timer.Tick += new EventHandler(kbTimer_Tick);
        timer.Interval = new TimeSpan(0, 0, 0, 0, 10);
        timer.Start();
    }

    public bool KeyDown(Key key)
    {
        if (keyPressed.Contains(key))
        {
            return true;
        }
        else
        {
            return false;
        }
    }

    private void WinKeyDown(object sender, KeyEventArgs e)
    {
        keyPressed.Add(e.Key);
    }

    private void WinKeyUp(object sender, KeyEventArgs e)
    {
        keyPressed.Remove(e.Key);
    }

    private void kbTimer_Tick(object sender, EventArgs e)
    {
        kbEvent(null, null);
    }
}

MainWindow.xaml.cs;

public MainWindow()
{
    EventHandler eh = new EventHandler(KeyboardFunction);
    kbc = new KeyboardController(this, eh);
}

private void KeyboardFunction(object sender, EventArgs e)
{
    if (kbc.KeyDown(Key.Right))
    {
        // do something.
    }
    else
    {
        // do something.
    }
}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$
  1. The main issue is that the key event handler and the timer tick will run on different threads which means that someone could press a key while the timer tick fires - in which case the HashSet could be modified while you are trying to read it from another thread. You will need to safeguard against that by protecting the access to the HashSet with a lock You can probably get away with it because the DispatcherTimer runs on the UI thread where the KeyDown/KeyUp events are also being raised.

  2. The idiomatic way of subscribing to event handler is to let the user subscribe to the event rather than passing the event handler via the constructor. What happens if the user has shorter lifetime than the event provider? In your case it would keep it alive because you can't unsubscribe.

  3. When raising the event you need to check if the vent handler is null - subscriptions to an event are considered optional which means no subscriber might exist. Also you should not pass null as sender or EventArgs. The idiomatic pattern in C# goes like this:

    private void kbTimer_Tick(object sender, EventArgs e)
    {
        var handler = kbEvent;
        if (handler != null)
            handler(this, EventArgs.Empty);
    }
    
  4. The KeyDown method can be reduced to 1 line of code:

    return keyPressed.Contains(key);
    
  5. The name for the event is a bit non-descriptive. How about keyboardTick?

  6. Standard naming convention in C# for public fields/properties/methods is PascalCase (so the event should be KeyboardTick).

In summary the final code could look like this:

class KeyboardController
{
    public event EventHandler KeyboardTick;
    private DispatcherTimer timer;
    private HashSet<Key> pressedKeys;
    private readonly object pressedKeysLock = new object();

    public KeyboardController(Window c)
    {
        c.KeyDown += WinKeyDown;
        c.KeyUp += WinKeyUp;
        pressedKeys = new HashSet<Key>();

        timer = new DispatcherTimer();
        timer.Tick += kbTimer_Tick;
        timer.Interval = new TimeSpan(0, 0, 0, 0, 10);
        timer.Start();
    }

    public bool KeyDown(Key key)
    {
        lock (pressedKeysLock)
        {
            return pressedKeys.Contains(key);
        }
    }

    private void WinKeyDown(object sender, KeyEventArgs e)
    {
        lock (pressedKeysLock)
        {
            keyPressed.Add(e.Key);
        }
    }

    private void WinKeyUp(object sender, KeyEventArgs e)
    {
        lock (pressedKeysLock)
        {
            keyPressed.Remove(e.Key);
        }
    }

    private void kbTimer_Tick(object sender, EventArgs e)
    {
        var handler = keyboardTick;
        if (handler != null)
        {
            handler(this, EventArgs.Empty);
        }
    }
}

and the usage:

public MainWindow()
{
    kbc = new KeyboardController(this);
    kbc.KeyboardTick += HandleKeyboardTick;
}

private void HandleKeyboardTick(object sender, EventArgs e)
{
    if (kbc.KeyDown(Key.Right))
    {
        // do something.
    }
    else
    {
        // do something.
    }
}
\$\endgroup\$
4
  • \$\begingroup\$ How do we know they run on different threads? All events run on different threads? \$\endgroup\$
    – ctulu
    Mar 15, 2014 at 10:08
  • \$\begingroup\$ @Cem, hm looking at it again you might get away without a lock because the DispatcherTimer runs on the UI thread where the KeyDown and KeyUp events are also likely being raised on. \$\endgroup\$
    – ChrisWue
    Mar 15, 2014 at 18:02
  • \$\begingroup\$ Are there any tool like debug that I can watch threads? \$\endgroup\$
    – ctulu
    Mar 15, 2014 at 19:06
  • \$\begingroup\$ In Visual Studio take a look at the Debug/Windows.. menu when debugging. You can view parallel stacks. That will show things running in parallel, in any loaded assembly. \$\endgroup\$ Mar 15, 2014 at 22:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.