6

I have the following code which defines a Car. Each Car has a color, along with a setColor(color) function. I want to add listener functions which are called whenever setColor(color) is called, and I want to be able to tack these listener functions on whenever I want. Is this a suitable approach? Is there a cleaner way?

function Car() {

    this._color = 'red';
    this._callbacks = {};

    this.setColor = function(color) {
        this._color = color;
        console.log(">>> set car color to " + color);
        if (this._callbacks['setColor']) {
            this._callbacks['setColor']();
        }
    };

    this.addListener = function(functionName, handler) {
        if (this._callbacks[functionName]) {
            var oldCallback = this._callbacks[functionName];
            this._callbacks[functionName] = function() {
                oldCallback();
                handler();
            }
        } else {
            this._callbacks[functionName] = function() {
                handler();
            }
        }
    };


}

var car = new Car();
car.setColor('blue');
car.addListener('setColor', function() { console.log("This is listener # 1"); });
car.setColor('green');
car.addListener('setColor', function() { console.log("This is listener # 2"); });
car.setColor('orange');

Output:

>>> setColor to blue
>>> setColor to green
This is listener # 1
>>> setColor to orange
This is listener # 1
This is listener # 2
Andrew
  • 143
  • 1
  • 1
  • 7

3 Answers3

3

I think an array to store the listeners would be a cleaner approach. Also, you should use the prototype object, or make the semiprivate properties real private variables.

function Car() {
    this._color = 'red';
    this._callbacks = {setColor:[]};
};
Car.prototype.setColor = function(color) {
    this._color = color;
    console.log(">>> set car color to " + color);
    for (var i=0; i<this._callbacks['setColor'].length; i++)
        this._callbacks['setColor'][i]();
};
Car.prototype.addListener = function(functionName, handler) {
    this._callbacks[functionName].push(handler);
};

Or:

function Car() {
    var color = 'red';
    var callbacks = {};

    this.setColor = function(c) {
        color = c;
        console.log(">>> set car color to " + color);
        for (var i=0; 'setColor' in callbacks && i<callbacks['setColor'].length; i++)
            callbacks['setColor'][i]();
    };
    this.addListener = function(functionName, handler) {
        if (functionName in callbacks)
            callbacks[functionName].push(handler);
        else
            callbacks[functionName] = [handler];
    };
}
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • I tweaked my own code to roughly match your second option. Similar question as I posted in an above comment: what is the advantage of using the prototype object? And I've also never been sure how exactly to "make semiprivate properties real private variables". Could you explain some more? – Andrew Aug 22 '12 at 15:56
  • See [Advantages of using prototype, vs defining methods straight in the constructor?](http://stackoverflow.com/questions/4508313) or [Declaring javascript object method in constructor function vs. in prototype](http://stackoverflow.com/q/9772307/1048572). In the first example, the properties of the object are public (only the underscore denoted they should not be used from outside) - in the second example I've used local variables scoped to the constructor, and they are only accessible to the two privileged methods. (Enough buzzwords to google ;-) – Bergi Aug 22 '12 at 16:08
  • The answer by @Utkanos was good too, but the second option given here is the solution I ended up using. This gets rid of the semiprivateness of my variables `color` and `callbacks` and puts the callback functions in an array instead of a closure. – Andrew Aug 26 '12 at 00:52
2

Something like this, perhaps.

//the 'class'
function Car() {

    //set up a static listeners object - an array for each method
    Car.listeners = {setColor: []};

    //called by methods on invocation, to fire their listeners
    Car.executeListeners = function(methodName) {
        for (var i=0, len = Car.listeners[methodName].length; i<len; i++)
            Car.listeners[methodName][i].apply(this);
    };

    //method - on invocation, fire any listeners
    this.setColor = function(color) {
        this.color = color;
        Car.executeListeners.call(this, 'setColor');
    };
}

//instance
var car = new Car();

//add a listener to instance.setColor invocations
Car.listeners.setColor.push(function() {
    alert("hello - this car's color is "+this.color);
});

//set color (triggers listener)
car.setColor('orange');

Note you are assigning prototype-esq methods to the instances rather than to the prototype itself - the place for inherited, reusable functionality. Inheritance is also faster in performance terms.

Mitya
  • 33,629
  • 9
  • 60
  • 107
  • I'm only somewhat confident in my knowledge of how prototyping works. What is wrong with making each of those `Car.{method}` functions a `this.{method}` function? (In other words, replacing the three instances of "Car" with "this" inside the `Car` function body. – Andrew Aug 22 '12 at 15:53
  • Why did you made the listeners static? I don't think that's a good idea. – Bergi Aug 22 '12 at 15:59
  • @Mokt - the prototype is the place for inheritable, reusable functionality. The way you've done it will work, but instance-based methods are slightly slower (granted you'd have to have a lot of hefty methods to notice the difference) and it means they're own properties (so they will be enumerable) of the instance rather than inherited. – Mitya Aug 22 '12 at 17:42
  • @Bergi - why so? You don't elaborate. Seems to work OK. One, static place for listeners for all methods. Works fine with multiple instances. http://jsfiddle.net/akxVD/1 Happy to be corrected. – Mitya Aug 22 '12 at 17:44
  • @Utkanos: Yes, it does work, but I don't think it is intended. Breaking the OPs functionality would need at least a comment on that – Bergi Aug 22 '12 at 17:48
1

If it's working for you then I see no reason to not go with it. Some thoughts on this approach:

  • you can't set the context of the handlers, if the handler should be called against a certain object, you should have the addListener method take a context object and do callback.call(context). If you don't care about this, then no need to worry about it.
  • If an early callback blows up, the later callbacks won't get called. Not sure if you care about this, but if you do you could instead use an array to store all the callbacks in, and iterate over the array calling each one in turn. You might need to wrap the call to the callback in a try/catch block to ensure the callbacks keep getting called.
  • Do you want to pass in the current color to the callback? Or even the Car object itself, something like callback(this, this._color)?
  • Your approach uses closures a fair amount. If you find performance becoming an issue, getting rid of the closures would help that.

Another thing to consider is using Object.defineProperty, but that's a more stylistic choice.

Matt Greer
  • 60,826
  • 17
  • 123
  • 123