Collections & Events

[Scott Stark]

Hello,

I've got some code working but I'm not sure that the way I implemented it is
the best way. I have a custom collection class with a boolean property
HasChanged that tells me if any of the data has changed. An event is raised
on certain methods (Add, Remove, Insert, etc). Let me write a little code to
describe what I've done and see if there's room for improvement:

I want to represent a class that functions like this when instantiated.

Product P = new Product(productID);
P.Options.Name = "Gender";
P.Options.Values[0] = "Male";
P.Options.Values[1] = "Female;

I need to set events on both the custom "Options" collection as well as the
custom "Values" collection contained within Options so that I know if either
a new option has been added or if a value has been modified.

Here's my solution (which again, work just fine, looking for areas I can
improve on):

<< THERE ARE ONE OF THESE FOR ProductOptionsCollection and
ProductOptionValuesCollection. ONLY DISPLAYING ONE FOR BREVITY >>

namespace Sample
{
public delegate void ProductOptionChangedEventHandler(object sender,
EventArgs e);

public class ProductOptionCollection : CollectionBase
{
public event ProductOptionChangedEventHandler Changed;

private bool _HasChanged;

public bool HasChanged
{
get { return _HasChanged; }
set { _HasChanged = value; }
}

public int Add(ProductOption item)
{
OnChanged(new EventArgs());
return List.Add(item);
}

.. OTHERS REMOVED FOR BREVITY..

protected virtual void OnChanged(EventArgs e)
{
if (Changed != null)
Changed(this, e);
}
}
}


Then, within the calling class:

_Options.Changed += ProductOptions_OnChange;

IS THIS THE RIGHT WAY TO REGISTER FOR EVENTS WITHN A COLLECTION
OF A COLLECTION?
for (int i=0; i < _Options.Count; i++)
{
_Options.Values.Changed += ProductOptions_OnChange;
}

NOTICE I'M SETTING A BOOLEAN VALUE WITHIN THE CUSTOM COLLECTION
THAT RAISED THE EVENT IN THE FIRST PLACE -- I can't just set it within the
collection itself when Add is called because I need to programatically
register the event handler after the initial population of the collection.
Is there a better way to do this?
private void ProductOptions_OnChange(object sender, EventArgs e)
{
_Options.HasChanged = true;
}

 

[Scott Stark]

All good points. I hammered out a couple of the example code lines as I was
in a rush out the door this morning. Yes, the first code portion (with the
indexers) is incorrect.

This is for a product inventory management system. I have products
(represented by the Product class) that can have various options. For
example, a golf club has

Product P = new Product();

Option O = new Option();
O.Name = "Shaft";
O.Values.Add("Graphite");
O.Values.Add("Steel");

P.Options.Add(O);

Option O1 = new Option();
O1.Name = "Gender";
O1.Values.Add = "Male";
O1.Values.Add = "Female"

P.Options.Add(O1);

So I can have a golf club with a graphite shaft made for a female.

When I create an instance of the class, the Options collection gets
populated in the constructor. What I need to know is, has the options
collection (or its Values sub-collection) changed since the object was
instantiated?

Since I'm adding products during initialization, I don't want the boolean
"HasChanged" value to be set there, so I'm wiring up the event handlers
after the collections are populated so I can detect any changes
post-initializaton.

If the Options.HasChanged flag is true when Product.Save() is called, I'll
persist those values back to the database, otherwise I'll leave them alone
and save the database call.

Sorry for being a little vague earlier, I was in a bit of a rush.

This is my first time exprimenting with some of this stuff, so forgive me if
my approaches or knowledge level leave a bit to be desired. That's why I'm
asking these questions in the first place.

[...]
IS THIS THE RIGHT WAY TO REGISTER FOR EVENTS WITHN A
COLLECTION OF A COLLECTION?
for (int i=0; i < _Options.Count; i++)
{
_Options.Values.Changed += ProductOptions_OnChange;
}

NOTICE I'M SETTING A BOOLEAN VALUE WITHIN THE CUSTOM
COLLECTION THAT RAISED THE EVENT IN THE FIRST PLACE -- I can't just set
it within the collection itself when Add is called because I need to
programatically register the event handler after the initial population
of the collection. Is there a better way to do this?

It's impossible to say for sure, given that we have basically no details
about the overall design. That said, some thoughts:

