12
\$\begingroup\$

Edit: I have added the revised code in an answer to this question.


Currently, I handle the keyboard shortcuts of my applications in a single huge method that looks like this:

    protected override bool ProcessCmdKey(ref Message msg, Keys keyData)
    {
        // Next issue (validate the fix)
        if (keyData == (Keys.Control | Keys.Enter) || 
            keyData == (Keys.Shift | Keys.Enter))
        {
            mark_as_fixed();
            return true;
        }
        // Skip to next issue without validating or changing anything
        if (keyData == (Keys.Alt | Keys.Down))
        {
            next_issue();
        }
        // Previous issue
        if (keyData == (Keys.Alt | Keys.Up))
        {
            previous_issue();
            return true;
        }
        ... [130 lines of this]

It works perfectly, it isn't particularly hard to read or maintain, but having a method 130 lines long and constantly growing just feels wrong.

Also I may want to implement shortcut customization in the future, and there is no easy way to find out what keys are mapped to which method so it will have to be rewritten.

Since all non trivial applications have a gazillion shortcuts, I assume it's a solved problem, but I couldn't find a good explanation on how applications handle their shortcuts.

\$\endgroup\$
  • 6
    \$\begingroup\$ Seems to me the [130 lines of this] could be relevant to a fruitful peer review. Can't you include the whole method body? \$\endgroup\$ – Mathieu Guindon Jul 14 '16 at 19:03
  • 2
    \$\begingroup\$ I'm afraid this question does not match what this site is about. Code Review is about improving complete working code. The stub code that you have posted is not reviewable in this form because it leaves us guessing at your intentions. Code Review needs to look at concrete code in a real context. Please see Why is hypothetical example code off-topic for CR? \$\endgroup\$ – Phrancis Jul 14 '16 at 19:04
  • 1
    \$\begingroup\$ @Mat'sMug I added the closing of the method. There is no point in posting the full list of my program's user methods and shortcuts. \$\endgroup\$ – Sylverdrag Jul 14 '16 at 19:27
  • 9
    \$\begingroup\$ @Phrancis Nothing hypothetical about this method. I use it in an actual program. There is no more context than what is given in this question: The user presses a shortcut key, this method catches it and calls the relevant method of the program. Not sure what a long list of all my program's methods would add to the question. \$\endgroup\$ – Sylverdrag Jul 14 '16 at 19:35
  • 3
    \$\begingroup\$ Hello! Please don't make changes to the original post once it has been reviewed, as that invalidates the current answers. Please see our meta side on performing iterative reviews for more information! \$\endgroup\$ – syb0rg Jul 14 '16 at 19:55
5
\$\begingroup\$

Reusing Mat's Mug's idea of a base Command class: what about using a Dictionary for the mapping?

This way you separate the ideas of command and shortcut (probably losing the potential help of reflection for programmatically instantiate your commands), but you can easily let your users map commands as they need.

| improve this answer | |
\$\endgroup\$
  • 8
    \$\begingroup\$ An IDictionary<Keys,Action> would be ideal and was my first idea, but I dismissed it when I realized some commands had more than one hotkey... although... I guess you could map the same action to two Keys and call it a day. \$\endgroup\$ – Mathieu Guindon Jul 14 '16 at 23:34
  • 2
    \$\begingroup\$ Awesome. IDictionary<Keys,Action> appears to be by far the easiest way to implement and maintain that feature. \$\endgroup\$ – Sylverdrag Jul 15 '16 at 10:55
  • 2
    \$\begingroup\$ To implement, yes. To maintain, no. \$\endgroup\$ – Mathieu Guindon Jul 15 '16 at 11:49
  • \$\begingroup\$ Certainly a Dictionary alone could lead to maintenance issues if the list of shortcuts grows too much. Still, having a big data structure to which add entries as needed is more readable than a huge switch statement (or the if...else equivalent). Probably using this in combination with a more OO solution like yours is the way. \$\endgroup\$ – bassfault Jul 16 '16 at 10:49
15
\$\begingroup\$

Also I may want to implement shortcut customization in the future, and there is no easy way to find out what keys are mapped to which method so it will have to be rewritten.

Stop right here! Extract a class for each command (e.g. instead of a mark_as_fixed method, you'll have a MarkAsFixedCommand class). If you're using WPF you can implement the ICommand interface and have nevermind, you're using WinForms. Still, extract the command classes - WPF's ICommand is a really simple interface that merely exposes bool CanExcute(object) and void Execute(object) methods (you don't have to implement the object parameter if you don't need it) - by extracting your logic into command classes, you can prepare the ground for the day you redo your UI with WPF (which you should!).

there is no easy way to find out what keys are mapped to which method

Make one! Have your command classes derive from an abstract CommandBase class that exposes a method for it:

public abstract bool IsShortcutKey(Keys keys);

Now every derived command class has to implement this IsShortcutKey method. The implementation for MarkAsFixedCommand could look like this:

public override bool IsShortcutKey(Keys keys)
{ 
    return keys == (Keys.Control | Keys.Enter)
        || keys == (Keys.Shift | Keys.Enter); 
}

A cleaner way would have been to expose some Keys ShortcutKey { get; } property getter, but that wouldn't play nice with multiple shortcuts for the same command.

Anyway now that each command implementation is responsible for is shortcut key logic, you can easily fetch the configuration and make that method return true when keys matches whatever Keys value you have in your config.

Then, whoever runs this ProcessCmdKey method simply needs to know about all available commands - take them as an IEnumerable<CommandBase> constructor parameter and let whoever is calling this constructor deal with providing it with the command instances (which you will acquire with some reflection code).

Now ProcessCmdKey can be as simple as this:

// assume only 1 command returns true for specified keyData value:
var command = _commands.SingleOrDefault(cmd => cmd.IsShortcutKey(keyData));
if (command != null && command.CanExecute())
{
    command.Execute();
    return true;
}

return base.ProcessCmdKey(ref msg, keyData);

PS - Method names should be PascalCase, not lower_snake_case.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I've thought to myself before how could I implement something like this into a class to keep it organized. I really like the Keys keys = (Keys.Control | Keys.Enter); trick. For some reason that thinking just flew right past me! Doing something like this would over complicate any programs I write as I'm only dealing with ~3-10 function keys for operators to use. +1 from me though. That's slick. \$\endgroup\$ – Timmy Jul 14 '16 at 20:18
  • 1
    \$\begingroup\$ How is having dedicated classes "hard to maintain"? I'd strongly recommend you start reading up on inversion of control and dependency injection - the commands get injected into the type as a dependency, there's nothing to maintain, no code needs to change anywhere when you implement a new command. A dictionary is useful for a few items, but needs to be populated, and the actions need to live somewhere. When you have 100 commands, you need an architecture, not a dictionary. \$\endgroup\$ – Mathieu Guindon Jul 15 '16 at 11:49
  • 1
    \$\begingroup\$ @Sylverdrag the beautiful thing about commands is that they're easy to reuse. You can take the same action from a hot key, a menu, a right click context menu, on an event, etc. \$\endgroup\$ – RubberDuck Jul 23 '16 at 23:30
  • 1
    \$\begingroup\$ @RubberDuck exactly. without having to implement the code in as many places being the key here ;-) \$\endgroup\$ – Mathieu Guindon Jul 23 '16 at 23:31
  • 1
    \$\begingroup\$ @Mat'sMug it doesn't take much effort to use commands in Winforms either. In fact, I've already done the hard work. github.com/ckuhn203/Rubberduck.Winforms/blob/master/README.md \$\endgroup\$ – RubberDuck Jul 23 '16 at 23:31
3
\$\begingroup\$

I use your method also except I would like to suggest using a case statement instead for readability.

I'm still a rookie programmer with less than a year of experience, so there could be better methods than this.

protected override bool ProcessCmdKey(ref Message msg, Keys keyData) 
    {
        switch (keyData)
        {
            case Keys.F1: 
                //Stuff
                return true;
            case Keys.F2:
                //Stuff
                return true;
            case Keys.Alt | Keys.Down:
                //Stuff
                return true;
        }
        return base.ProcessCmdKey(ref msg, keyData);
    }

Also don't forget your return base.ProcessCmdKey(ref msg, keyData); So you aren't disabling all other keys not specified.

| improve this answer | |
\$\endgroup\$
  • 3
    \$\begingroup\$ In addition you could make the routines like mark_as_fixed return true. This would make the cases one-liners: case Keys.F1: return mark_as_fixed(); \$\endgroup\$ – Olivier Jacot-Descombes Jul 14 '16 at 19:14
  • 1
    \$\begingroup\$ I thought about using a switch, it would save a few lines but really, not much of a difference. It's still a long list of unmapped shortcuts. I am not concerned by how long it is just because it takes space, but because I think there has to be a better way to organize a program shortcuts, with some kind of mapping that would allow to the user to customize the shortcuts, find the current shortcuts for various features, print a list of shortcuts, etc. \$\endgroup\$ – Sylverdrag Jul 14 '16 at 19:45
1
\$\begingroup\$

I'd rather use a more flexible design where you or the user can configure the shortcuts.

In this design each operation would be an ICommand with two members:

public interface ICommand
{
    Keys Keys { get; }
    void Execute();
}

An abstract CommandBase class would implement the Keys property:

public abstract class CommandBase : ICommand
{
    protected CommandBase(Keys keys) { Keys = keys; }
    public Keys Keys { get; }
    public abstract void Execute();
}

and concrete types the Execute method:

public class LoadReportCommand : CommandBase
{
    public LoadReportCommand(Keys keys) : base(keys) { }
    public override void Execute() { }
}
public class MarkAsFixedCommand : CommandBase
{
    public MarkAsFixedCommand(Keys keys) : base(keys) { }
    public override void Execute() { }
}

I would then make the shortcuts configurable via the app.config or any other configuration. The key would be the command name and the the value the keys:

private static IEnumerable<ICommand> _commands;

void Main()
{
    // this would come from a configuration
    var commandShortCuts = new Dictionary<string, string>
    {
        ["LoadReport"] = "Control+O",
        ["MarkAsFixed"] = "Shift+Enter",
    };

    // get all commands in this assembly
    var commandTypes =
        Assembly.GetExecutingAssembly()
        .GetTypes()
        .Where(t => t.BaseType == typeof(CommandBase))
        .ToList();

    // initialize commands and their shortcuts
    _commands = commandShortCuts.Select(x =>
    {
        var keys = x.Value.Split('+').Aggregate(
            Keys.None, 
            (result, next) => 
                result |= (Keys)Enum.Parse(typeof(Keys), next));
        var commandType = commandTypes.SingleOrDefault(t => t.Name.Equals(x.Key + "Command"));
        return (ICommand)Activator.CreateInstance(commandType, keys);
    })
    .ToList();
}

Then you could execute a command like this:

protected override bool ProcessCmdKey(ref Message msg, Keys keyData)
{
    var command = _commands.SingleOrDefault(c => c.Keys == keyData);
    command?.Execute();
}
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Ooops, I now see that I've actaully implemented @Mat'sMug answer. Sorry? ;-) \$\endgroup\$ – t3chb0t Jul 22 '16 at 6:26
0
\$\begingroup\$

