CODEWITHSUNDEEP
javascript
Chris
Chris

Reputation: 954

Debugging a simple Javascript error

I am trying to move down a div container with Id( topleft) using java script, but my java script code has a problem. Logically, it should work, can someone check?

HTML code

<html>
    <title>Test</title>
    <head>
        <link rel="stylesheet" type="text/css" href="style.css">
        <script type="text/javascript" src="javascript.js"></script>
    </head>
    <body>
        <div  id="container">
            <div onclick="start()" id="topleft"></div>
            <div id="topright"></div>
            <div id="bottomleft"></div>
            <div id="bottomright"></div>
        </div>

    </body>
</html>

CSS code

#container {
    width: 400px;
    height: 400px;
    background-color: red;
    margin: 0 auto;
    position: relative
}
#topright, #topleft,#bottomright, #bottomleft {
    width: 50px;
    height: 50px;
    background-color: black;
    position: absolute;
}
#bottomright, #bottomleft {
    bottom:0;/*175*/
}
#topright, #topleft {
    top:0; /*175*/
}
#bottomleft, #topleft {
    left:0; /*175*/
}
#bottomright, #topright {
    right:0; /*175*/
}

JavaScript code

mt=0;
function move(){
    topleft=document.getElementById("topleft");
    for (n=0;n<=175;n++){
        topp=topleft.style.top;
        topp=parseInt(topp);
        topp++
        topleft.style.top=topp+"px"
    }
}

function start(){
    setInterval("move()",1);
}

Can anybody rectify it?

Upvotes: 0

Views: 74

Answers (2)

T.J. Crowder
T.J. Crowder

Reputation: 1075635

There are several issues with that code, I suspect #4 and #7 are causing you the most immediate problem:

  1. setInterval("move()"1); is a syntax error, and so that would be why it's not running. You need a comma before the 1: (you've edited the question to fix this)

    setInterval("move()", 1);
// Comma -----------^

  2. You're falling prey to The Horror of Implicit Globals by not declaring your mt, n, topleft, and topp variables. Always declare your variables, and declare them in the innermost scope in which they make sense.

  3. Passing strings into setInterval is poor practice; it requires the engine to do an implicit compilation and the scope of that compilation is probably not what you expect it to be (although with your code as quoted — e.g., if move really is a global — it would work). Just use the function reference:

    setInterval(move, 1);

  4. You're using parseInt on topleft.style.top (indirectly), but the topleft element doesn't have any style attribute, and therefore topleft.style.top is "". So you're getting NaN, which isn't useful. The style property on elements does not reflect CSS, it only reflects what's in the style attribute of the element. Look at getComputedStyle (for standards-based browsers) or currentStyle (for older IE).

  5. It's almost always best to use a radix with parseInt, e.g.: parseInt(topp, 10) rather than just parseInt(topp).

  6. FWIW, if the setInterval works, that code will run really, really frequently (though not every millisecond; the specification says engines should treat values below 4 as being 4, and in any case it's subject to the implementation and the single JavaScript UI thread not being busy with other things).

  7. Making a series of changes to elements in a loop without yielding to the browser won't animate those elements. Instead of looping, remember n and move the element just one bit on each call. Each time move returns, that gives the browser a chance to update the UI.

Here's an example with the minimal modifications I'd make (doesn't handle older IE's currentStyle): Live Copy | Live Source

<!doctype html>
<html>
  <title>Test</title>
  <head>
    <style>
      #container {
        width: 400px;
        height: 400px;
        background-color: red;
        margin: 0 auto;
        position: relative
      }
      #topright, #topleft,#bottomright, #bottomleft {
        width: 50px;
        height: 50px;
        background-color: black;
        position: absolute;
      }
      #bottomright, #bottomleft {
        bottom:0;/*175*/
      }
      #topright, #topleft {
        top:0; /*175*/
      }
      #bottomleft, #topleft {
        left:0; /*175*/
      }
      #bottomright, #topright {
        right:0; /*175*/
      }
    </style>
  </head>
  <body>
    <div  id="container">
      <div id="topleft"></div>
      <div id="topright"></div>
      <div id="bottomleft"></div>
      <div id="bottomright"></div>
    </div>
    <script>
      (function() {
        var delay = 10;
        var topleft = document.getElementById("topleft");
        var currentPos = parseInt(getComputedStyle(topleft).top, 10);

        topleft.addEventListener("click", function() {
          // Start
          setTimeout(move, delay);
        });

        function move(){
          if (currentPos <= 175) {
            ++currentPos;
            topleft.style.top = currentPos + "px";
            setTimeout(move, delay);
          }
        }
      })();
    </script>
  </body>
</html>

Notes:

  • I put the code inside a scoping function to avoid creating globals.

  • I put the code at the end, just before the closing </body> tag, so all elements exist as of when it runs.

  • I used a chained series of setTimeout rather than setInterval (optionaly, in this case it's really a matter of preference).

  • Various things from the list at the beginning of the answer.

  • I only make one change per callback, so the browser has time to do the animation.

  • I made the delay between animation frames a variable (delay) and set it to 10. You can use whatever value suits you, note #6 on the list above if you use values below 4.

  • I used addEventListener to hook up the event rather than using an onclick attribute. Using onclick attributes in the markup requires that you create global functions, which isn't generally a good idea. (On older versions of IE -- including IE8 -- you'll have to use attachEvent instead. Or you can do topleft.onclick = function() { ... }; but that's fairly limiting [you can only have a single handler].)

Upvotes: 3

Mykola Prymak
Mykola Prymak

Reputation: 447

As say T.J. Crowder replace the

function start(){
    setInterval("move()"1);
}

to

function start(){
    setInterval(move, 1);
}

Upvotes: -1

Related Questions