Move function for a game

Question

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.

• Stuart: what do you mean by backwards coding?
• Math chiller: also known as a "hack", kinda like the famous change the drop-down arrow, the point is code that works, but isnt meant to do what its doing. its a common term...
• Stuart: it doesn't look like a hack. You're using animate to do animation. Can you maybe explain what you're trying to do and what the problem is with your current approach?

Answer 1

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 9*scale/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.

• elclanrs: This is just a basic DRY abstraction of your current code. The scale comes from wherever it comes in your actual code, a higher scope I suppose. This should be a drop-in replacement if I took everything into account... A dictionary lookup is a common alternative to a switch statement.
• Math chiller: your right i thought you came up with scale, i still dont want to use a object, even if its common, im asking if you can make a block of code wait till the animate finishes without using .animate(.. , .. , function(){/*code here*/}).
• Blazemonger: Just DRYing up the code a bit more: Replace 9*scale/10 with scale*0.9. You can also skip the op argument as well by making scale a negative number when appropriate.
• Stuart: I think this won't work because you are adjusting the left property even when the movement is supposed to be up or down.
• elclanrs: What is this fourth argument? You have the properties, the speed, and the callback. I don't see any other argument in your code...

Answer 2

$('#player')$('#player')$('#player')

$('#player')$('#player')$('#player')$('#player')
$('#player')$('#player')$('#player')$('#player')

Do not use $('#player') more than once in your code if you intend on fetching data or setting data to the DOM element more than once. Instead save your element to a javascript variable so you can quickly access it. If not, then your jQuery will search through the DOM over and over again.

var playerNode = $('#player');
playerNode.animate(stuff);

Anonymous Functions Are Evil

Notice how

function() {
 $("#player").appendTo("#" + moveTo);
 $('#player').attr("style", "");
}

is exactly the same as

function() {
 $("#player").appendTo("#" + moveTo);
 $('#player').attr("style", "");
}

That means you don't need to write the same line twice. Just create one function object and call itself for each method.

var animCallback = function() {
 $("#player").appendTo("#" + moveTo);
 $('#player').attr("style", "");
};

But wait! There's more! We can clean up that nasty callback because jQuery supports chaining, which allows you to just write one-liners with ease. In fact, while we're at it, we may as well use that variable that holds the player DOM node instead of searching for it again.

var animCallback = function() {
 playerNode.appendTo("#" + moveTo).attr("style", "")
};
playerNode.animate({ left: "+=" + scale }, "slow", animCallback);

If V.S. Case

Now here's a discrepancy. Some people say that switch case is faster than if else. However, you'll notice that newer browsers are focusing on improving the speed of if else by very large amounts. Some browsers are even faster with if else than switch case. Speed isn't the problem here, it's readability. Use an if else statement if there's only 4 possibilities because it makes more sense.

if(direction === 'left') {
 playerNode.animate({ left: "+=" + scale }, "slow", animCallback);
} else if(direction === 'right') {
 playerNode.animate({ right: "+=" + scale }, "slow", animCallback);
} ...

jQuery

You can make a game in your web browser, it'll just be slower than a C++ program. Likewise, you can make a game with jQuery, but it'll be slower than raw Javascript or your own custom libraries. I'm personally okay with jQuery being a setup for a game because it can normalize values that would appear different from browsers. Keep in mind, though, that document.getElementById('player') is about 10 times faster than $('#player').

All Together Now

var playerNode = $(document.getElementById('player'));
function move(moveTo, direction) {
 var animCallback = function() {
 playerNode.appendTo("#" + moveTo).attr("style", "")
 };
 if(direction === 'left') {
 playerNode.animate({ left: "-=" + scale }, "slow", animCallback)
 } else if(direction === 'right') {
 playerNode.animate({ left: "+=" + scale }, "slow", animCallback)
 } else if(direction === 'up') {
 playerNode.animate({ top: "-=" + (9 * (scale / 10)) }, "slow", animCallback)
 } else {
 playerNode.animate({ top: "+=" + (9 * (scale / 10)) }, "slow", animCallback)
 }
}