This is what I have now, based on Mat's and Bel8z' answers:

Dictionary<Keys, Func<Keys, bool>> dic_shortcut_mngr = new Dictionary<Keys, Func<Keys, bool>>();
private void init_shortcuts()
{
    //Files
    dic_shortcut_mngr.Add((Keys.Control | Keys.O), open);
    dic_shortcut_mngr.Add((Keys.Control | Keys.L), load_report);
    // Operations
    dic_shortcut_mngr.Add((Keys.Control | Keys.Enter), mark_as_fixed);
    dic_shortcut_mngr.Add((Keys.Shift | Keys.Enter), mark_as_fixed);
    dic_shortcut_mngr.Add((Keys.Alt | Keys.Down), next_issue);
    dic_shortcut_mngr.Add((Keys.Alt | Keys.Up), previous_issue);
    dic_shortcut_mngr.Add((Keys.Insert), copy);
    //Copy items
    dic_shortcut_mngr.Add((Keys.Control | Keys.NumPad0), copy_item);
    dic_shortcut_mngr.Add((Keys.Control | Keys.NumPad1), copy_item);
    dic_shortcut_mngr.Add((Keys.Control | Keys.NumPad2), copy_item);
    ...
}

protected override bool ProcessCmdKey(ref Message msg, Keys keyData)
{
    if (dic_shortcut_mngr.ContainsKey(keyData))
    {
        return dic_shortcut_mngr[keyData].Invoke(keyData);
    }
    return base.ProcessCmdKey(ref msg, keyData);
}
private bool copy_item(Keys key)
{
    //NumKeys start at index 96. Removing 96 gets the numkey's value 
    int i = (int)key - 96;
    if (dgv.Rows.Count > i && dgv.Rows[i].Cells["item"] != null)
    {
        string item = dgv.Rows[i].Cells["item"].Value.ToString();
        Clipboard.SetText(item);
        rtb_fix_target.Paste();
    }
    return true;
}

