Delegate/Event Naming Conventions

[Jeff Gaines]

I am doing a New Year clean up of my library files and trying to ensure I
use the 'accepted' naming conventions in my Delegates and Events. I think
the following uses those conventions but would appreciate any comments:

internal delegate void PathChangedEventHandler(object sender, JTagData
tagData);
internal event PathChangedEventHandler PathChanged;
protected virtual void OnPathChanged(object sender, JTagData e)
{
if (PathChanged != null)
PathChanged(sender, e);
}

private void RaisePathChangedEvent(object sender, JTagData tagData)
{
OnPathChanged(sender, tagData);
}

It is then hooked up from the program using the class as:
m_SysListView.PathChanged += new
PathChangedEventHandler(m_SysListView_PathChanged);

I could probably skip the RaisePathChangedEvent but sometimes I do
conversions in there and I want to stick to a standard.

Would appreciate any guidance and Happy New Year

 

[RayLopez99]

Would appreciate any guidance and Happy New Year

Looks OK to me. If your library is like mine, likely nobody will use
it other than you, so just comment your code heavily and don't worry
about convention.

RL

 

[Family Tree Mike]

I am doing a New Year clean up of my library files and trying to ensure
I use the 'accepted' naming conventions in my Delegates and Events. I
think the following uses those conventions but would appreciate any
comments:

internal delegate void PathChangedEventHandler(object sender, JTagData
tagData);
internal event PathChangedEventHandler PathChanged;
protected virtual void OnPathChanged(object sender, JTagData e)
{
if (PathChanged != null)
PathChanged(sender, e);
}

private void RaisePathChangedEvent(object sender, JTagData tagData)
{
OnPathChanged(sender, tagData);
}

It is then hooked up from the program using the class as:
m_SysListView.PathChanged += new
PathChangedEventHandler(m_SysListView_PathChanged);

I could probably skip the RaisePathChangedEvent but sometimes I do
conversions in there and I want to stick to a standard.

Would appreciate any guidance and Happy New Year

My only recommended change would be, that from what is posted, your
second argument type doesn't convey that it derives from
System.EventArgs. I would tend to use something more along the lines of
PathChangedEventArgs, instead of JTagData.

If you ask 1000 people, you'll get 1000 answers though...

 

[Peter Duniho]

[...]
protected virtual void OnPathChanged(object sender, JTagData e)
{
if (PathChanged != null)
PathChanged(sender, e);
}

To the OP: if you are trying to clean things up, you might as well make
the above method thread-safe:

protected virtual void OnPathChanged(object sender, JTagData e)
{
PathChangedEventHandler handler = PathChanged;

if (handler != null)
PathChanged(sender, e);
}

For most things, I wouldn't advise worrying about thread-safety. But,
the default implementation of the event itself (i.e. the add and remove
methods) are thread-safe already, and so you might as well finish the
job in your own code.

The other argument in favor of doing that is that events are harder for
client code to impose thread-safety on. The client can easily wrap
changes to the event with synchronization, but it's often less-obvious
where additional synchronization is needed in order to synchronize
changes to the event with actual signaling of the event.

Anyway, it's not really that big of a deal. A class not advertised as
being thread-safe (i.e. 99.9% of the classes written) doesn't _really_
need to make event-raising thread-safe. But it's so easy to do, and
doesn't really affect performance or introduce new blocking behavior, I
figure you might as well.

Note that an alternative to the above is to simply pre-initialize the
event field with an empty delegate, so that you don't ever have to worry
about it being null:

internal event PathChangedEventHandler PathChanged = delegate { };

My only recommended change would be, that from what is posted, your
second argument type doesn't convey that it derives from
System.EventArgs. I would tend to use something more along the lines of
PathChangedEventArgs, instead of JTagData.

If you ask 1000 people, you'll get 1000 answers though...

If you ask 1000 people what the best approach is, you'll get 1000
answers. But, that's a subjective question. The question here is
objectively answered by looking at the .NET conventions, and they're
exactly as you suggest: naming the EventArgs sub-class
"PathChangedEventArgs" is the way to go.

Finally, I'll point out what might be stating the obvious: the naming of
this event implies that there is a property in the class named "Path".
Following the "XXXChanged" pattern might still be suitable in absence of
a property of the name "XXX", but I would take some time to make sure
that was really the right thing to do. There might be a different name
that is more descriptive of what the event really means.

Pete

 

[Harlan Messinger]

[...]
protected virtual void OnPathChanged(object sender, JTagData e)
{
if (PathChanged != null)
PathChanged(sender, e);
}

Is the threading issue the obvious one that if PathChanges is found to
be non-null, it can still have become non-null by the time it's fired?