• Math chiller: 1) document.getElementById('player').animate returns TypeError: document.getElementById(...).animate is not a function [Break On This Error] (its faster because it includes less) 2) i see no reason to use if/else over switch 3) you are right about Anonymous Functions, and next time read the question first - Is there a way to do this other than making a function? 4) i do repeat player, i should replace with playerNode.... but im looking for more then that, i want to have the whole .animate({ left: "+=" + scale }, "slow", also not repeat itself.
• Andrew: (1) Getting an element by its ID gives you a DOM element. Getting it with jQuery returns an array of DOM elements normalized with jQuery. You can wrap document.getElementById('player') inside the jQuery function to get the animate method on it like this: $(document.getElementById('player')). (2) if/else statements make code more readable for other people. It might not be for you, but if somebody has to come up behind your code, then they'll easily understand it. (3) Trust me ;) your code will run faster without the anonymous functions. (4) The only way might be editing jQuery itself.
• Math chiller: (1) show a source that $(document.getElementById('player')) is faster then $("$player") please (2) i dont see how if/else is easier to read then switch (3) what i meant was i know how to do it with function's is there a even better way without multiple function calls (4) ok so not the whole .animate({ left: "+=" + scale }, "slow", but i want to not have to write all of it 4 times, how much can i eliminate and how? also adding a block of sample finished code at the end would be better.
• Andrew: (1) The way you're doing it is 3 times faster than what you were before. (2) It's called syntactic sugar and it's the reason why you use camelCase for your variables. It's just a standard for the programming language. (3) Putting the callback function in the $().animate method doesn't call it. It just references it like an object the same way you reference playerNode. (4) The only way I can think of is to make it default to that within jQuery itself. Not much you can do.
• Math chiller: (1) 230,783 ops/sec will have to do for now... (2) i know what syntactic sugar is, i just dont think if/else is more "sugary" then switch, and i see certain advantages in clarity for switch (3) you are right, but it is defiantly clearer if i could write (the following is pseudo-code) switch(..){case:...case:...} $("#player").done().appendTo("#" + moveTo).attr("style", ""); (4) again i would want something that replaces at least part i.e. .animate({ dir, amount }, "slow", and similar ideas. oh and i +1 for putting so much time in thanks, i hope you solve this and earn the bounty.

Answer 3

I'm not sure why you are set against creating an object to use here. It is really the best option for you here.

But since you asked for this specifically.

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

• Math chiller: thats simple... why in the world didnt i c that?
• nickles80: forest for the trees...
• Math chiller: yup yup, (im a virgo)... happens all the time to me.

Answer 4

You are concerned about the repeated calls to .appendTo() and .attr() but you should also be concerned about your calls .animate() which are very repetitive as well.

Basically you are mapping the four directions to modifications of top and right.

To keep that mapping simple I propose this:

directionMap = 
{
 right : { attribute : "left" , operator: "+=" , scaleMultiplier : 1 },
 left : { attribute : "left" , operator: "-=" , scaleMultiplier : 1 },
 up : { attribute : "top" , operator: "-=" , scaleMultiplier : 0.9 },
 down : { attribute : "top" , operator: "+=" , scaleMultiplier : 0.9 }
}

This mapping should be done once, without knowing your code, I cannot tell you where.

Then, in the move function, you can retrieve the details for the given direction, construct the properties object with the proper css name/value pair.

function move(moveTo, direction) 
{
 var approach = directionMap[direction];
 if( approach )
 {
 var properties = {};
 properties[approach.attribute] = approach.operator + ( scale * approach.scaleMultiplier );
 $('#player').animate( properties , "slow", function () 
 {
 $(this).appendTo("#" + moveTo).attr("style", "");
 }); 
 }
}

The only other thing to note is that this in the callback function is the animated DOM element, and constructing a jQuery selector from an element is faster than constructing it from an ID.

• Math chiller: it isnt multiple calls, js is lazy. the thing is those two are always exactly the same while the .animate changes, your answer is still good.
• Math chiller: i dont want to create a object. i stated that pretty clearly, i want another method, i rather it this way then create a object for this.
• Mathieu Guindon: ...but objects are your friends :)
• Math chiller: ooh yes they are... really i love them (if you check my page -its in my profile- you will c that), its just thati like using them as objects as in real life, a object that is there to be interacted with, not as a place to temporarily store some code, to be used as a switch, to me thats backwards coding, and im trying to make mine straightforward.
• elclanrs: An object is just another data structure. Don't think of objects as classes in JavaScript. People here are trying to give you good advice. Again, an object (or dictionary) look-up is a common alternative to a switch statement, specially in functional programming languages like JavaScript. You can program in JS without if,else,while,for,switch so no side effects.