addyosmani / essential-js-design-patterns

Repo for my 'Learning JavaScript Design Patterns' book
http://addyosmani.com/resources/essentialjsdesignpatterns/book/
4.8k stars 791 forks source link

Bug and advice in Observer Pattern’s Codes #158

Open ElvisKang opened 9 years ago

ElvisKang commented 9 years ago

Version:1.6.2 There are two issues in Observer Pattern's code:

1.(advice) the indexOf method:

Original:

ObserverList.prototype.indexOf = function (obj, startIndex) {
    var i = startIndex;

    while (i < this.observerList.length) {
        if (this.observerList[i] === obj) {
            return i;
        }
        i++;
    }

    return -1;
};

I looked up the ECMASCRIPT 5.1 doc,and at 15.4.4.14, I found:

indexOf compares searchElement to the elements of the array, in ascending order, using the internal Strict Equality Comparison Algorithm (11.9.6),

So,I think that the code can be modified :

ObserverList.prototype.indexOf = function(obj,start){
    var start = start || 0;
    return this.list.indexOf(obj,start);
};

Test code:

var sub = new Subject();
var a = {x:1};
var b={x:2};
var c={x:3};
sub.addObserver(a);
sub.addObserver({x:2});
sub.addObserver(b);
sub.addObserver(c);
console.log(sub.observers);
console.log(sub.observers.indexOf(b));

Result:

{ observerList: [ { x: 1 }, { x: 2 }, { x: 2 }, { x: 3 } ] }
2

2.(bug) the Subject.prototype.removeObserver method:

Code:

Subject.prototype.removeObserver = function (observer) {
    this.observers.removeAt(this.observers.indexOf(observer,0));
};

If indexOfmethod returns -1,then the last of element in observers would be deleted incorrectly. Test code:

var sub = new Subject();
var a = {x:1};
var b={x:2};
var c={x:3};
sub.addObserver(a);
sub.addObserver(b);
sub.addObserver(c);
console.log(sub.observers);
var d ={x:4};
sub.removeObserver(d);
console.log(sub.observers);

Result:

{ observerList: [ { x: 1 }, { x: 2 }, { x: 3 } ] }
{ observerList: [ { x: 1 }, { x: 2 } ] }