AddDependentProperty doesn't check for duplicates

AddDependentProperty doesn't check for duplicates

Old forum URL: forums.lhotka.net/forums/t/8340.aspx

nelis posted on Wednesday, January 13, 2010

Subject says it all.

I am trying to resolve dependencies from an Expression (which is pretty easy) but that may result in adding the same dependency multiple times. It doesn't have any effect on the business logic but CheckRules is called multiple times (which is an unnecessary waste of CPU)

RockfordLhotka replied on Wednesday, January 13, 2010

So you'd rather have CSLA throw an exception to help you debug the problem? I'll add that to the wish list.

rxelizondo replied on Wednesday, January 13, 2010

If you are going to be making changes on that area perhaps you may also want to consider throwing an exception if the user is creating circular dependencies such as:

ValidationRules.AddDependentProperty(StartedProperty, StartedProperty);

I know its unlikely that someone would be doing something like that but just throwing it out there.

JonnyBee replied on Wednesday, January 13, 2010

Hi,

I once ran into this one too:

ValidationRules.AddDependentProperty(StartedProperty, EndedProperty);
ValidationRules.AddDependentProperty(EndedProperty, StartedProperty);

creates a circular reference and code should actually be
ValidationRules.AddDependentProperty(StartedProperty, EndedProperty, true);

So if Csla is going to do a better check (or update dependency to be bidirectional internally)  it would also help.

nelis replied on Thursday, January 14, 2010

I wasn't thinking of an exception. Just a list.Contains check before the list.Add in

public void AddDependentProperty(string propertyName, string dependentPropertyName)

{

// get the list of rules for the property

List<string> list = GetRulesForProperty(propertyName, true).GetDependancyList(true);

// we have the list, add the dependency

list.Add(dependentPropertyName);

}

RockfordLhotka replied on Thursday, January 14, 2010

nelis:

I wasn't thinking of an exception. Just a list.Contains check before the list.Add in

Allow the bug and just smooth it over? :)

nelis replied on Friday, January 15, 2010

Why would you consider calling AddDependentProperty twice or more with the same arguments a bug? 'Redundant' I can live with ;-)

ajj3085 replied on Friday, January 15, 2010

Depends.  Maybe its happening because intellisense kicked in and picked the wrong value. 

RockfordLhotka replied on Friday, January 15, 2010

And having the your code duplicated is messy at best.

Suppose you want to remove the dependency, so you remove the line of code that you see which sets up the dependency. Of course the dependency remains, because some other line of code is also setting up the dependency.

While perhaps not a "bug", this is the kind of poor coding that leads to bugs later.

The responsible thing for CSLA to do (if anything) is to help you locate scenarios where your code is incorrect so you can fix it.

nelis replied on Friday, January 15, 2010

Let me explain how duplicate registrations may occur in my case.
I created extension methods for ValidationRules:
public static void AddConditionalRule(this ValidationRules validationRules, Expression> condition, RuleHandler handler, ...)
The idea is that the rule is only applied when the condition evaluates to true. You could of course include the condition in a custom rule but usually it is much more convenient to use a common rule in combination with a dedicated condition (e.g. StringRequired when a certain flag is set).
The condition is analyzed for dependencies that are added using AddDependentProperty. However a dependency may occur more than once:
c => c.Code == "A" || c.Code == "B"
As far as I can see there is no way for me to get a list of already registered dependencies. Doing my own duplicate bookkeeping also seems a bit odd.

I do agree on your 'poor coding' remark. The alternative would be to have access to the list of registered dependencies.

RockfordLhotka replied on Wednesday, January 13, 2010

Actually Jonny, your examples are identical.

The only reason the method takes that bool parameter is as a shortcut so you can do in one line of code what you had to do in two lines of code.

There's absolutely nothing wrong with bi-directional dependent properties, and I don't plan to change how that works at all.

Copyright (c) Marimer LLC