Broken closure - please help me fix it_问答_开发者

in a related question I have posted this code. It almost works, but the counter doesn't.

Can we fix it? (no jQuery, please)

<script type="text/javascript">
var intervals = [];
var counters = {
  "shoes":{"id":"shoe1","minutes":1,"seconds":5},
  "trousers":{"id":"trouser1","minutes":10,"seconds":0}
}; // generate this on the server and note there is no comma after the last item
window.onload = function() {
  for (var el in counters) { countdown(counters[el]) };
}

function countdown(element) {
    intervals[element.id] = setInterval(function() {
        var el = document.getElementById(element.id);
        var minutes = element.minutes;
        var seconds = element.seconds;
        if(seconds == 0) {
开发者_运维技巧            if(minutes == 0) {
                el.innerHTML = "countdown's over!";                    
                clearInterval(intervals[element.id]);
                return;
            } else {
                minutes--;
                seconds = 60;
            }
        }
        if(minutes > 0) {
            var minute_text = minutes + (minutes > 1 ? ' minutes' : ' minute');
        } else {
            var minute_text = '';
        }
        var second_text = seconds > 1 ? 'seconds' : 'second';
        el.innerHTML = minute_text + ' ' + seconds + ' ' + second_text + ' remaining';
        seconds--;
    }, 1000);
}
</script>
shoes: <span id="shoe1"></span><br />
trousers: <span id="trouser1"></span><br />

You just need to take the minutes and seconds variable declarations out of the inner function, like this:

function countdown(element) {
    var minutes = element.minutes;
    var seconds = element.seconds;

    intervals[element.id] = setInterval(function() {
        var el = document.getElementById(element.id);
        if(seconds == 0) {
            if(minutes == 0) {
                el.innerHTML = "countdown's over!";                    
                clearInterval(intervals[element.id]);
                return;
            } else {
                minutes--;
                seconds = 60;
            }
        }
        if(minutes > 0) {
            var minute_text = minutes + (minutes > 1 ? ' minutes' : ' minute');
        } else {
            var minute_text = '';
        }
        var second_text = seconds > 1 ? 'seconds' : 'second';
        el.innerHTML = minute_text + ' ' + seconds + ' ' + second_text + ' remaining';
        seconds--;
    }, 1000);
}

When you call countdown, you want to fetch the initial values for each countdown and then slowly decrease them (closure makes sure that the values will stay available as long as the anonymous function requires them). What you were doing before was reset the countdown values at the start of each tick, so the countdown never had a chance to... well, count down.

Update:

