Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I made a piece of software that basically adds the functionality of a compose key to Windows, after switching from one keyboard layout to another (for programming reasons), and being unable to find a piece of proper software that did what I wanted.

This was my first venture into making applications for Windows, never touching languages like C# before. I'm using two libraries (which I assume is written in C and compiled) for hooking and simulating keypresses, as I could not find any way of doing this in C#.

Now, this is bound to have some mistakes, bad practices or potential issues, and without knowing someone that could possibly review my code, I came here. The program is running and I've tested this on both Windows 8.x and Windows 7 and seem to run just fine on both in my tests. I used VS13 community edition, which has been of great help getting started.

The project in its entirety is hosted on GitHub, although I will post the main file here, I'd love to get a review of the entire structure/codebase.

using Microsoft.Win32;
using MouseKeyboardActivityMonitor;
using MouseKeyboardActivityMonitor.WinApi;
using System;
using System.Collections.Generic;
using System.Reflection;
using System.Windows.Forms;
using WindowsInput;

namespace Compose
{
    public class App : Form
    {
        [STAThread]
        public static void Main()
        {
            Application.EnableVisualStyles();
            Application.SetCompatibleTextRenderingDefault(false);
            Application.Run(new App());
        }

        private KeyboardHookListener keyboardHook;
        private InputSimulator simulator;

        public int composeIndex = 0;
        public String composeString = "";
        public bool isComposing;

        public Dictionary<int, char> lowerCase;
        public Dictionary<int, char> upperCase;
        public Dictionary<string, char> combinations;

        private NotifyIcon trayIcon;
        private ContextMenu trayMenu;

        public App()
        {
            AppDomain.CurrentDomain.AssemblyResolve += (sender, args) =>
            {
                string resourceName = new AssemblyName(args.Name).Name + ".dll";
                string resource = Array.Find(this.GetType().Assembly.GetManifestResourceNames(), element => element.EndsWith(resourceName));

                using (var stream = Assembly.GetExecutingAssembly().GetManifestResourceStream(resource))
                {
                    Byte[] assemblyData = new Byte[stream.Length];
                    stream.Read(assemblyData, 0, assemblyData.Length);
                    return Assembly.Load(assemblyData);
                }
            };

            InititalizeComponent();
        }

