5
\$\begingroup\$

This is my first test at making a key event dispatcher. Some of the code is unnecessary if one is interested in separating lower and upper cases, however I want to treat them as the same. Hence my wonderfully named method keyToLowerCaseIfUpperCase(char c). I also want to lock the key if it has been pressed untill it is released by the user. To run an event twice the user first needs to release the key.

First of it checks if the key is in [A...Z], if so it returns the corresponding lower key. If not it will keep the key intact. Then I check whetever a key was pressed or not pressed, if it was pressed it will check if that specific key actually can be pressed (using my lock). If it was a key release it will simply release the lock of that key.

import java.awt.KeyEventDispatcher;
import java.awt.event.KeyEvent;

import java.util.HashMap;

public class KeyBindManager implements KeyEventDispatcher
{
    private HashMap<String, Boolean> keyPresses = new HashMap<>();

    @Override
    public boolean dispatchKeyEvent(KeyEvent e) {
        String pressed = keyToLowerCaseIfUpperCase(e.getKeyChar())+"";

        if (e.getID() == KeyEvent.KEY_PRESSED) {

            if (keyPresses.get(pressed) != null && keyPresses.get(pressed).booleanValue()) {
                return false;
            }
            keyPresses.put(pressed, new Boolean(true));

            switch (keyToLowerCaseIfUpperCase(e.getKeyChar())) {
                case 'a': System.out.println("D"); break; // these are just two test cases
                case 'b': System.out.println("E"); break;
                default: break;
            }

        } else if (e.getID() == KeyEvent.KEY_RELEASED){
            keyPresses.put(pressed, new Boolean(false));
        }
        return false;
    }

    private char keyToLowerCaseIfUpperCase (char c) {
        if (c >= 'A' && c <= 'Z') {
            c = Character.toLowerCase(c);
        }

        return c;
    }
}

Please tell me this can be done in a more clean approach than what I've accomplished. I don't like the looks of my code very much at all. For now, it gets the job done.

To emphasize, it is the first draft for it. Every criticism is welcome.

\$\endgroup\$
  • \$\begingroup\$ First, why? Why won't some other approach work better? Second, don't use the keyChar, instead, use the keyCode. It's possible that until the key is released that the keyChar won't be interrupted. The keyCodes are also independent of the modifier keys (shift, alt, ctrl, meta)... \$\endgroup\$ – MadProgrammer Feb 15 '15 at 23:38
  • \$\begingroup\$ @MadProgrammer, I never said some other approach couldn't work better. More like the opposite, "Please tell me this can be done in a more clean approach than what I've accomplished.". \$\endgroup\$ – Emz Feb 15 '15 at 23:46
  • \$\begingroup\$ I'm still not sure what it's you're hoping to achieve though, what is the problem you are trying to overcome? \$\endgroup\$ – MadProgrammer Feb 15 '15 at 23:47
  • \$\begingroup\$ I want a key manager that works globally for the project. That is easily to build upon without having to manage every component by themselves. I am trying to avoid components and instead work directly with Graphics. \$\endgroup\$ – Emz Feb 15 '15 at 23:52
  • \$\begingroup\$ So, unless you're restricting yourself to AWT based APIs, you will have at least one single JComponent somewhere (I'm just throwing fuel on the fire :)) \$\endgroup\$ – MadProgrammer Feb 16 '15 at 0:00
6
\$\begingroup\$

First...

I'd, personally, rely on the virtual key code over the key character. It's possible that the key character may not have been interpreted until the KEY_TYPED event (or at least I remember some issue where the char could be interpreted at certain stages)

private HashMap<Integer, Boolean> keyPresses = new HashMap<>();

@Override
public boolean dispatchKeyEvent(KeyEvent e) {
    int keyCode = e.getKeyCode();

    if (e.getID() == KeyEvent.KEY_PRESSED) {

        if (keyPresses.contains(keyCode) && keyPresses.get(keyCode).booleanValue()) {
            return false;
        }
        keyPresses.put(keyCode, new Boolean(true));

This also means you don't need to convert the key to lower case, as the virtual key doesn't carry the modifier information (the key character is the key code + the modifiers)

There is a problem with this, KeyEvent.VK_1 can represent both 1 and !, so you might need to consider using KeyStroke instead...

Second...

It's a nitpick on my behalf, but I like a single entry and single exit point in the code...

@Override
public boolean dispatchKeyEvent(KeyEvent e) {
    int keyCode = e.getKeyCode();

    if (e.getID() == KeyEvent.KEY_PRESSED) {

        if (!keyPresses.contains(keyCode) || !keyPresses.get(keyCode).booleanValue()) {
            keyPresses.put(keyCode, new Boolean(true));

It generally makes the code easier to read and the logic to follow as return statements can "hide" in code and it's easy to miss them (IMHO)

Third

Rather than something like this...

switch (keyToLowerCaseIfUpperCase(e.getKeyChar())) {
    case 'a': System.out.println("D"); break; // these are just two test cases
    case 'b': System.out.println("E"); break;
    default: break;
}

I would consider using another Map, to map between the virtual key and the value or "callback" you want to call. It will be easier to add new values and reuse the code

I'd also consider using a callback mechanism, which would notify of the KEY_PRESSED and KEY_RELEASED events, so the dispatcher didn't do the conversion itself, but simply managed the notification processes, further separating the implementation logic and allow the dispatcher to be more reusable.

You could achieve the same thing through abstraction, but I think some kind of Observer Pattern would be more pratical (IMHO)

Fourth

Unless you really need to know if a key is not been pressed, I would remove the keyCode from the Map on the KEY_RELEASED event

} else if (e.getID() == KeyEvent.KEY_RELEASED){
    keyPresses.remove(keyCode);
}

As it would simpler to just use something like keyPresses.contains(keyCode) instead of then actually needing to check the value associated with, but this comes down to needs.

You could replace the Map with a Set instead which would be simpler, either the keyCode exists or it doesn't...

\$\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.