0

This code runs perfectly in Chrome, but Firefox says the function tile1 is not defined. What could be the problem?

Also, is there any way to shorten this function? I have tried using a for-loop inside tile1 and also an if-else statement but I didn't succeed.

$('div.tile').each(function(index, element) {
  for(var i=0;i<=index;i++){

  var tile1=function(){
    var one ="div.tile div.one";
    var two =" div.tile div.two";
    var three = "div.tile div.three";
    if(index==0){
      one="div.tile div.one";
      two="div.tile div.two";
      three="div.tile div.three";
    } else {
      one ="div.tile div.one"+index;
      two ="div.tile div.two"+index;
      three ="div.tile div.three"+index;
    }
      var $one=$(one);
      var $two = $(two);
      var $three=$(three);
      var oneTop = $one.top;
      var twoTop = $two.top;
      var threeTop = $three.top;
      delayRate += 3000; // delayRate 5 sec (5000) by default 

     $one
       .delay(delayRate)
       .animate({top: "-100.5%"},300,easing);   
     $two
       .delay(delayRate)
       .animate({top:"0%"},300,easing);
     $three
       .delay(delayRate)
       .animate({top:"100.5%"},300,easing);     

     $one
       .delay(12000)
       .animate({top: "-200.5%"},300,easing);
     $two
       .delay(12000)
       .animate({top:"-100.5%"},300,easing);
     $three
       .delay(12000)
       .animate({top:"0"},300,easing);

     $one
       .delay(12000)
       .animate({top: "-100.5%"},300,easing);
     $two
       .delay(12000)
       .animate({top:"0"},300,easing);
     $three
       .delay(12000)
       .animate({top:"100.5%"},300,easing);

     $one
       .delay(15000-delayRate)
       .animate({top: "0"},300,easing);
     $two
       .delay(15000-delayRate)
       .animate({top:"100.5%"},300,easing);
     $three
       .delay(15000-delayRate)
       .animate({top:"200.5%"},300,easing);

     if(i==3){
       delayRate=0;
     }
   }
 }  
 window.setInterval(tile1, 3000);
});

As I called the function the index comes randomly like 0,3,1,2, and there are 4 divs that the index corresponds to.

halfer
  • 18,701
  • 13
  • 79
  • 158