        private void InititalizeComponent()
        {
            #region Keys to lowercase character
            lowerCase = new Dictionary<int, char>();
            lowerCase.Add(65, 'a');
            lowerCase.Add(66, 'b');
            lowerCase.Add(67, 'c');
            lowerCase.Add(68, 'd');
            lowerCase.Add(69, 'e');
            lowerCase.Add(70, 'f');
            lowerCase.Add(71, 'g');
            lowerCase.Add(72, 'h');
            lowerCase.Add(73, 'i');
            lowerCase.Add(74, 'j');
            lowerCase.Add(75, 'k');
            lowerCase.Add(76, 'l');
            lowerCase.Add(77, 'm');
            lowerCase.Add(78, 'n');
            lowerCase.Add(79, 'o');
            lowerCase.Add(80, 'p');
            lowerCase.Add(82, 'r');
            lowerCase.Add(83, 's');
            lowerCase.Add(84, 't');
            lowerCase.Add(85, 'u');
            lowerCase.Add(86, 'v');
            lowerCase.Add(87, 'w');
            lowerCase.Add(88, 'x');
            lowerCase.Add(89, 'y');
            lowerCase.Add(90, 'z');
            lowerCase.Add(49, '1');
            lowerCase.Add(50, '2');
            lowerCase.Add(51, '3');
            lowerCase.Add(52, '4');
            lowerCase.Add(53, '5');
            lowerCase.Add(54, '6');
            lowerCase.Add(55, '7');
            lowerCase.Add(56, '8');
            lowerCase.Add(57, '9');
            lowerCase.Add(48, '0');
            lowerCase.Add(189, '-');
            lowerCase.Add(187, '=');
            lowerCase.Add(186, ';');
            lowerCase.Add(222, '\'');
            lowerCase.Add(220, '\\');
            lowerCase.Add(188, ',');
            lowerCase.Add(190, '.');
            lowerCase.Add(191, '/');
            lowerCase.Add(192, '`');
            #endregion

            #region Keys to uppercase character
            upperCase = new Dictionary<int, char>();
            upperCase.Add(65, 'A');
            upperCase.Add(66, 'B');
            upperCase.Add(67, 'C');
            upperCase.Add(68, 'D');
            upperCase.Add(69, 'E');
            upperCase.Add(70, 'F');
            upperCase.Add(71, 'G');
            upperCase.Add(72, 'H');
            upperCase.Add(73, 'I');
            upperCase.Add(74, 'J');
            upperCase.Add(75, 'K');
            upperCase.Add(76, 'L');
            upperCase.Add(77, 'M');
            upperCase.Add(78, 'N');
            upperCase.Add(79, 'O');
            upperCase.Add(80, 'P');
            upperCase.Add(82, 'R');
            upperCase.Add(83, 'S');
            upperCase.Add(84, 'T');
            upperCase.Add(85, 'U');
            upperCase.Add(86, 'V');
            upperCase.Add(87, 'W');
            upperCase.Add(88, 'X');
            upperCase.Add(89, 'Y');
            upperCase.Add(90, 'Z');
            upperCase.Add(49, '!');
            upperCase.Add(53, '%');
            upperCase.Add(54, '^');
            upperCase.Add(57, '(');
            upperCase.Add(48, ')');
            upperCase.Add(189, '_');
            upperCase.Add(187, '+');
            upperCase.Add(186, ':');
            upperCase.Add(222, '"');
            upperCase.Add(220, '|');
            upperCase.Add(188, '<');
            upperCase.Add(190, '>');
            upperCase.Add(191, '?');
            upperCase.Add(192, '~');
            #endregion

            #region Compose combinations to unicode character
            combinations = new Dictionary<string, char>();
            combinations.Add("!!", '\u00a1'); // Inverted exclamation mark
            combinations.Add("??", '\u00bf'); // Inverted question mark
            combinations.Add("!^", '\u00a6'); // Broken bar
            combinations.Add("<<", '\u00ab'); // Left-pointing double angle quotation mark
            combinations.Add(">>", '\u00bb'); // Right-pointing double angle quotation mark
            combinations.Add("<'", '\u2018'); // Left single quotation mark
            combinations.Add("'<", '\u2018'); // Left single quotation mark
            combinations.Add(">'", '\u2019'); // Right single quotation mark
            combinations.Add("'>", '\u2019'); // Right single quotation mark
            combinations.Add("<\"", '\u201c'); // Left double quotation mark
            combinations.Add("\">", '\u201d'); // Right double quotation mark
            combinations.Add("|c", '\u00a2'); // Cent sign
            combinations.Add("c|", '\u00a2'); // Cent sign
            combinations.Add("/c", '\u00a2'); // Cent sign
            combinations.Add("c/", '\u00a2'); // Cent sign
            // ^^^ This is just a snippet of the entire list of combinations, please see the original file (App.cs) on GitHub for all of them if needed
            #endregion

            keyboardHook = new KeyboardHookListener(new GlobalHooker());
            keyboardHook.Enabled = true;
            keyboardHook.KeyDown += keyboardHook_KeyDown;

            simulator = new InputSimulator();

            trayMenu = new ContextMenu();
            trayMenu.MenuItems.Add("Settings", trayMenu_Settings);
            trayMenu.MenuItems.Add("Exit", trayMenu_Exit);

            trayIcon = new NotifyIcon();
            trayIcon.Text = "Compose";
            trayIcon.Icon = Resources.Icon;
            trayIcon.ContextMenu = trayMenu;
            trayIcon.Visible = true;
        }

        private void keyboardHook_KeyDown(object sender, KeyEventArgs e)
        {
            if (Register.active)
                return;

            var modifier = (Keys)Settings.GetModifier();
            if (isComposing)
            {
                if (e.KeyCode == modifier || e.KeyCode == Keys.Escape || e.KeyCode == Keys.Back || e.KeyCode == Keys.Enter)
                {
                    isComposing = false;

                    if (Settings.ShouldIndicate())
                        simulator.Keyboard.KeyPress(WindowsInput.Native.VirtualKeyCode.SCROLL);
                }
                else
                {
                    if (e.KeyCode != Keys.LShiftKey && e.KeyCode != Keys.RShiftKey)
                        composeIndex++;
                    else
                        return;

                    var keyCode = (int)e.KeyCode;
                    if (e.KeyCode == Keys.Space)
                        composeString += " ";
                    else if (e.Shift && upperCase.ContainsKey(keyCode))
                        composeString += upperCase[keyCode];
                    else if (!e.Shift && lowerCase.ContainsKey(keyCode))
                        composeString += lowerCase[keyCode];

                    if (composeIndex == 2)
                    {
                        isComposing = false;

                        if (Settings.ShouldIndicate())
                            simulator.Keyboard.KeyPress(WindowsInput.Native.VirtualKeyCode.SCROLL);

                        if (combinations.ContainsKey(composeString))
                            simulator.Keyboard.TextEntry(combinations[composeString]);
                    }
                }

                e.SuppressKeyPress = true;
            }
            else
            {
                if (e.KeyCode == modifier)
                {
                    if (Settings.ShouldIndicate())
                        simulator.Keyboard.KeyPress(WindowsInput.Native.VirtualKeyCode.SCROLL);

                    isComposing = true;
                    e.SuppressKeyPress = true;

                    composeIndex = 0;
                    composeString = "";
                }
            }
        }

