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

Step by Step, ooh HotKey

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

Problem

Still hooked on Windows, I refactored the hook into a number of interfaces, and I wanted to support 2-step hotkeys, so I started with defining an IHook that can be attached, detached, exposes a property that tells whether a hook is attached or not, and raises an event when the hook receives a message:

public interface IHook
{
    event EventHandler MessageReceived;
    void OnMessageReceived();

    bool IsAttached { get; }

    void Attach();
    void Detach();
}


The HookEventArgs simply carry a Keys enum value:

public class HookEventArgs : EventArgs
{
    private readonly Keys _key;

    public HookEventArgs(Keys key)
    {
        _key = key;
    }

    public Keys Key { get { return _key; } }

    public new static HookEventArgs Empty {get { return new HookEventArgs(Keys.None); }}
}


Then I extended IHook into a IHotKeyHook, which exposes a HookInfo value and a flag that indicates whether that hotkey is a 2-step hotkey, like Visual Studio's Ctrl+R,M for refactor/extract method:

public interface IHotKeyHook : IHook
{
    HookInfo HookInfo { get; }
    bool IsTwoStepHotKey { get; }
}


I also extracted a TimerHook, and gave it its own interface:

public interface ITimerHook
{
    event EventHandler Tick;
    void Attach();
    void Detach();
}


Lastly, I added a level of abstraction for an object that would be responsible for managing all these hooks and passing the events to the Rubberduck App class:

public interface IRubberduckHooks : IHook, IDisposable
{
    IEnumerable Hooks { get; }
    void AddHook(THook hook) where THook : IHook;
}


App

Purely for context, here's how the App class consumes IRubberduckHooks - I have yet to actually handle the hotkeys and map them to actual commands, but the backbone is here:

```
private async void hooks_MessageReceived(object sender, HookEventArgs e)
{
if (sender is LowLevelKeyboardHook)
{
if (_skipKeyUp)
{

Solution

In HookEventArgs, do you have multiple keys or a single one?

Because this : public Keys Key { get { return _key; } } is confusing!

Keys is an enum. According to the naming guideline, your enum should be pluralized if it's a Flag enum. I don't know if that's your case, but if so the property name should be Keys, otherwise the enum should be Key.

And shouldn't you check for null? Mat, I remember writing this comment everytime I review your code. We should have a chat :p

In App, the cyclomatic complexity must be pretty high. Because there's the todo in there, I can't really propose a refactor. Plus, well maybe you won't be able to refactor since it seems there's a specific order in the execution of your code. But maybe there are methods you could extract!

In RubberduckHooks, const members should be on top, and I think readonly members should be separated from the others, but I'm not sure.

I know we should use brackets all the time :

if (!IsAttached)
{
    return;
}


But, well... Maybe, just maybe, using this : if (!IsAttached) return; would be good. It removes LoC, and well, considering it's on the same line you can't really introduce bugs. But that's just my thing, I dislike having brackets everywhere for returns.

Finally, maybe it's just because I have very minimal knowledge of the winapi but this :

/// 
/// Gets the integer portion of a word
/// 
private static int LoWord(int dw)
{
    return (dw & 0x8000) != 0
        ? 0x8000 | (dw & 0x7FFF)
        : dw & 0xFFFF;
}


lacks comments. I have almost no idea what is happening in that method.

In HotKeyHook, fields/properties/constructor are mixed up.

Code Snippets

if (!IsAttached)
{
    return;
}
/// <summary>
/// Gets the integer portion of a word
/// </summary>
private static int LoWord(int dw)
{
    return (dw & 0x8000) != 0
        ? 0x8000 | (dw & 0x7FFF)
        : dw & 0xFFFF;
}

Context

StackExchange Code Review Q#113498, answer score: 7

Revisions (0)

No revisions yet.