Javascript setTimout within an object/function help

Question

I'm new to closures and have a 'gets me by doing most things' understanding of javascript and so am wondering how I can improve etc on the following which was me trying to have an object that has a counter in it...trying to improve/further my understanding.

edit: the following code works, sure...but likely it is wrong (is it?)...I don't have even close to an idea of whether it is code that is correct or incorrect...where can i improve...is there a better way to have a timer in an object/function?

function myObj() {
    this.outputId = 'div_output1';
    this.counter = 0;
    this.limit = 10;
    this.count = function() {
     // reference to self
     var me = this;

     if (me.doStuff(me)) setTimeout(function() {
      me.count();
     },1000);
    };
    this.doStuff = function(me) {
     if (me.counter >= me.limit) {
      document.getElementById(me.outputId).innerText = 'count reached limit';
      return false;

     } else {
      document.getElementById(me.outputId).innerText = 'count = ' + me.counter;
      me.counter += 1;
      return true;
     }
    }
}

// example usage of the object...

window.onload = function() {

    var x = new myObj;
    x.outputId = 'div_output2';
    x.count();

    var y = new myObj;
    y.limit = 8;
    y.count();
}

Answer 1

I would use the closure over this just for the function call in the callback function. The other methods of the object can use this as usual. The callback needs an explicit reference to the object because it needs to call the objects method "from the outside". But for the called method it is just a normal function call and it can use the implicit this to access it's object, just as usual.

I also normally move the method declaration out of the constructor into the objects prototype because it's clearer and more efficient:

function myObj() {
    this.outputId = 'div_output1';
    this.counter = 0;
    this.limit = 10;
}

myObj.prototype.count = function() {
    // reference to self
    var me = this;
    var callback = function() {
        me.count();
    };

    if (this.doStuff())
        setTimeout(callback,1000);
}

myObj.prototype.doStuff = function() {
    if (this.counter >= this.limit) {
            document.getElementById(this.outputId).innerText = 'count reached limit';
            return false;

    } else {
            document.getElementById(this.outputId).innerText = 'count = ' + this.counter;
            this.counter += 1;
            return true;
    }
}

Answer 2

Some of the ways I would clean up the code are:

1. Use encapsultion i.e. Don't just make all your variables & functions public. You have the doStuff method as public on that object which means anyone could call that not just the count method. This also applies to the count, limit and outputId. Instead they should be passed to the constructor.

2. Make the reference object "me" a private member of the class so you don't set it in the method. You could potentially have a problem the way you do it now if you were to call the method using call or apply because the context of this will have changed.

e.g.

var obj = new myObj();
var otherobj = {};
obj.call.call(otherobj);

1. I would also change that inline if you have in count. Your just asking for trouble!!!

   if (doStuff()) setTimeout(function() { me.count(); },1000);

Here is the code with all of my suggestions

function myObj(outputId, limit) {
    var counter = 0,
        me = this;

    function doStuff() {
        if (counter >= limit) {
                document.getElementById(outputId).innerText = 'count reached limit';
                return false;

        } else {
                document.getElementById(outputId).innerText = 'count = ' + counter;
                counter += 1;
                return true;
        }
    }
    this.count = function() {
        if (doStuff()) {
            setTimeout(function() {
                me.count();
            },1000);
        }
    };
}

Answer 3

I don't see any real problems with it, but there are a few things I would probably clean up.

• I would specify the outputId and limit as parameters to the constructor. If you still want to have default values, you could check the arguments for undefined, or pass an options object.
• The me parameter in doStuff is redundant, since it's always the same as this, and you don't need to capture it in a closure.

• I would probably write doStuff like this:

  var outputDiv = document.getElementById(this.outputId);
  this.doStuff = function() {
      this.counter++;
      if (this.counter > this.limit) {
          outputDiv.innerText = 'count reached limit';
          return false;
      }
      else {
          outputDiv.innerText = 'count = ' + this.counter;
          return true;
      }
  }
  

  It's a minor style thing, but I think it's more obvious that doStuff is modifying the counter, since it's at the beginning of the function instead of buried inside the else clause. Also, moving the getElementById outside the function reduces duplicated code slightly.

Answer 4

You are using closure correctly. Because when setTimeout calls your function, the 'this' will be 'Window' object and you have to create a closure (which you did by assigning 'this' to me) and accessing it.

Anyway, I would still write your code a little differently. I would make doStuff call itself instead of making it return true/false and then deciding whether to call doStuff again or not.

I don't like how you are passing the 'this' object to this.doStuff function. There is no need for that. To understand how 'this' works in JavaScript, check my detailed answer on the subject.

function Counter(opts)
{
    this.outputId = opts.outputId || 'div_output1';
    this._currentCount = 0;
    this.limit = opts.limit || 10;

    this.count = function()
                 {                          
                    this.deferDoStuff();
                 };

    this.deferDoStuff = function()
                 {
                     var me = this;
                     setTimeout(function() { me.doStuff(); }, 1000);
                 };

    this.doStuff = function()
                   {
                        if(this._currentCount > this.limit)
                        {
                            document.getElementById(this.outputId).innerHTML = 'Count limit reached';
                            return;
                        }

                        document.getElementById(this.outputId).innerHTML = 'count = ' + this._currentCount;
                        this._currentCount++;
                        this.deferDoStuff();
                   };
}

Usage:

        var x = new Counter( { 'outputId' : 'div1' } );
        var y = new Counter( { 'outputId' : 'div2' } );

        x.count();
        y.count();