        private void trayMenu_Settings(object sender, EventArgs e)
        {
            Options options = Application.OpenForms["Options"] as Options;
            if (options != null)
                options.BringToFront();
            else
            {
                var newOptions = new Options();
                newOptions.Show();
            }
        }

        private void trayMenu_Exit(object sender, EventArgs e)
        {
            Application.Exit();
        }

        protected override void Dispose(bool disposing)
        {
            if (disposing)
                trayIcon.Dispose();

            base.Dispose(disposing);
        }

        protected override void OnLoad(EventArgs e)
        {
            Visible = false;
            ShowInTaskbar = false;

            base.OnLoad(e);
        }
    }

    public class Settings
    {
        private static String appPath = "HKEY_CURRENT_USER\\Software\\Compose";
        private static String runPath = "HKEY_CURRENT_USER\\Software\\Microsoft\\Windows\\CurrentVersion\\Run";

        public static int GetModifier()
        {
            return Convert.ToInt32(Registry.GetValue(appPath, "Modifier", 0));
        }

        public static bool ShouldIndicate()
        {
            return Convert.ToString(Registry.GetValue(appPath, "Indicate", 0)).Length != 0;
        }

        public static bool ShouldAutoRun()
        {
            return Convert.ToString(Registry.GetValue(runPath, "Compose", "")).Length != 0;
        }

        public static void SetModifier(int modifier)
        {
            Registry.SetValue(appPath, "Modifier", modifier, RegistryValueKind.DWord);
        }

        public static void SetShouldIndicate(bool enabled)
        {
            Registry.SetValue(appPath, "Indicate", enabled, RegistryValueKind.DWord);
        }

        public static void SetShouldAutoRun(bool enabled)
        {
            if (enabled)
                Registry.SetValue(runPath, "Compose", Application.ExecutablePath, RegistryValueKind.String);
            else
                Registry.SetValue(runPath, "Compose", "", RegistryValueKind.String);
        }
    }
}
share|improve this question

1 Answer 1

I'm not vehemently opposed to the use of Regions, but I am against using them inside of methods. I consider it a definitive code smell. Regions inside of methods should be extracted into their own method and called. For example

private void InititalizeComponent()
{
    lowerCase = GetLowerCaseDictionary();
    upperCase = GetUpperCaseDictionary();
    combinations = GetCombinationsDictionary();

    SetupKeyboardHook();

    simulator = new InputSimulator();

    SetupTrayMenu();
    SetupTrayIcon();

}

Now, if a change needs to be made to how any of these things gets setup, you can go directly to that method instead of searching through hundreds of lines of code for the part of the setup you're looking for.

Okay... Let me rephrase this. Extract Methods. Extract Methods everywhere. Your keyboardHook_KeyDown method is 59 lines. I have to scroll several times to see the whole method. We're human, we can't hold that much in our heads at once. Extracting methods isn't just to remove repetition from our code. It's for making methods singly responsible and small enough to understand at a glance. If we want the detail, we dive into the smaller methods. Otherwise, we get a quick overview of what's happening without concerning ourselves with the details.


Your Settings class is much better in this regard, but there are other issues with it.

  • All of the methods are static, so you might as well make the class itself static. There's no sense in ever having instances of it, there is no state.
  • It's arguably preferable to use an XML configuration over the Windows Registry. Please see What is App.config in C#.Net and How do I use it? However, there's nothing wrong with using the registry. It can be considered to be a matter of preference.
  • If you decide to stick with the registry, you should change where you're registering your keys to.

    private static String appPath = "HKEY_CURRENT_USER\\Software\\Compose";
    private static String runPath = "HKEY_CURRENT_USER\\Software\\Microsoft\\Windows\\CurrentVersion\\Run";
    

    Take a look at the other keys under Software. Note how all (most) of them are in the form of Company\NameOfSoftware\. You should follow this convention. It makes it infinitely less likely that you'll run into a "namespace" conflict with some other piece of software. Since this is open source software, I recommend using your github user name.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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