[rules-dev] BUG: [5.3.0.Final] CollectSetAccumulateFunction should probably use IdentityHashMap internally

[SirMungus]

Mark Proctor wrote
> 
> accumulate supports custom functions, just add your own version of 
> collectSet that uses IdentityMap.
> 
I could do that, but please let me take one more stab, just in case I
haven't been clear enough.

1) I am not suggesting that collectSet() return an identity-based set. I am
content to have it return a normal set, though I had early confusion on that
point.

2) The discussion above about stated vs. justified facts turns out to be a
distraction to the main point of the bug that I believe to be in
CollectSetAccumulateFunction.  My failure to understand the distinction is
not germane to the bug.

3) I am not disagreeing with the value behind implementing equals() to only
take into account invariant fields, though I am stating that goes beyond the
explicity documentation of equals()/hashCode() and is explicitly *not*
followed by many classes in the JRE (most notably, collections).

There is nothing about the purpose of collectSet() that logically suggests
that it should only work with objects that have invariant
equals()/hashCode() functions. If five objects match a rule, they should be
able to be collected into a set (which may end up with fewer than five
objects). If things change such that only three facts match the rule, they
should still be able to be collected into a set.  There's no compelling
reason why that should only apply to objects with invariant equals(). And it
seems to me that you rarely want to leave an NPE in code, rather than
catching it and throwing a more intelligible exception (e.g., "Object to be
removed from collectSet() was not found").

If the internal maintenance of state in CollectSetAccumulateFunction used
identity semantics and applied the equality semantics only when you asked it
for its results, it would maintain the same external behavior (returning a
"normal" equality-based set) without failing to work for objects with
variant equals.

If you Google around for the proper implementation of equals(), you will
find many serious sites that discuss it at length, yet provide examples
like:

public boolean equals(Object aThat){
  if ( this == aThat ) return true;
  if ( !(aThat instanceof Car) ) return false;
  Car that = (Car)aThat;
  return
    EqualsUtil.areEqual(this.fName, that.fName) &&
    EqualsUtil.areEqual(this.fNumDoors, that.fNumDoors) &&
    EqualsUtil.areEqual(this.fGasMileage, that.fGasMileage) &&
    EqualsUtil.areEqual(this.fColor, that.fColor) &&
    Arrays.equals(this.fMaintenanceChecks, that.fMaintenanceChecks);
//array!
}

Ironically, that is explicitly against Wolfgang's suggestion above!


--
View this message in context: http://drools.46999.n3.nabble.com/BUG-5-3-0-Final-CollectSetAccumulateFunction-should-probably-use-IdentityHashMap-internally-tp3774079p3781313.html
Sent from the Drools: Developer (committer) mailing list mailing list archive at Nabble.com.