HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptModerate

Move function for a game

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
movegamefunctionfor

Problem

I have a move function in this game that I'm making. It works fine, but I'd like to reduce its size a bit.

function move(moveTo, direction) {

    switch (direction) {
        case "right":
            $('#player').animate({ left: "+=" + scale }, "slow", function () {
                $("#player").appendTo("#" + moveTo);$('#player').attr("style", "");
            });break;
        case "left":
            $('#player').animate({ left: "-=" + scale }, "slow", function () {
                $("#player").appendTo("#" + moveTo);$('#player').attr("style", "");
            });break;
        case "up":
            $('#player').animate({ top: "-=" + (9 * (scale / 10)) }, "slow", function () {
                $("#player").appendTo("#" + moveTo);$('#player').attr("style", "");
            });break;
        case "down":
            $('#player').animate({ top: "+=" + (9 * (scale / 10)) }, "slow", function () {
               $("#player").appendTo("#" + moveTo);$('#player').attr("style", "");
            });break;
    }
}


I always execute the same commands:

$("#player").appendTo("#" + moveTo);


and

$('#player').attr("style", "");


I can't put it after the switch statement because I want to wait until the animate is finished. Is there a way to do this other than making a function? I also want this type of thing for some other code, so a bit less case specific would be better.

Solution

I would abstract the animation logic into a high order function and the possible directions in an object, then just lookup the corresponding function for each direction, in other words:

function move(to, dir) {

  function animate(props) {
    return function() {
      $('#player').animate(props, 'slow', function() {
        $(this).attr('style', '').appendTo('#'+ to);
      });
    }
  }

  var dirs = {
    right: animate({ left: '+='+ scale }),
    left: animate({ left: '-='+ scale }),
    up: animate({ top: '-='+ (9*scale/10) }),
    down: animate({ top: '+=' + (9*scale/10) })
  };

  if (dir in dirs) dirs[dir]();
}


I fixed a few other things. You can use $(this) to refer to the element being animated in the callback and chain those methods.

Then (9(scale/10)) is effectively the same as 9scale/10; associative operators.

You probably need to abstract even more if you add more code; ideally you'd want separate all your logic from your DOM manipulation and create a clear data structure that you can reuse. Also think about using classes instead of IDs. Classes are more reusable and lead to simpler code.

Code Snippets

function move(to, dir) {

  function animate(props) {
    return function() {
      $('#player').animate(props, 'slow', function() {
        $(this).attr('style', '').appendTo('#'+ to);
      });
    }
  }

  var dirs = {
    right: animate({ left: '+='+ scale }),
    left: animate({ left: '-='+ scale }),
    up: animate({ top: '-='+ (9*scale/10) }),
    down: animate({ top: '+=' + (9*scale/10) })
  };

  if (dir in dirs) dirs[dir]();
}

Context

StackExchange Code Review Q#31028, answer score: 11

Revisions (0)

No revisions yet.