void IParent.RemoveChild(IEditableBusinessObject child)

rxelizondo posted on Saturday, December 12, 2009

Hi there,

While I was messing around with the "Csla.Core.BusinessBase" object I found the following interesting method:

void IParent.RemoveChild(IEditableBusinessObject child)
{
var info = FieldManager.FindProperty(child);
FieldManager.RemoveField(info);
}

The "IParent.RemoveChild" method XML documentation states the following: This method is called by a child object when it wants to be removed from the collection.

Ok, now that we got that out of the way, let’s dig inside this function. As we can see, the code inside the function makes a call to the "FieldManager.FindProperty()", so let’s take a look at that method and see what that method does:

internal IPropertyInfo FindProperty(object value)
{
var index = 0;
foreach (var item in _fieldData)
{
if (item != null && item.Value != null && item.Value.Equals(value))
return _propertyList[index];
index += 1;
}
return null;
}

The interesting part of this method is the “item.Value.Equals(value)” call. The problem here is that it’s using “Equals” to compare the object when I think that what this should be doing is a reference comparison.

If we do a value (Equals) comparison we risk that the developer might have implemented the "Equals" method in a way that two BusinessObjects could be evaluated as being the same and this will end up causing problems.

Perhaps I am wrong about this but I just wanted to make the call out in case this could be a potential issue.

Thanks.

tmg4340 replied on Saturday, December 12, 2009

Well - I'd argue that having two objects in a collection which are logically equal presents its own set of issues, irrespective of whether "RemoveChild" gets the right one. Determing "the right one" in that case is relatively ambiguous. I realize the method call is receiving "the right one", but that's usually initiated by developer code, so who's to say the developer found the right one to delete in the first place?

And ReferenceEquals wouldn't necessarily solve the issue either - what if the developer adds the same child object to the collection twice? Normally I would think you'd disallow that, but if you're worried about logical equality within the collection, then your case removes the unique constraint from the collection.

In any event, Equals() has been the .NET standard for equality-checking for a while...

- Scott

rxelizondo replied on Saturday, December 12, 2009

Scott,

OK, I am fine with what you said, but.... that is the reason I included the following paragraph in my original post:

rxelizondo:

The "IParent.RemoveChild" method XML documentation states the following: This method is called by a child object when it wants to be removed from the collection.

The child object is calling the method when*it* wants to be removed from the collection. I would not make sense that child "abc" called the method to get itself removed from the collection and end up removing child "xyz" right?

Just making a call out :)

Like you said, a well implemented property would probably never run into an issue like the one I explained, and of course, there is also the issue with the function accepting an object which would mean that it could accept value type object which would mean that reference comparison would never be true.... Maybe the FindProperty object should only accept IEditableBusinessObject since any other object would probably not make sense?

Thanks.

ajj3085 replied on Monday, December 14, 2009

Csla used to force you to implement a GetIdValue method, the idea being that you WANTED the logically "equal" instance. So the above code is correct from that perspective.

This requirement was dropped however when it was discovered that Wpf does the wrong thing; it was using Equals when it it should have been using ReferenceEqual, and this discepency was causing Wpf not to referesh its databinding.

If someone wants to use logical equality and they can, I don't see any reason Csla should prevent them from doing so.

You can read about the original issue here: http://www.lhotka.net/weblog/CommentView,guid,06f305de-ec32-4e20-b042-171b58f305ae.aspx

rxelizondo replied on Monday, December 14, 2009

Thanks Andy,

To me, the use of Equals should have never been used regardless of whether the CSLA forced you to use GetIdValue or not.

Please note (this is important) that I am basing this post *solely* by the XML documentation of that method (this is *key*).

The XML documentation does *not* state: This method is called by a child object when it wants itself *or* a value equivalent child to be removed from the collection.

It clearly states: This method is called by a child object when *it* wants to be removed from the collection.

So basically, if object in heap address of OxF54E74AF wants to be removed form the parent then object heap address of OxF54E74AF should be the one removed and none other.

So using Equals is just asking for trouble, this should have been implemented as RefEquals from the get go. If anything, from a performance point of view, RefEquals would be one whole microsecond faster so there :)

In any event, this is just a silly post, even if I am right about this, there is a 99.9% probabilities that is not going to get fixed because its unlikely that someone will ever run into this issue so lets not bother with this. I was just putting it out there so Rocky would see it and if he wants to do something about it that is fine with me, if not that is also fine with me.

RockfordLhotka replied on Tuesday, December 15, 2009

Prior to verion 3.0 CSLA supported the concept of "logical equality" as a general practice. That was very valuable.

Unfortunately WPF came along, and WPF doesn't allow any interpretation of Equals() as being different from ReferenceEquals(). This (imo) is really poor on their part, because it means Equals() is essentially always useless - but it is what they did in WPF.

So CSLA 3.0 and higher doesn't support logical equality. If you override Equals() you'll break CSLA and WPF and (I think) Silverlight.

Since overriding Equals() breaks core .NET functionality, I don't feel too bad if it also breaks CSLA functionality - Microsoft set the bar, I'm just lowering myself to match it :)

rxelizondo replied on Wednesday, December 16, 2009

RockfordLhotka:
Unfortunately WPF came along, and WPF doesn't allow any interpretation of Equals() as being different from ReferenceEquals().

I guess technically, Microsoft broke the Equal/ReferenceEquals semantics from the get go. For example:

object o1 = new object();
object o2 = new object();

bool foo = o1.Equals(o2);

In the example avobe, the variable “foo” is false when it really shold be true right? I am sure there must be a good reason for this but it still contradicting what Equal and ReferenceEquals should stand for.

tmg4340 replied on Wednesday, December 16, 2009

Actually, from MSDN:

"The default implementation of Equals supports reference equality for reference types".

Which is why your example works as it does.

I don't necessarily consider them contradicting. MSDN calls it "value equality versus reference equality". Equals is overridable so that "value equality" can be defined for an object.

HTH