To the OP: if you are trying to clean things up, you might as well make
the above method thread-safe:

protected virtual void OnPathChanged(object sender, JTagData e)
{
PathChangedEventHandler handler = PathChanged;

if (handler != null)
PathChanged(sender, e);
}

I'm not seeing how this helps. Would you mind explaining it?

 

[Peter Duniho]

On 1/2/2010 7:57 AM, Jeff Gaines wrote:

[...]
protected virtual void OnPathChanged(object sender, JTagData e)
{
if (PathChanged != null)
PathChanged(sender, e);
}

Is the threading issue the obvious one that if PathChanges is found to
be non-null, it can still have become non-null by the time it's fired?
Yes.

To the OP: if you are trying to clean things up, you might as well
make the above method thread-safe:

protected virtual void OnPathChanged(object sender, JTagData e)
{
PathChangedEventHandler handler = PathChanged;

if (handler != null)
PathChanged(sender, e);
}

I'm not seeing how this helps. Would you mind explaining it?

Well, it might have helped if I had actually fixed up the invocation so
that it was correct for that pattern. The above should read:

protected virtual void OnPathChanged(object sender, JTagData e)
{
PathChangedEventHandler handler = PathChanged;

if (handler != null)
handler(sender, e);
}

Perhaps that's enough to answer your question? If not, then…

The delegate type is a reference type, so assignment is atomic. So,
storing the current value locally ensures that even if the actual field
should change before the delegate can be invoked, the invocation takes
place with a delegate reference that is guaranteed not to change.

There is, of course, a race condition. But that would be true
regardless. It's important for clients of events in a multi-threaded
context to not assume that just because they have unsubscribed from an
event in one thread, their handler still won't be called later. The
only way to provide that guarantee is for the client itself to know
exactly how the event might be raised, and to handle synchronization
itself so that event cannot be raised at the same time other code might
be unsubscribing.

Pete

 

[Jeff Gaines]

Well, it might have helped if I had actually fixed up the invocation so
that it was correct for that pattern. The above should read:

protected virtual void OnPathChanged(object sender, JTagData e)
{
PathChangedEventHandler handler = PathChanged;

if (handler != null)
handler(sender, e);
}

Many thanks for all the replies

Thanks to Harlan for asking the question I was struggling to put into words!

To paraphrase Peter's reply in really simple terms are you saying that by
allocating handler to PathChanged (which is presumably defined elsewhere)
there is a guarantee that handler won't change its state between the test
for null and actually being called? Presumably the garbage collector won't
collect it until the function is exited whereas the original PathChanged
could have been disconnected, set to null and/or disposed on another
thread during that tiny gap between testing for null and the event being
called.

Sorry for taking it down to baby talk level but I would really like to get
mu head round it

 

[Peter Duniho]

Many thanks for all the replies

Thanks to Harlan for asking the question I was struggling to put into
words!

To paraphrase Peter's reply in really simple terms are you saying that
by allocating handler to PathChanged (which is presumably defined
elsewhere)

I think you're misusing the word "allocating". The local variable is
_assigned_ the value of the PathChanged field, which is the field
automatically generated by the compiler when you declare an event of the
same name (in the class where the event is declared, you get the
field…outside the class, you get the event).

No allocation happens in the variable's initialization.

there is a guarantee that handler won't change its state
between the test for null and actually being called?

Like the String class, the delegate types are immutable. Modification
of an existing delegate instance, such as adding or removing another
delegate instance (i.e. the type of operation that typically occurs
during the add and remove operations for an event), always yield a whole
new delegate instance, while the previous one remains unchanged.

Presumably the
garbage collector won't collect it until the function is exited whereas
the original PathChanged could have been disconnected, set to null
and/or disposed on another thread during that tiny gap between testing
for null and the event being called.

The value of the PathChanged field may be changed, by the adding or
removal of an event handler. Note: it's not that the instance is
modified; the field gets a whole new instance.

So you can save the current instance reference in a local variable
without fear of _that_ instance being modified. It can't be, because
the type is immutable. Even if some other thread modifies the event
itself, the only way to do that is to replace the entire event field's
value with a new delegate instance, constructed based on the old one.

This is one of the very useful aspects to immutable types. They make
thread-safe programming much easier, because it's not possible for an
instance of something to change in one thread while another thread is
trying to use. They can't change at all.

It's no panacea, but you can see that in a context like this, simply
saving the current value to a local variable is sufficient to protect
one thread from the operations of another.

Pete

 

[Jeff Gaines]

It's no panacea, but you can see that in a context like this, simply
saving the current value to a local variable is sufficient to protect one
thread from the operations of another.