Ankit Ladhania
  • 1,005
  • 1
  • 12
  • 19
  • 1
    Actually can you clarify whether you want to create a whole bunch of functions or just one `tile1`? Thanks. And if you only want one, then what is the value of `i` you want it to have? – Ray Toal Jun 21 '13 at 05:21
  • possible duplicate of [Javascript function cannot be found](http://stackoverflow.com/questions/15573202/javascript-function-cannot-be-found) – Bergi Jun 21 '13 at 05:40
  • what can i do to remove the index problem – Ankit Ladhania Jun 21 '13 at 06:08

1 Answers1

6

The use of function statements in JavaScript is discouraged. Check out Mozilla's page on function scope which has a great section on function statements vs. function expressions, and states:

Functions can be conditionally defined using either //function statements// (an allowed extension to the ECMA-262 Edition 3 standard) or the Function constructor. Please note that such function statements are no longer allowed in ES5 strict. Additionally, this feature does not work consistently cross-browser, so you should not rely on it.

The fact that you are seeing differences between browsers with this code is not surprising.

Try

var tile1 = function () {
    ...
}

While this should work for you here, it does so only because variable definitions with var are hoisted. As JavaScript evolves and we start using let instead of var, your use of tile1 in the setInterval call outside the loop in which tile1 is defined won't work.

One of the problems that can occur is that when you use i inside the inner function, you are always referring to the single instance of i in the outer scope (the loop counter), whose value is always equal to index. (EDIT: I show how to fix this below.)

Always be very, very careful when defining functions inside a loop. You really need to understand closures and hoisting and related concepts. Is there any way that tile1 can be defined globally, outside the loop?

Regarding your question on simplifying the code structure, I think you would be okay with defining tile1 with var, but I don't think you need that inner loop with i. Try:

$('div.tile').each(function(index, element) {
  var tile1 = function () {
    var one ="div.tile div.one";
    .
    .
    .
    if (index === 3) {   // CHANGED I TO INDEX HERE.
      delayRate=0;
    }
  }
  window.setInterval(tile1, 3000);
});

I'm not sure what the inner loop was buying you.

ASIDE: There is an effort underway for future JavaScript versions to deal with function statements in a block scope; you can see here that this is supported by certain versions of Chrome, but not Firefox.

ADDENDUM

Okay now I see that you want to cycle three steps in your tile1 function. While it is possible to put a for-loop inside the function, the JavaScript way is to have the function just run one step of its animation each time. If it needs some kind of counter, the counter should be external. One way to do this is like this:

var tile1 = function (i) {
  .
  .
  // use the value i here as needed
  .
  .
  setTimeout(function () {tile1((i + 1) % 3)}, 3000);
};
tile1(0);

What this does is it first calls your tile function with the value 0. Then after you do what you want at 0, you will schedule the next frame to run 3 seconds later with the i = 1. Then three seconds or so later with 2, then three seconds or so later with 0.

There is a little drift here, so you might want to use setInterval. This requires a closure. The form of the solution is this:

(function () {
  var i = 0;
  var tile1 = function () {
    .
    .
    // use the value i here as needed
    .
    .
    i = (i + 1) % 3;
  };
  setInterval(tile1, 3000);
}());

This code is pretty cool. It is a call of an anonymous function which calls setInterval to schedule the tile1 function to run every 3 seconds. Each time tile1 runs it uses the value of a non-local i which is hidden from the rest of the code with the closure. Each execution of tile1 uses the right value of i, then finishes by changing i to the proper value for its next invocation!

Both of these techniques are good to master in JavaScript. Have fun with them. The second one, of course, has no clock drift so it is probably better. To professionalize the code, you might want to assign the result of setInterval to a variable so you can call clearInterval at a later time.

Ray Toal
  • 79,229
  • 13
  • 156
  • 215
  • and ray what about the 2nd question that i asked about the loop.. thanks for your help – Ankit Ladhania Jun 21 '13 at 04:55
  • Okay I will work on that and edit the answer. In the meantime see [this SO question](http://stackoverflow.com/questions/7652546/defining-functions-inside-of-a-loop) which deals with the old JavaScript functions defined inside a loop alternatives. – Ray Toal Jun 21 '13 at 04:56
  • i tried to use the offset function in the script so that i don't have to define the % from top but that didn't work mate is there a way that i could use some variable to define the top of the tiles. – Ankit Ladhania Jun 21 '13 at 05:00
  • @ArunPJohny True but function statements don't work reliably cross-browser. From [this page](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions_and_function_scope): "Please note that such function statements are no longer allowed in ES5 strict. Additionally, this feature does not work consistently cross-browser, so you should not rely on it." I'm not surprised the OP is seeing different behaviors in different browsers. – Ray Toal Jun 21 '13 at 05:05
  • 1
    @RayToal AFAIK it is not a block level function scope because `tile1` is defined in the scope of the click handler and the setTimeout accepts a function reference as the parameter – Arun P Johny Jun 21 '13 at 05:07
  • Ah, fair enough (+1). However, it is (unless I am mistaken) a [function statement](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions_and_function_scope#The_function_declaration_.28function_statement.29), which is known not to be handled consistently cross-browser. – Ray Toal Jun 21 '13 at 05:13
  • hey ray can u pls help me with what i asked in the comment thanks – Ankit Ladhania Jun 21 '13 at 05:24
  • if you have notice that i have 4 tiles on which i run the animation and each tile has 3 images in it and the after 3sec change the images position so i tried to do it in a very long method of defining the top of every image.. i used the "i" for calling the tiles i named the tiles like div.one calls the first tile dive.one1 calls the 2nd tile and so on – Ankit Ladhania Jun 21 '13 at 05:39
  • Well actually no, I did not notice there were _four_ tiles but that doesn't really matter - you properly did an `each` on `div.tile`. But what you are saying is that each time you call `tile1` you want a new value for `i`? First 0 then 1 then 2 then 0 then 1 then.... Sorry I can't quite figure out the intent. – Ray Toal Jun 21 '13 at 05:45
  • thanks mate u helped alot i was my mistake should have defined the for loop inside the function thanks alot – Ankit Ladhania Jun 21 '13 at 05:50
  • let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/32124/discussion-between-ankit-ladhania-and-ray-toal) – Ankit Ladhania Jun 21 '13 at 06:02