Keyboard handler to move a shape in response to arrow keys

Question

I finished Codeacademy and I'm looking to practice and get better at JavaScript. Is this coded correctly or should I have made a function for it somehow? The purpose of this code is to move a square around the page with arrow keys or buttons.

I have the entire script hosted here.

I mostly want to "Simplify this chain of if statements" but also the post about using a map instead of the if statements is something I needed to know.

function anim(e){
    if((e.keyCode === 37)||(e === 37)){
      y = shipLeft;
      shipLeft -= 11;
      y -= 11;
      y.toString();
      y = y + 'px';
      ship.style.left = y;
      changeColor();
    return shipLeft
}
    else if ((e.keyCode === 39) || (e === 39)){
      y = shipLeft;
      shipLeft += 11;
      y += 11;
      y.toString();
      y = y + 'px';
      ship.style.left = y;
      changeColor();
    return shipLeft
    }
    else if ((e.keyCode === 40) || (e === 40)){
      y = shipTop;
      y += 11;
      shipTop += 11;
      y.toString();
      y = y + 'px';
      ship.style.top = y;
      changeColor();
    return shipTop;
    }
    else if ((e.keyCode === 38) || ( e === 38)){
      y = shipTop;
      y -= 11;
      shipTop -= 11;
      y.toString();
      y = y + 'px';
      ship.style.top = y;
      changeColor();
    return shipTop;
    }
 }

Answer 1

If you move shipLeft and shipTop into an object called shipPositions rather than just top level variables, you could also use a map-driven approach like this:

function anim(e) {
    var key = e.KeyCode || e, val, info;
    var keyMap = {
        "37": {direction: "left", ship: "shipLeft", delta: -11},
        "39": {direction: "left", ship: "shipLeft", delta: 11},
        "40": {direction: "top", ship: "shipTop", delta: 11},
        "38": {direction: "top", ship: "shipTop", delta: -11},

    };
    info = keyMap[key];
    if (!info) {
        return;
    }
    shipPositions[info.ship] += info.delta;
    val = shipPositions[info.ship];
    ship.style[info.direction] = val + 'px';
    changeColor();
    return val;
}

Answer 2

Basically, each branch of your if/else does the following:

1. Update the appropriate shipXXX variable with an appropriate increment.
2. Set the appropriate ship.style.xxx value.
3. Call changeColor().
4. return the newly modified value.

The last three steps are pretty much the same for all branches once you have the value and know the style name. So, you can move those three steps to a common place and do them once and just set up the appropriate state in the if/else.

Also, this structure:

(e.keyCode === 37)||(e === 37)

Is repeated each time. Since there can't be a case where e is an object that has a .KeyCode property and where e ==== 37, then you can figure out once before all the comparisons whether there is a .KeyCode property or not and just use that knowledge. This also simplifies the comparisons.

I'm also assuming that the variable y in your code is just something you are temporarily using and not something you're really trying to set (it appears to be an accidental global and is not being used elsewhere so I've removed it).

You can extract out some of the repeated code like this:

function anim(e) {
    var direction, val, key = e.KeyCode || e;
    if (key === 37) {
        direction = "left";
        shipLeft -= 11;
        val = shipLeft;
    } else if (key === 39) {
        direction = "left";
        shipLeft += 11;
        val = shipLeft;
    } else if (key === 40) {
        direction = "top";
        shipTop += 11;
        val = shipTop;
    } else if (key === 38) {
        direction = "top";
        shipTop -= 11;
        val = shipTop;
    } else {
        return;
    }
    ship.style[direction] = val + 'px';
    changeColor();
    return val;
}