[ FIXED ] removeMediator not managing observer list properly

[thomasvst]

I think there is a bug in the View.removeMediator function.

When an element is removed from the Array with the 'splice' function, the next Observer in the Array is skipped because the 'i' counter is increased anyway.

Here is a reproduction of the bug ;

:

var vegetables:Array = new Array("spinach", "green pepper", "cilantro", "onion", "avocado");
for ( var i:int = 0;  i < vegetables.length; i++ ) {
    if ((vegetables[i] as String).indexOf("o") > -1) {
    trace(vegetables[i]);
        vegetables.splice(i,1);
        // i--;
    }
}

The result should be cilantro onion avocado but is cilantro avocado.

[puremvc]

Thomas,

Good work in tracking this one down! Two solutions come to mind.

Brute force approach:
Do a for-each-in instead of an iterator.

Once an observer has been found and needs to be removed, we need to know its index so that we can splice it, which will require another nested, iterated loop to find the match, then we splice that index out and break from the iterated loop.

This smells, because in a large system it could potentially make the call very time consuming.

Copy survivors to a new observer list:
We define a new array to hold the indices of the observers to be removed. We pass over the  observer list using an iterator and store the indices to be removed.

If we found anything to remove, we define a second array, a new observer list. We do a second pass over the array, but we cannot use splice to remove,  so we copy everything that should survive into the new observer list.

Then we set we set the current observerMap key equal to the new observer list, unless the list is empty, in which case we delete the key from the observerMap because there are no more observers for that notification.

The previous observer list was the only thing with a reference to the observers to be removed and the only reference to it was the observerMap. So, when we overwrite the reference to the old observer list with the new observer list, the old list's reference count will fall to zero and it will be garbage collected, and since it was the only thing holding a reference to the observers to be removed, they too are garbage collected.

I believe this should fix the problem. I will work on implementing the change and there will be a new version shortly that addresses this.

Update: See the next reply....
-=Cliff>

[puremvc]

A much more elegant approach:

Splice the targets out in reverse order:
We define a new array to hold the indices of the observers to be removed. We pass over the  observer list using an iterator and store the indices to be removed.

Since this list is in ascending order, we now enter a while loop that executes as long as the list of removal targets is greater than zero. We pop a target off the stack and splice it from the observer list.

After this, if the observer list length has fallen to zero we remove the notification key from the observer map.

That's it.

We're able to splice the items out of the array with this approach because the while/pop combination is effectively skipping us backwards through the observer array. So, the list elements collapsing back to fill the space where the spliced-out element was doesn't affect the position of the lower-numbered indices we've yet to remove.

This is far simpler and the most performant of all three I described. All three require a single pass over the observer list, but the other two require at least one if not many further iterations through the array.

This one performs one more loop backwards over the array, but skipping directly the indices to be removed.

This is what has been implemented. Currently unit tests are passing, but I'm creating a specific test for this.

-=Cliff>

[thomasvst]

Why not a "i--" after the slice call ?

[puremvc]

Thomas,

Although i-- after the splice would technically handle the problem, and a shorter amount of code, it would feel too much like a hack. Reminiscent of the famous Draperism 'If 2+2=5 then let 2+2=4'.

The splice should not be happening inside the loop to begin with, since it messes with the iterator. Clearly an oversight on my part; perhaps in too much of a rush to get the bug fixed.

The approach described as 'Splice the targets out in reverse order' is nearly as performant, and has the virtue of making it clear what's happening and not employing any questionable practices.

-=Cliff>

[puremvc]

OK folks, this one's in the can. Version 1.7 is released.
http://puremvc.org/content/view/45/153/

-=Cliff>