I decided to use Func<Keys, bool> instead of Action to be able to return "true" and pass on the keydata to the methods (which is useful handling the numpad shortcuts as they define the item to be copied)

| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Your naming convention is still very weird. \$\endgroup\$ – t3chb0t Jul 16 '16 at 21:20
  • \$\begingroup\$ @t3chb0t I work alone and I have opted for lower snake case everywhere. Keeps things very simple, fairly readable and neatly differentiates my code from 3rd party code. Are there benefits to a more conventional (and more complex) .NET naming convention that would apply to a single developer? \$\endgroup\$ – Sylverdrag Jul 24 '16 at 20:07
  • \$\begingroup\$ You are never a single developer. You show other devs your code to review it or someone will need to take care of your code in the future and they won't be happy seeing this as we aren't ;-) There is a convention in C# that uses the underscore-snake casing for event handlers. Everytime I see some_name I need to stop and think: is this an event handler? \$\endgroup\$ – t3chb0t Jul 24 '16 at 20:16
  • \$\begingroup\$ @t3chb0t I can hardly see myself changing my naming conventions just because I might post a few lines here every once in a while. If it comes to selling and turning over my code to someone else, Resharper can change the naming convention in a jiffy. I am always keen to improve, but there has to be some benefit beyond "that's how we do it" (whoever is "we" anyway?). As for the confusion with event handlers, if I am not mistaken, .NET event handlers are a mix of camelCase, snake_case and PascalCase: "toolStripButton_DoubleClick()". \$\endgroup\$ – Sylverdrag Jul 24 '16 at 22:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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