7
\$\begingroup\$

I have several UI elements that can be opened using hotkeys for example the settings menu's key is Escape. The problem is that I have the Keycode stored in Dictionary along with an Action or a Button.ButtonClickedEvent here's how the two dictionaries are declared:

    public static readonly Dictionary<KeyCode, Button.ButtonClickedEvent> KeysUIElementsWithButtonEvents = new Dictionary<KeyCode, Button.ButtonClickedEvent>
    {
        { KeyCode.P, SpellbookButton.onClick },
        { KeyCode.Escape, SettingsMenuButton.onClick },
    };

    public static readonly Dictionary<KeyCode, Action> KeysUIElementsWithActions = new Dictionary<KeyCode, Action>
    {
        { KeyCode.LeftAlt, SwitchCursorState},
    };

And here's how I used to invoke them :

        foreach (var item in Settings.Settings.KeysUIElementsWithButtonEvents)
        {
            if(Input.GetKeyDown(item.Key))
            {
                if(Cursor.lockState == CursorLockMode.Locked)
                {
                    Settings.Settings.SwitchCursorState();
                }
                item.Value.Invoke();
            }
        }

        foreach (var item in Settings.Settings.KeysUIElementsWithActions)
        {
            if (Input.GetKeyDown(item.Key))
            {
                if (Cursor.lockState == CursorLockMode.Locked)
                {
                    Settings.Settings.SwitchCursorState();
                }
                item.Value.Invoke();
            }
        }

I didn't like the repetitive code in here so I decided to create a function which solves the problem:

    CheckUIKeysCollection(Settings.Settings.KeysUIElementsWithButtonEvents);                              
    CheckUIKeysCollection(Settings.Settings.KeysUIElementsWithActions);

    private void CheckUIKeysCollection<T>(Dictionary<KeyCode, T> collection)
    {
        foreach (var item in collection)
        {
            if (Input.GetKeyDown(item.Key))
            {
                if (Cursor.lockState == CursorLockMode.Locked)
                {
                    Settings.Settings.SwitchCursorState();
                }
                if (item.Value is Action)
                {
                    (item.Value as Action).Invoke();
                    continue;
                }
                (item.Value as Button.ButtonClickedEvent).Invoke();
            }
        }
    }

However I'm not sure if the method is efficient enough it involves some casting here and there and it's also being called in the Update function (every frame) and if I have big collections it might hurt the performance. What can I do to improve the method?

\$\endgroup\$
  • \$\begingroup\$ Maybe you could wrap your Button.ButtonClickedEvent and Action in a class that would do the work. This solution would allow you to have one dictionary instead of two and a simpler invoke process in the iteration. \$\endgroup\$ – Stud Nov 28 '16 at 11:20
5
\$\begingroup\$

The easiest way to solve this with one dictionary is to simply create an action for the Button.ButtonClickedEvent calls:

public static readonly Dictionary<KeyCode, Button.ButtonClickedEvent> KeysUIElementsWithButtonEvents = new Dictionary<KeyCode, Button.ButtonClickedEvent>
{
    { KeyCode.P, SpellbookButton.onClick },
    { KeyCode.Escape, SettingsMenuButton.onClick },
};

public static readonly Dictionary<KeyCode, Action> KeysUIElementsWithActions = new Dictionary<KeyCode, Action>
{
    { KeyCode.LeftAlt, SwitchCursorState},
};

Can be one Dictionary<KeyCode, Action>:

public static readonly Dictionary<KeyCode, Action> KeysUIElementsWithActions = new Dictionary<KeyCode, Action>
{
    { KeyCode.P, () => SpellbookButton.onClick.Invoke() },
    { KeyCode.Escape, () => SettingsMenuButton.onClick.Invoke() },
    { KeyCode.LeftAlt, SwitchCursorState }
}

Then this:

private void CheckUIKeysCollection<T>(Dictionary<KeyCode, T> collection)
{
    foreach (var item in collection)
    {
        if (Input.GetKeyDown(item.Key))
        {
            if (Cursor.lockState == CursorLockMode.Locked)
            {
                Settings.Settings.SwitchCursorState();
            }
            if (item.Value is Action)
            {
                (item.Value as Action).Invoke();
                continue;
            }
            (item.Value as Button.ButtonClickedEvent).Invoke();
        }
    }
}

Becomes this:

private void CheckUIKeysCollection(Dictionary<KeyCode, Action> collection)
{
    foreach (var item in collection)
    {
        if (Input.GetKeyDown(item.Key))
        {
            if (Cursor.lockState == CursorLockMode.Locked)
            {
                Settings.Settings.SwitchCursorState();
            }
            item.Value.Invoke();
        }
    }
}

No more generics, no more casting, no more inferences. Simply create an action that calls the ButtonClickedEvent.Invoke() method for the appropriate Button.

You could, also, create a method that does that for you:

public void ClickButton(Button button)
{
    button.onClick.Invoke();
}

Then make your dictionary:

public static readonly Dictionary<KeyCode, Action> KeysUIElementsWithActions = new Dictionary<KeyCode, Action>
{
    { KeyCode.P, ClickButton(SpellbookButton) },
    { KeyCode.Escape, ClickButton(SettingsMenuButton) },
    { KeyCode.LeftAlt, SwitchCursorState }
}

This also means it's simpler to click buttons from anywhere in your code: simply call ClickButton(Button), instead of Button.onClick.Invoke().


Apart from that:

Using is then as is not recommended, simply use as then check if it's null:

var temp = value as string;

if (temp != null)
{
    Console.WriteLine(temp.Length);
}
\$\endgroup\$

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.