current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 5 down vote favorite

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.

share|improve this question
    
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)... –  MadProgrammer Feb 15 at 23:38
    
@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.". –  Emz Feb 15 at 23:46
    
I'm still not sure what it's you're hoping to achieve though, what is the problem you are trying to overcome? –  MadProgrammer Feb 15 at 23:47
    
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. –  Emz Feb 15 at 23:52
    
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 :)) –  MadProgrammer Feb 16 at 0:00

1 Answer 1

up vote 5 down vote accepted

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

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.