Over-Abstraction? A debate.
I've been having an argument with a co-worker of mine about the KeyboardHook code I recently published here.
His claim - by exposing a KeyboardHook object and letting users add Filters for specific keystrokes, I am exposing too much of my inner implementation that shouldn't interest the user of my library. Properly abstracted and refactored, the user should treat each KeyboardHook object as a specific keystroke-capture object, not have to deal with two different objects here. Various implementation details like explicitly calling Install() should be implicitly called whenever the event is hooked up to an event handler, rather than being left to the innocent caller.
While my code uses this:
KeyboardHook hook = new KeyboardHook();
hook.AddFilter(Keys.F, true, false, true);
hook.AddFilter(Keys.F6, false, true, false);
hook.KeyPressed +=new KeyboardHookEventHandler(hook_KeyPressed);
hook.Install();
He believes it should be more like this:
KeyboardHook firstHook = KeyboardHook.Create(Keys.F, true, false, true);
KeyboardHook secondHook = KeyboardHook.Create(Keys.F6, false, true, false);
firstHook.KeyPressed +=new KeyboardHookEventHandler(firstHook_KeyPressed);
secondHook.KeyPressed +=new KeyboardHookEventHandler(secondHook_KeyPressed);
My counter-claim is that while this may be more abstract and refactored, it may be taking it a bit too far. A Windows hook is an actual operating system object, not just a logical construct I create in my code, and there can be serious performance implications for defining one of them for each keystroke rather than defining just one generic hook for all strokes. Furthermore, I want the users of my library to be aware of this difference, rather than hiding this information. I want them to be aware of the implications of having a hook installed, and allow them to install and uninstall it selectively.
His counter-counter-claim was that I can implement all my static methods to hide the actual system hook and have all my KeyboardHook objects actually receive their events from that one OS object. I can have a reference counting mechanism set up to allow me to disable individual hooks, and only uninstall the inner hook when all stroke-captures are disabled.
My prompt reply was that this might, indeed, produce rather clean and elegant code. However, it would also involve rewriting most of the existing code for this library for a relatively small and theoretical gain for the library user, and I'll be damned if I go and do that any time soon, and he's welcome to do it himself if he-thinks-he-knows-so-much.
The conversation generally deteriorated from that point onwards.
So, anyone have any comments on the current design or the proposed changes, the benefits and shortcomings of both?
11 Comments
Comments have been disabled for this content.
Omer van Kloeten said
I think your design is more consistent with other design choices made in Windows Forms, for instance, the KeyPressed (and suchlike) events: There's only one event for any key that might have been pressed and you're supposed to sit there and call a switch on the key code.
I would, however, change the behaviour to a Singleton so that you could simply do this:
KeyboardHook[Keys.F6].KeyPressed += new KeyboardHookEventHandler(hook_KeyPressed);
And underneath it, I would have any implementation needed.
This way, you would not have to create objects and also have only one point of installation (instead of adding filters, event handlers and calling Install).
Avner Kashtan said
First of all, this opens up the syntax hassle of having an indexer with multiple parameters:
KeyboardHook[Keys.F6,true,false,false].KeyPressed += ...
Secondly, this isn't really a singleton - the indexer would return 'this', I assume, the single KeyboardHook instance, while doing some plumbing beforehand to add a new filter for the given combination. This seems to be a problem, since a user expects an indexer to return an instance out of a collection, not return the collection instance itself while performing some side-effect code behind the scenes.
Avner Kashtan said
To be a bit clearer - in your code, this:
KeyboardHook[Keys.F6] == KeyboardHook[Keys.F];
would return true - and I don't think that should be appropriate behavior.
James said
I'd tend to agree with your co-worker that in theory his abstraction is preferable. With your implementation, logic has to be inserted into the event handling method to switch based on the key pressed, whereas with his implementation, there can be one handler method per logical event (i.e. F8_Pressed(object sender, EventArgs e), F10_Pressed(object sender, EventArgs e)). This would generally lead to easier-to-read code.
That being said, it is only a minor issue, nothing worth spending a lot of time changing.
Omer van Kloeten said
First off, worst-case, it's just one more parameter for the indexer - a Flags enumeration containing Ctrl, Shift and Alt.
What I was thinking of is a redesign of the interface. I hadn't looked at the whole source before posting (meaning I had no idea what the 3 booleans were for :P), but now that I have, you could just create this:
KeyboardHook[Modifiers.None, Keys.F6] += new KeyboardHookEventHandler(hook_KeyPressed);
The exposed object would be a delegate (KeyboardHook is a delegate collection). When a key is pressed, you call the specific delegate, etc.
Dotan Dimet said
Why separate calls for AddFilter/Create and setting the callback method?
The one advantage I see in your friend's method is that the user has a distinct callback for each key stroke he adds (alt_shift_F_keypressed_hook, for example) rather than a single callback that has to handle all the different keystrokes (presumably in some switch statement?)
Otherwise, it doesn't gain the user much (still two calls), and by messing about behind the scenes with reference counting instead of letting the user do his own resource management (calling KeyboardHook::new and KeyboardHook::install), you increase the chance of a problem in your code while decreasing the ability of the user to find it.
Scott Allen said
I prefer your original implementation - the hook itself is the primary abstraction, and a single hook can have multiple filters. It makes instant sense to me, whereas the proposed change has me looking hard to see what is happening.
weblogs.asp.net said
426743.. Slap-up :)
Hair said
You really make it seem so easy together with your presentation but I to find this matter to be really one thing that I feel I would never understand. It kind of feels too complicated and extremely huge for me. I'm looking forward for your subsequent post, I will attempt to get the grasp of it!
Gonzalez said
nice post. Thumbs up!
Mccreary said
As the stages worsen, the patient will be unable to recognize your face and who you are and they will not know what year they are in or where they are. The recovery rate of damaged muscles is very high. The key is feeling comfortable with one's professional massage therapist. It works on sore & tired muscles as a tool to prevent injuries!! In all likelihood, you will also be seeing a physical (physio) therapist. Apart from these, there are also certain conditions you may not know you suffer from, but which can be the cause of your neck pain. This means they end up taking more last minute clients to make ends meet. So what do you do if it's just too pricey.