-- Your subscription loop is not consistent with the code you posted
first. That is, you are indexing "_Options" (whatever that is), but in
the very first code example you gave, "Options" has no indexer. Probably
the first code example was wrong, but inconsistencies create ambiguities,
making it even harder to know what you're doing.

-- I think the "HasChanged" property is superfluous and poor design.
"HasChanged" since when? The "since when" is not defined by the class at
all, especially if the class itself isn't maintaining the property. If
you need to know this information, put the boolean somewhere else, in code
where it can be clearly stated when the flag is set and reset. Even
better, don't write code that relies on a state variable like that in the
first place.

-- You've declared a delegate type that is exactly the same signature
as the EventHandler delegate type. You should just use EventHandler
instead of making your own. Alternatively, make the event more useful by
creating an EventArgs sub-class that will include information about the
change (e.g. what option was added or changed), and then create a new
delegate type based on _that_.

-- The idea that an option might change after being added seems very
odd to me, especially given the example you've provided ("Gender"). But
granted, there's so little information about the bigger picture being
provided here, it's hard to know for sure whether that's a valid
criticism.

Now, all that said, there is nothing fundamentally wrong with executing a
loop that subscribes the same event handler to a particular event on a
series of objects. So as far as your specific question goes, sure...that
seems fine.

Pete

 

[Scott Stark]

Thanks.

I actually opted to go with your first suggestion. As I said, the events
work and it was a good experience for me just to practice creating and
wiring them up. I got it in my head that I needed to monitor the state of
something in another object changing so events just seemed the first choice
and I went with it.

Having said that, if I were to implement it as an event, would the method I
used be approrpriate or is there a more accepted practice to accomplish what
I was trying to do?

[...]
When I create an instance of the class, the Options collection gets
populated in the constructor. What I need to know is, has the options
collection (or its Values sub-collection) changed since the object was
instantiated?

Since I'm adding products during initialization, I don't want the
boolean "HasChanged" value to be set there, so I'm wiring up the event
handlers after the collections are populated so I can detect any changes
post-initializaton.

I would advise one of two alternatives, both of which involving having the
"changed" state updated exactly where the collections are modified rather
than relying on events:

-- simply clear the state after initialization in the constructor
-- suppress changing of the state by including some private flag in
the class set during initialization

I think the first approach is the simplest, but if you had some specific
reason for not even wanting the state to change during initialization, the
second might be appropriate.

The events seem like overkill to me.

Pete

 

[Scott Stark]

I just thought of something after I wrote my last post...

_Options.Changed += ProductOptions_OnChange;

for (int i=0; i < _Options.Count; i++)
{
_Options.Values.Changed += ProductOptions_OnChange;
}

private void ProductOptions_OnChange(object sender, EventArgs
e)
{
_Options.HasChanged = true;
}

With this code in mind, you can see that the same event handler
(ProductOptions_OnChange) is triggered when either the Options collection is
modified OR any of the Options.Values data is modified and flips the boolean
value of Options.OnChange to true.

So Options.HasChanged == true whenever ANY data within the collection or any
of it's sub-collections is modified. This means I only have to do a check:

if (Options.HasChanged) { //do stuff }

If I set a private boolean flag within each custom collection, when it comes
time to check for the state, I'd have to do this:

if (Options.HasChanged || someFunction(Options.Values)) { //do stuff }

private bool someFunction(ProductOptionValues POV)
{
// loop through the ProductOptionValues collection and check the
HasChanged value on all items in the collection returning true if ANY of
them are true
}

Would this change your mind on whether events are the best approach to use?
Since the ProductOptionValues collection has no intrinsic knowledge of the
Options class, I can't set that flag from within it and am forced to use a
more complicated check of the value.

[...]
When I create an instance of the class, the Options collection gets
populated in the constructor. What I need to know is, has the options
collection (or its Values sub-collection) changed since the object was
instantiated?

Since I'm adding products during initialization, I don't want the
boolean "HasChanged" value to be set there, so I'm wiring up the event
handlers after the collections are populated so I can detect any changes
post-initializaton.

I would advise one of two alternatives, both of which involving having the
"changed" state updated exactly where the collections are modified rather
than relying on events:

-- simply clear the state after initialization in the constructor
-- suppress changing of the state by including some private flag in
the class set during initialization

I think the first approach is the simplest, but if you had some specific
reason for not even wanting the state to change during initialization, the
second might be appropriate.

The events seem like overkill to me.

Pete