If you need to update the values inside window.counters while the countdowns are active (although I don't see why you would want to do that; if you want to do anything meaningful with the "current" countdown values, just do it inside the anonymous function), you can simply add this at the end:

var second_text = seconds > 1 ? 'seconds' : 'second';
el.innerHTML = minute_text + ' ' + seconds + ' ' + second_text + ' remaining';
seconds--;

// ADD THIS:
element.minutes = minutes;
element.seconds = seconds;

Please check this

<html>
    <body>
        <script type="text/javascript">
        var intervals = [];
        var counters = {
          "shoes":{"id":"shoe1","minutes":1,"seconds":5},
          "trousers":{"id":"trouser1","minutes":10,"seconds":0}
        }; // generate this on the server and note there is no comma after the last item
        window.onload = function() {
          for (var el in counters) { countdown(counters[el]) };
        }

        function countdown(element) {
            intervals[element.id] = setInterval(function() {
                var el = document.getElementById(element.id);
                var minutes = element.minutes;
                var seconds = element.seconds;

                if(seconds == 0) {
                    if(minutes == 0) {
                        el.innerHTML = "countdown's over!";                    
                        clearInterval(intervals[element.id]);
                        return;
                    } else {
                        minutes--;
                        seconds = 60;
                    }
                }
                if(minutes > 0) {
                    var minute_text = minutes + (minutes > 1 ? ' minutes' : ' minute');
                } else {
                    var minute_text = '';
                }
                var second_text = seconds > 1 ? 'seconds' : 'second';
                el.innerHTML = minute_text + ' ' + seconds + ' ' + second_text + ' remaining';
                seconds--;

                element.seconds = seconds;
                element.minutes = minutes;
            }, 1000);
        }
        </script>
        shoes: <span id="shoe1"></span><br />
        trousers: <span id="trouser1"></span><br />
    </body>
</html>

Working solution is here.

EDIT: There was a minor issue with the script. I've fixed it.

After recalculating the seconds and minutes you have to set the new values back in the element object.

You're decrementing the wrong variable inside your interval callback. Instead of doing seconds-- and minutes-- you need to reference the element members.

intervals[element.id] = setInterval(function() {
    var el = document.getElementById(element.id);

    if(element.seconds == 0) {
        if(element.minutes == 0) {
            el.innerHTML = "countdown's over!";                    
            clearInterval(intervals[element.id]);
            return;
        } else {
            element.minutes--;
            element.seconds = 60;
        }
    }
    if(element.minutes > 0) {
        var minute_text = element.minutes + (element.minutes > 1 ? ' minutes' : ' minute');
    } else {
        var minute_text = '';
    }
    var second_text = element.seconds > 1 ? 'seconds' : 'second';
    el.innerHTML = minute_text + ' ' + element.seconds + ' ' + second_text + ' remaining';
    element.seconds--;
}, 1000);

All of the answers you've been given so far fail to handle one simple fact: setInterval does not happen reliably at the time you set. It can be delayed or even skipped if the browser is busy doing something else. This means you can't rely on your update function to decrement the number of seconds left, you'll start drifting from reality very quickly. Maybe that's okay, maybe not. There's no need for it, in any case: Just calculate when the timeout occurs (e.g., a minute and five seconds from now for your "shoes" counter), and then at each update, calculate how long you have left. That way if an interval got dropped or something, you won't drift; you're deferring the computer's clock.

Here's a procedural version:

// Note that to prevent globals, everything is enclosed in
// a function. In this case, we're using window.onload, but
// you don't have to wait that long (window.onload happens
// *very* late, after all images are loaded).
window.onload = function() {

  // Our counters
  var counters = {
    "shoes":{
      "id": "shoe1",
      "minutes": 1,
      "seconds":5
    },
    "trousers":{
      "id": "trouser1",
      "minutes": 10,
      "seconds":0
    }
  };

  // Start them
  var name;
  for (name in counters) {
    start(counters[name]);
  }

  // A function for starting a counter
  function start(counter) {
    // Find the time (in ms since The Epoch) at which
    // this item expires
    counter.timeout = new Date().getTime() +
                      (((counter.minutes * 60) + counter.seconds) * 1000);

    // Get this counter's target element
    counter.element = document.getElementById(counter.id);
    if (counter.element) {
      // Do the first update
      tick(counter);

      // Schedule the remaining ones to happen *roughly*
      // every quarter second. (Once a second will look
      // rough).
      counter.timer = setInterval(function() {
        tick(counter);
      }, 250);
    }
  }

  // Function to stop a counter
  function stop(counter) {
    if (counter.timer) {
      clearInterval(counter.timer);
      delete counter.timer;
    }
    delete counter.element;
  }

  // The function called on each "tick"
  function tick(counter) {
    var remaining, str;

    // How many seconds left?
    remaining = Math.floor(
      (counter.timeout - new Date().getTime()) / 1000
    );

    // Same as last time?
    if (remaining != counter.lastRemaining) {
      // No, do an update
      counter.lastRemaining = remaining;
      if (remaining <= 0) {
        // Done! Stop the counter.
        str = "done";
        alert("Stopped " + counter.id);
        stop(counter);
      }
      else {
        // More than a minute left?
        if (remaining >= 120) {
          // Yup, output a number
          str = Math.floor(remaining / 60) + " minutes";
        }
        else if (remaining >= 60) {
          // Just one minute left
          str = "one minute";
        }
        else {
          // Down to seconds!
          str = "";
        }

            // Truncate the minutes, just leave seconds (0..59)
        remaining %= 60;

        // Any seconds?
        if (remaining > 0) {
          // Yes, if there were minutes add an "and"
          if (str.length > 0) {
            str += " and ";
          }

          // If only one second left, use a word; else, 
          // a number
          if (remaining === 1) {
            str += "one second";
          }
          else {
            str += Math.floor(remaining) + " seconds";
              }
        }

        // Finish up
        str += " left";
      }

      // Write to the element
      counter.element.innerHTML = str;
    }
  }

};

Live example

Here's an OOP version (using the module pattern so Counter can have named functions and a private one [tick]):

// A Counter constructor function
var Counter = (function() {
  var p;

  // The actual constructor (our return value)
  function Counter(id, minutes, seconds) {
    this.id = id;
    this.minutes = minutes || 0;
    this.seconds = seconds || 0;
  }

      // Shortcut to the prototype
  p = Counter.prototype;

  // Start a counter
  p.start = Counter_start;
  function Counter_start() {
    var me = this;

    // Find the time (in ms since The Epoch) at which
    // this item expires
    this.timeout = new Date().getTime() +
                      (((this.minutes * 60) + this.seconds) * 1000);

    // Get this counter's target element
    this.element = document.getElementById(this.id);
    if (this.element) {
      // Do the first update
      tick(this);

      // Schedule the remaining ones to happen *roughly*
      // every quarter second. (Once a second will look
      // rough).
      this.timer = setInterval(function() {
        tick(me);
      }, 250);
    }
  }

  // Stop a counter
  p.stop = Counter_stop;
  function Counter_stop() {
    if (this.timer) {
      clearInterval(this.timer);
      delete this.timer;
    }
    delete this.element;
  }

  // The function we use to update a counter; not exported
  // on the Counter prototype because we only need one for
  // all counters.
  function tick(counter) {
    var remaining, str;

    // How many seconds left?
    remaining = Math.floor(
      (counter.timeout - new Date().getTime()) / 1000
    );

    // Same as last time?
    if (remaining != counter.lastRemaining) {
      // No, do an update
      counter.lastRemaining = remaining;
      if (remaining <= 0) {
        // Done! Stop the counter.
        str = "done";
        alert("Stopped " + counter.id);
        stop(counter);
      }
      else {
        // More than a minute left?
        if (remaining >= 120) {
          // Yup, output a number
          str = Math.floor(remaining / 60) + " minutes";
        }
        else if (remaining >= 60) {
          // Just one minute left
          str = "one minute";
        }
        else {
          // Down to seconds!
          str = "";
        }

        // Truncate the minutes, just leave seconds (0..59)
        remaining %= 60;

        // Any seconds?
        if (remaining > 0) {
          // Yes, if there were minutes add an "and"
          if (str.length > 0) {
            str += " and ";
          }

          // If only one second left, use a word; else, 
          // a number
          if (remaining === 1) {
            str += "one second";
          }
          else {
            str += Math.floor(remaining) + " seconds";
          }
        }

        // Finish up
        str += " left";
      }

      // Write to the element
      counter.element.innerHTML = str;
    }
  }

  // Return the constructor function reference. This
  // gets assigned to the external var, which is how
  // everyone calls it.
  return Counter;
})();

// Note that to prevent globals, everything is enclosed in
// a function. In this case, we're using window.onload, but
// you don't have to wait that long (window.onload happens
// *very* late, after all images are loaded).
window.onload = function() {

  // Our counters
  var counters = {
    "shoes":    new Counter("shoe1", 1, 5),
    "trousers": new Counter("trouser1", 10, 0)
  };

  // Start them
  var name;
  for (name in counters) {
    counters[name].start();
  }

};

Live example

More about closures here.

I think it'd make your code a lot cleaner and save you some ifs if you would keep the timeout in code as seconds, rather than minutes and seconds:

var intervals = [];
var counters = {
  "shoes":{"id":"shoe1","seconds":65},
  "trousers":{"id":"trouser1","seconds":600}
}; // generate this on the server and note there is no comma after the last item

window.onload = function() {
  for (var el in counters) { countdown(counters[el]) };
}

function countdown(element) {
    intervals[element.id] = setInterval(function() {
        var el = document.getElementById(element.id);

        if(element.seconds == 0) {
            el.innerHTML = "countdown's over!";                    
            clearInterval(intervals[element.id]);
            return;
        }

        var minutes = (element.seconds - (element.seconds % 60)) / 60;
        if(minutes > 0) {
            var minute_text = minutes + (minutes > 1 ? ' minutes' : ' minute');
        } else {
            var minute_text = '';
        }

        var second_text = (element.seconds%60) > 1 ? 'seconds' : 'second';
        el.innerHTML = minute_text + ' ' + (element.seconds%60) + ' ' + second_text + ' remaining';

        element.seconds--;

    }, 1000);
}

(I'd post as a comment if it weren't for all the code...)