current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

Yes, I know there are plenty of libraries to get this done (Q, step, async), but only for learning I'm wondering if the following code is ok and usable or have major drawbacks and it is only usable at a small programs.

First, the usage:

var g = new Manager();

g.then ( 
    function() { 
        console.log("hello");
        setTimeout( function () { g.done("hello done") }, 2000 );
    }
);

g.start();

g.then ( 
    function() { 
        console.log("two");
        g.done ("two done");
    }
);

g.then ( 
    function(resp) { 
        console.log("I got: " + resp);
        setTimeout( function () { g.done("happy ending") }, 2000 );
    }
);

g.then ( 
    function(resp) { 
        console.log("last msg: " + resp);
        g.done();
    }
);

and now the class:

//...............................
// class Manager
//...............................
function Manager () { 
    this.functionList = [];
    this.allDone = false;
} // ()

//...............................
//...............................
Manager.prototype.then = function (fun) { 
    this.functionList.push (fun);
    if (this.allDone) { 
        this.allDone = false;
        this.done(this.previousResult);
        console.log ( "then(): continuing");
    } 

} // ()

//...............................
//...............................
Manager.prototype.done = function (prevRes) { 
    this.previousResult = prevRes;
    if ( this.where <= this.functionList.length-2 ) { 
        this.where++;
        this.functionList [this.where](this.previousResult);
    } else { 
        this.allDone = true;
        console.log ( "done(): stopping because allDone: where=" + this.where + " and length = " + this.functionList.length);
    } 

} // ()

//...............................
//...............................
Manager.prototype.start = function () { 
    this.allDone = false;
    this.where=-1;
    this.done();
} // ()
share|improve this question
1  
Better suited at Code Review (flagged for moving) –  Bergi Jul 5 '13 at 8:43
    
Oh, thanks Bergi, didn't ever heard of code review. Better there. –  cibercitizen1 Jul 5 '13 at 8:57
add comment

migrated from stackoverflow.com Jul 7 '13 at 1:25

This question came from our site for professional and enthusiast programmers.

1 Answer

up vote 1 down vote accepted

Some points:

  • g.then(function() { … g.done ("two done");}) I (personally) don't like that signature, I'm more used to callbacks that get passed as parameters (g.then(function(done) { … done ("two done");})). They tend make the chained functions better reusable - in your case it might not matter since you're using dedicated function expressions anyway which are bound to g. And your signature simplifies argument handling in the Manager :-)
  • this.allDone = false; seems to be a misnomer for me. The way you're using it, it should rather be called startedAndNotRunning. Technically if you're initialising the empty list they are all done already :-)

  • This is highly confusing:

    if ( this.where <= this.functionList.length-2 ) { 
    this.where++;
    …

    What happens to the last two functions? Nothing special. It better is written

    this.where++;
if (this.where < this.functionList.length) {
    …

    Then the iterator variable (where) will become the length of the array after the "loop", which is the common behaviour. Yet, you cannot use done any more to [re]start the chain - you'd need to decrement where before. Therefore I'd recommend to restructure the methods to

    function Manager () {
    …
    this.where = 0;
}
Manager.prototype.start = function () {
    // you might reset allDone and where
    this.next();
};
Manager.prototype.done = function (prevRes) { 
    this.previousResult = prevRes;
    this.where++;
    this.next();
};
Manager.prototype.next = function() {
    if (this.where < this.functionList.length)
        this.functionList[this.where](this.previousResult);
    else { 
        this.allDone = true;
        console.log ( "done(): stopping at where=" + this.where + " with length = " + this.functionList.length);
    }
};
Manager.prototype.then = function (fun) { 
    this.functionList.push (fun);
    if (this.allDone) {
        this.allDone = false;
        this.start();
        console.log ( "then(): continuing");
    } 
};
share|improve this answer
    
I appreciate your comments. The internal implementation Manager may be not the best, but I was more interested in the usability of the interface (client code) and with regard to the implementation (here, consider your version) if you think that it can be used as a library for general development, or it must be restricted to cases with few ".then" –  cibercitizen1 Jul 8 '13 at 15:13
    
What do you mean by "cases with few then"? It has no performance problems if you mean that. On the usability of the interface, only my first bullet point is interesting. It often is subjective, so ask your actual client :-) –  Bergi Jul 8 '13 at 15:57
    
Yes, its on performance. And here I suppose there is no need to think in "thread safe" implementations. –  cibercitizen1 Jul 8 '13 at 16:33
1  
Yes. And since the Manager is dedicated for asynchronous function queues, you don't need to worry about the call stack as well. –  Bergi Jul 8 '13 at 17:30
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.