1
\$\begingroup\$

I'm having trouble deciding if I should leave the default: break; in the switch statement or not. It does kinda show intent, i.e. that most keys are not handled, but it's also effectively useless code. I feel like it's clear enough anyway, but maybe being explicit is better?

function handleKeyDownEvent(event) {
    switch (event.keyCode) {
        case 87: // w
            player.dy = -50
            break
        case 65: // a
            player.dx = -50
            break
        case 83: // s
            player.dy = 50
            break
        case 68: // d
            player.dx = 50
            break
        default:
            break
    }
}

Maps seem to be the preferred way of doing this, instead of a switch statement. How does the movement map work with the corresponding handleKeyUpEvent which works the same way, except that it zeroes the dx or dy values?

Should I keep the default: break; or not?

\$\endgroup\$
  • \$\begingroup\$ Usually is a good practice to put a break sentence on every case. I was going to develop a bit more on why but I found a post that resumes it quite well. \$\endgroup\$ – A. Romeu Mar 8 '16 at 22:33
1
\$\begingroup\$

Your code is rather short, but can still be greatly improved.


Should I keep the default: break; or not?

Do not keep it. Since you aren't doing anything for the default case, there is no point in having it. You can remove that entire thing.


Map

In order to improve the readability of your code (and overall efficiency), I recommend creating a map that maps the key code values to the changes needed to be made to dx and dy. Here is what I mean:

var keyToChangeMap = {
    87: [0, -50], // [change in x, change in y]
    65: [-50, 0]
    ...
};

Now, you have an \$O(1)\$ method for finding out how to change the dx and dy values. This is also flexible: you can easily change the values that x and y change by, and you can easily add more keys to listen to.

Then, in your function, simply use the key code to get a value from the map:

function handleKeyDownEvent(event) {
    var keyCode = event.keyCode;
    if(keyToChangeMap.hasOwnProperty(keyCode)) {
        var change = keyToChangeMap[keyCode];
        player.dx += change[0];
        player.dy += change[1];
    }
}

Magic numbers

You have some magic numbers in your code. It's pretty difficult to understand these numbers, so you should create constants to represent these values.

For example,

var W_KEY = 87;
var A_KEY = 65;

You don't even have to be that specific. You can do something like this:

var UP_KEY = 87;
var LEFT_KEY = 65;

which is even more readable.

\$\endgroup\$
1
\$\begingroup\$

In this case, I think that default: would be noise. It's obvious that this code makes no attempt to handle every possible keycode.

I suggest using a variant of @SirPython's suggestion to use a lookup table. It can be written in a way that reminds you of the keyboard layout.

var MOVEMENT = {    /* W */ 87: { dy: -50 },
  /* A */ 65: { dx: -50 },              /* D */ 68: { dx: +50 },
                    /* S */ 83: { dy: +50 },
};

function handleKeyDownEvent(event) {
    var movement;
    if (movement = MOVEMENT[event.keyCode]) {
        player.dx += (movement.dx || 0);
        player.dy += (movement.dy || 0);
    }
}
\$\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.