Why this very simple javascript/jquery code won't work correctly

Question

I have a very simple JavaScript/jquery code which won't just work correctly. The problem seems to be that positioning of the div with id 'circle' does not seem to get calculated when the loop is run. Just need to know why the problem is occurring and if any fixes available.

Here is the link http://jsfiddle.net/TDRyS/ Basically what I am trying to do is trying to move the ball up once it is at a distance of 500px from the top.

The code:

var maxa = document.width;
var maxb = document.height;
$(document).ready(function() {

    dothis();
});

function dothis() {
    var left = parseInt(document.getElementById('circle').style.left);
    var top = parseInt(document.getElementById('circle').style.top);
    if (top >= 500) {
        $("#circle").animate({
            top: "-=" + Math.floor(Math.random() * 100 + 1) + "px",
            left: "-=" + Math.floor(Math.random() * 100 + 1) + "px"
        }, 1000);
    }
    else {
        $("#circle").animate({
            top: "+=" + Math.floor(Math.random() * 100 + 1) + "px",
            left: "+=" + Math.floor(Math.random() * 100 + 1) + "px"
        }, 1000);

    }

    dothis();
}

The jsfiddle appears to be something of a DOS attack. I can't bring it up without my browser hanging. — Pointy
– Pointy, Commented Jun 24, 2012 at 16:06
OK it works in chrome. Well one problem is that iterating by tail recursion like that in JavaScript is very likely to make your program run out of stack space and fail. — Pointy
– Pointy, Commented Jun 24, 2012 at 16:09

gdoron · Accepted Answer · 2012-06-24 16:20:48Z

3

You need to setTimeout before calling the function again\ pass the function as a callback

Live DEMO

Full code:

var maxa = document.width;
var maxb = document.height;
var up = false;
$(document).ready(function() {

    doThis();
});

function doThis() {

    var left = parseInt(document.getElementById('circle').style.left);
    var top = parseInt(document.getElementById('circle').style.top);
    if (top < 50) {
        up = false;
    }
    if (top >= 500 || up) {

        up = true;
        $("#circle").animate({
            top: "-=" + Math.floor(Math.random() * 100 + 1) + "px",
            left: "-=" + Math.floor(Math.random() * 100 + 1) + "px"
        }, 500, doThis);
    }
    else {
        $("#circle").animate({
            top: "+=" + Math.floor(Math.random() * 100 + 1) + "px",
            left: "+=" + Math.floor(Math.random() * 100 + 1) + "px"
        }, 500, doThis);

    }
}

edited Jun 24, 2012 at 16:20

answered Jun 24, 2012 at 16:09

gdoron

151k59 gold badges302 silver badges375 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Pointy Over a year ago

... or use the function as a callback from the ".animate()" routine itself.

Pointy Over a year ago

No. He just has to pass "dothis" as the third parameter to the "animate" function; no need to create a new function. It's no more a DRY violation than it is to repeat all those random calculations or the 500 :-) If we're really trying to eliminate repeated code, there's a lot of revising to do!

gdoron Over a year ago

@Pointy. I appreciate your wise opinion, I changed my code. thanks.

Pointy · Accepted Answer · 2012-06-24 16:15:09Z

1

There are several problems with this:

You can't read style information that comes from CSS from the "style" property of a DOM element. You're using jQuery anyway; it can give you the style information.
The calls to "animate" do not complete synchronously. You can pass "dothis" as a third parameter, and jQuery will call your function when the animation completes.

Here is a working version.

answered Jun 24, 2012 at 16:15

Pointy

415k62 gold badges600 silver badges631 bronze badges

3 Comments

manojadams Over a year ago

Hi pointy, thanks for you help. I know that my code is not the most optimised one. It's just been something I am trying to do casually but you said "You can't read style information that comes from CSS from the "style" property of a DOM element. " I am quite not sure here.

gdoron Over a year ago

+1, I didn't see your answer or else I wouldn't answer my self. But in your script the ball is stuck near the 500px. I added a test to see which direction the ball should go- up\down, though I didn't put to much effort making it effective nor DRY.

Pointy Over a year ago

@gdoron yes that makes it make a little more sense; it looked like a "work in progress" maybe ...

Cranio · Accepted Answer · 2012-06-24 16:10:02Z

To calculate left and top you can also use:

$("#circle").position().left // or .top (relative to parent object)

$("#circle").offset().left // or .top (relative to window)

Anyway the code seems wrong to me, because you are continuosly calling "animate"; you don't let any time for an animation to "run" before calling another animation (and do a nasty function recursion if top>500px).

What you have to do is - wait for the animation to complete, using a time delay or using some event - use a timer to check every n milliseconds if top>500. If so, STOP the current animation and start a new one in the opposite direction

Collectives™ on Stack Overflow

Why this very simple javascript/jquery code won't work correctly

3 Answers 3

3 Comments

3 Comments

Comments

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

3 Comments

3 Comments

Comments

Related