Many thanks Peter, that sentence does it for me in language I can
understand

I have saved your post and will work on understanding it in more depth.

 

[Harlan Messinger]

Family Tree Mike wrote:
On 1/2/2010 7:57 AM, Jeff Gaines wrote:

[...]
protected virtual void OnPathChanged(object sender, JTagData e)
{
if (PathChanged != null)
PathChanged(sender, e);
}

Is the threading issue the obvious one that if PathChanges is found to
be non-null, it can still have become non-null by the time it's fired?
Yes.

To the OP: if you are trying to clean things up, you might as well
make the above method thread-safe:

protected virtual void OnPathChanged(object sender, JTagData e)
{
PathChangedEventHandler handler = PathChanged;

if (handler != null)
PathChanged(sender, e);
}

I'm not seeing how this helps. Would you mind explaining it?

Well, it might have helped if I had actually fixed up the invocation so
that it was correct for that pattern. The above should read:

protected virtual void OnPathChanged(object sender, JTagData e)
{
PathChangedEventHandler handler = PathChanged;

if (handler != null)
handler(sender, e);
}

Perhaps that's enough to answer your question? If not, then…
Yep!


The delegate type is a reference type, so assignment is atomic. So,
storing the current value locally ensures that even if the actual field
should change before the delegate can be invoked, the invocation takes
place with a delegate reference that is guaranteed not to change.

There is, of course, a race condition. But that would be true
regardless. It's important for clients of events in a multi-threaded
context to not assume that just because they have unsubscribed from an
event in one thread, their handler still won't be called later. The
only way to provide that guarantee is for the client itself to know
exactly how the event might be raised, and to handle synchronization
itself so that event cannot be raised at the same time other code might
be unsubscribing.

Yup. I figured that much out during one of my early forays into
multithreading. Just because the exact moment when a handler's removal
was achieved came before the realization of a particular thread's
"intention" to call the handler, if the timing isn't critical--if things
would have been just as well if, as it happened, the removal didn't
happen until after that thread had made its way through the
handler--then it isn't a critical situation. It's like a store that
closes at 9, but that still gives customers still in the store when 9
rolls around 10 more minutes to reach the checkout line.

 

[Harlan Messinger]

I think you're misusing the word "allocating". The local variable is
_assigned_ the value of the PathChanged field, which is the field
automatically generated by the compiler when you declare an event of the
same name (in the class where the event is declared, you get the
field…outside the class, you get the event).

No allocation happens in the variable's initialization.


Like the String class, the delegate types are immutable. Modification
of an existing delegate instance, such as adding or removing another
delegate instance (i.e. the type of operation that typically occurs
during the add and remove operations for an event), always yield a whole
new delegate instance, while the previous one remains unchanged.

I'm realizing that I'm confused by the nomenclature. Is an event not a
separate thing from a delegate type? I thought

internal event PathChangedEventHandler PathChanged;

meant that PathChanged is an event with associated delegate type
PathChangedEventHandler. But you're assigning the event itself to a
variable of delegate type.

 

[Peter Duniho]

I'm realizing that I'm confused by the nomenclature. Is an event not a
separate thing from a delegate type?

Yes, it is. But…

I thought

internal event PathChangedEventHandler PathChanged;

meant that PathChanged is an event with associated delegate type
PathChangedEventHandler. But you're assigning the event itself to a
variable of delegate type.

No, I'm not. I'm assigning the field that stores the current state of
the event to a variable of a delegate type. That field has the same
delegate type as that for the event.

To elaborate…

In the most common case, we take advantage of auto-implemented events.
They are a lot like auto-implemented properties: the compiler generates
the implementation.

But! There's still implementation!

The compiler provides access to that implementation by providing two
completely different meanings to the identifier "PathChanged" (or
whatever you've named your event). As I wrote before, when you use that
name in code outside the class that declares the event, it refers to the
event.

But, when you use that name in code inside the class, you aren't
referring to the event. Instead, you're referring to the private field
where the event's state is itself stored. That's what the code I posted
does. It's the only reason you can even use an auto-implemented event.
After all, the only thing an event can do is either add something to
itself or remove something from itself. It's not readable, and you
can't invoke it.

Hence, the dual meaning of the identifier naming the event, depending on
context.

Pete

 

[RayLopez99]

Hence, the dual meaning of the identifier naming the event, depending on
context.

Pardon me for interrupting, but it seems both of you are making a
mountain out of a molehill with semantics. Who cares? Just test your
code, and if it's not thread safe (an easy way to check thread
safeness is to insert this.Thread.Sleep(0); throughout your code) just
throw the stuff that's not into your anonymous asynchronous delegate
portion.

My two cents, not that it matters.

RL