PDA

View Full Version : MixedCollection.add implementation



tryanDLS
20 Nov 2006, 3:42 PM
Jack,

I was looking at the add method, and I can see the possibility for some very hard to find bugs occurring.
Since the method doesn't check for key existence, you can add multiple items with the same key and step on things. Also, if you called each in this situation, weird things could happen. You'd have the action take place 2 times on one element, since it would be in 2 places in the items array.

Maybe there's a use for this implementation, but it would seem to be the exception, rather than the norm, and should be handled as a subclass.

I think I'd rather know that I added a dup key via an exception, unless I specifically wanted a type of collection that allowed a dup key to do a replace. Even in that situation, the implementation isn't right as there are still 2 items in the array being iterated over.

EDIT:
So I was playing with implementing a check in add for a dup key, and found a bug in indexOfKey() - it passes 'key' as the arg, the compare uses 'o'. Also, the for loop is looking at 'this.key.length' instead of 'this.keys.length'

jack.slocum
21 Nov 2006, 3:48 AM
I don't know what happened there. Those indexOfKey bugs both look like retarded bugs.

As for the add and duplicates, I agree the behavior is confusing. add() is just that though -> add only. If the element might already exist, I'd recommend using replace(). replace will add the element if it doesn't already exist or replace it in a consistent manner.

However, replace() did not use getKey() which breaks the generic key reading feature of mixed collection. That sucks and I have corrected it by adding the standard "if arguments.length == 1..." block to the top of replace.

tryanDLS
21 Nov 2006, 8:34 AM
Not sure I've got the hang of this SVN thing when I made changes to my local version. I got what I think is the lastest ver w/your fixes and it looks like indexOfKey still has a bug in the else - indexOf(o) s/b indexOf(key)