0
\$\begingroup\$

The objective of this code is to make the difference inbetween two lists of items, that is knowing which items are missing in the second list in regards to the first one.

The first list is built from an enumerator with the following method:

keysToPress = Enum.GetValues(enumType).ToList();

Once this list is built, now the second list, the one we desire to compare has to be retrieved. In my code, I ask a device I am testing the information of a register. Processing this register returns me a list of type enumType.

// ReadKeyboard method retrieves a list of all the keys that have been 
// pressed at least one time during the device test. 
// (Read notes to know why the code is missing, I feel like it is not needed)
keysPressed = GetKeyList();

Once both list have been retrieved, I only have to make the difference in between them with the following code:

keysPressed.ForEach(x=> keysToPress.Remove(x));

This way, I remove every item in the keysPressed list from the keysToPress list.

The resulting code is as follows:

private List<enumType> CheckKeysToPress()
{
    List<enumType> keysToPress = Enum.GetValues(typeof(enumType)).Cast<enumType>().ToList();

    // ReadKeyboardMask returns a enumType list
    List<enumType> keysPressed = GetKeyList();

    // Here I try to remove all the items in the keysPressed list from the keysToPress list
    keysPressed.ForEach(x=> keysToPress.Remove(x));

    return keysToPress;
}

As you can see, the return value of the method is a list of all remaining keys to press.

An example for the following code, given the enum:

[Flags]
public enum Keys
{
    NONE = 0x0000,
    UP = 0x0001,
    DOWN = 0x0002,
    LEFT = 0x0004,
    RIGHT = 0x0008,
    OK = 0x0010,
    POWER = 0x0020
}

The keysToPress list would have the following items:

  • NONE
  • UP
  • DOWN
  • LEFT
  • RIGHT
  • OK
  • POWER

With a reading from the device that returns a list with the following items.

  • UP
  • DOWN
  • LEFT
  • RIGHT

would output the following list:

  • NONE
  • OK
  • POWER

I want to know, as the information retrieved is a set of bit flags, if it is better to provide the bit flags and then remove those items that match the enum, to optimise and make less error prone the code.

Note: The code regarding GetKeyList() is missing because it is a function that interacts with a device to retrieve information from it communicating through ModBus protocol.

\$\endgroup\$
2
\$\begingroup\$

What I like about the method is that it's implementation is straight forward and easy to understand. There are, however, a few things that may be done done differently.

  1. The name of the method is misleading, as it does not actually check keys, but it build/returns a list of keys not pressed at the current moment.
  2. The result of Enum.GetValues(typeof(enumType)) should never change during runtime, thus running it on every method call must be sub-optimal on recurring calls.
  3. Using the ForEach method and using List<T>.Remove to remove the items looks fine, but List<T> might not be the best data type to remove items from. You could also use a HashSet<T>, which would not go all the way to Array.IndexOf<T>.

I tested around a bit and found that another option - not removing items from a list - is actually marginally faster.

    private readonly IReadOnlyList<Keys> keysToPress = Enum.GetValues(typeof(Keys)).Cast<Keys>().ToList();

    private List<Keys> CheckKeysToPressWithExcept()
    {
        // You could just use LINQ - the method probably won't get any simpler than this.
        return this.keysToPress.Except(GetKeyList()).ToList();
    }

    private List<Keys> CheckKeysToPressWithSet()
    {
        var keysPressed = new HashSet<Keys>(GetKeyList());

        // Or you can construct the resulting list manually.
        // Performs better, but is more code to read.
        var result = new List<Keys>(this.keysToPress.Count);
        foreach (var key in this.keysToPress)
        {
            if (!keysPressed.Contains(key))
            {
                result.Add(key);
            }
        }

        return result;
    }

In a quick benchmark, CheckKeysToPressWithSet was the fastest, a modified version of your CheckKeysToPress that uses the IReadOnlyList<Keys> keysToPress instead of re-creating the enum value list every time, was 2% slower and the LINQ was 14% slower.

\$\endgroup\$
  • \$\begingroup\$ In regard to your points: 1) The register returns which keys have been pressed at least one time, not the currently pressed ones. This verifies that the keyboard is working fine and no keys have a soldering or functional problem. I believe I have not expressed myself well in that regard. 2) This is fantastic, don't know why I didn't think of this before, since this method is particular and not generic, there's no reason to build in runtime the list. \$\endgroup\$ – Oscar Guillamon Nov 25 '15 at 9:04

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.