1
\$\begingroup\$

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;
    }
 }
\$\endgroup\$
6
  • \$\begingroup\$ Proper code indentation would really help a lot. I can't even really attempt to read it without first fixing the formatting. \$\endgroup\$ – jfriend00 Feb 26 '16 at 21:35
  • 3
    \$\begingroup\$ Welcome to Code Review. Please take a moment to edit your title and question to explain what the code is doing. See How to get the best value out of Code Review - Asking Questions \$\endgroup\$ – Phrancis Feb 26 '16 at 21:38
  • 2
    \$\begingroup\$ Welcome to Code Review! Please fix up your post. No where do you say what your code is doing (a title would be a good place). I also recommend this to improve your question. \$\endgroup\$ – SirPython Feb 26 '16 at 21:38
  • \$\begingroup\$ i think i have seen these same comments on every post... \$\endgroup\$ – Drwooi Poipoi Feb 27 '16 at 18:04
  • \$\begingroup\$ The problem with the if statements is obvious enough. The objection that some users have is likely that your question didn't write a description of the purpose of the code (which is only apparent after seeing the off-site demo). \$\endgroup\$ – 200_success Feb 27 '16 at 18:12
3
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • \$\begingroup\$ thanks! I was also stuck on this. I was thinking I should use a 2d array... but that didn't work out (for me atleast). I will try this out! \$\endgroup\$ – Drwooi Poipoi Feb 27 '16 at 17:56
2
\$\begingroup\$

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;
}
\$\endgroup\$
1
  • \$\begingroup\$ thanks! I like the way you went through your thought process! I'll definitely try that out and use it on my next project. Both comments are incredibly useful for someone starting out! \$\endgroup\$ – Drwooi Poipoi Feb 27 '16 at 17:53

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.