SerialSebMaking the case against OnXxx
[Updated the code sample for the raising event method. See my response to the comments that were made.]
Some people, like Jeremy, really don't like events. I have to admit having a nearly-fanatic interest in good event patterns. One of such patterns I've seen used and misused continuously is the OnXxx pattern. Let's have a quick look at what the MSDN gods have to say about this.
You raise the event by calling the protected OnEventName method in the class that defined the event, or in a derived class. The OnEventName method raises the event by invoking the delegates, passing in any event-specific data. The delegate methods for the event can perform actions for the event or process the event-specific data.
Note:
The protected OnEventName method also allows derived classes to override the event without attaching a delegate to it. A derived class must always call the OnEventName method of the base class to ensure that registered delegates receive the event.When you want to handle events raised in another class, you add delegate methods to the event.
So far so good. Let's open reflector and check how well is this pattern implemented. First candidate (selected through my advanced AI randomisation algorythm called click until you find a class with an OnXxx patern), FileSystemWatcher.
protected void OnChanged(FileSystemEventArgs e)
{
FileSystemEventHandler onChangedHandler = this.onChangedHandler;
if (onChangedHandler != null)
{
if ((this.SynchronizingObject != null) && this.SynchronizingObject.InvokeRequired)
{
this.SynchronizingObject.BeginInvoke(onChangedHandler, new object[] { this, e });
}
else
{
onChangedHandler(this, e);
}
}
}
So far so good. Let's stay within mscorlib (just in case the pattern was team specific), and have a look at DictionaryBase.
protected virtual void OnClear()
{
}
Oh. Where's the event call? Let's see what the method is described as doing.
Performs additional custom processes before clearing the contents of the DictionaryBase instance.
Wait a second, I thought we were supposed to use OnXxx for raising events, not to change stuff. Ok, let's move on to the ado.net team, and have a look at DataSet, and for the same price I give you not one but two methods.
protected internal void RaisePropertyChanging(string name)
{
this.OnPropertyChanging(new PropertyChangedEventArgs(name));
}
protected virtual void OnPropertyChanging(PropertyChangedEventArgs pcevent)
{
if (this.onPropertyChangingDelegate != null)
{
this.onPropertyChangingDelegate(this, pcevent);
}
}
Alright... Now I'm getting quite confused. I can call RaiseXxx(name) which in turns call the proper OnXxx pattern that calls the event. This is now getting quite messy. Let's see what the guys in the asp.net team do with the DataSourceControl.
protected virtual void RaiseDataSourceChangedEvent(EventArgs e)
{
this.OnDataSourceChangedInternal(e);
this.OnDataSourceChanged(e);
}
Note that both OnXxx are now private methods... At least in DataSet you have two ways to do the same thing, one legal and one not legal (again, according to the msdn documentation). Not convinced yet? Ok, let's see what the Winforms guy have been doing and have a look at Control. I'll point to two examples.
[EditorBrowsable(EditorBrowsableState.Advanced)]
protected virtual void OnNotifyMessage(Message m)
{
}
And example 2.
[EditorBrowsable(EditorBrowsableState.Advanced)]
protected void RaisePaintEvent(object key, PaintEventArgs e)
{
PaintEventHandler handler = (PaintEventHandler)base.Events[EventPaint];
if (handler != null)
{
handler(this, e);
}
}
Note that the OnPaint event exists and does mostly the same thing. Isn't it great to have that much flexibility? Ok, last but not least, let's see what the latest greatest brings us with WPF's UIElement.
public void RaiseEvent(RoutedEventArgs e)
{
if (e == null)
{
throw new ArgumentNullException("e");
}
e.ClearUserInitiated();
this.RaiseEventImpl(e);
}
protected virtual void OnMouseLeave(MouseEventArgs e)
{
}
Can you make any sense of the OnXxx notation? No? Me neither, neither do my developers. So I hereby propose (again) to just get done with it already and *aknowledge* that OnXxx is an anti-pattern used either for raising or for handling events, is overused and misused, and should die a long and painful death. I propose the simpler and more semantically correct syntax:
public class DoingEventsProperly
{
public DoingEventsProperly()
{
this.SomethingChanged += HandleSomethingChangedEvent;
}
public event EventHandler<PropertyChangedEventArgs> SomethingChanged = (src, ea) => { };
protected virtual void RaiseSomethingChangedEvent(PropertyChangedEventArgs e) { SomethingChanged(this, e); }
protected virtual void HandleSomethingChangedEvent(object src, PropertyChangedEventArgs ea) { }
}
What this class achieves is covering all the scenarios we just encountered.
- The event handler is defaulted to an empty anonymous method, so it cannot be null, which means RaiseSomethingChangedEvent doesn't need to check for null.
- The semantic of the RaiseXxxEvent method is simple: It raises the event. Want to cancel the event? Override RaiseXxxEvent.
- The semantic of HandleXxxEvent is simple: It aint raising the event! Want to do some stuff in your class when the event is raised, override that method.
And it's also easy to explain: Call it Raise when you're Raising an event and call it Handle if you're handling an event. I think the semantic complexity is achievable.
I may go into the other patterns such as explicitly cancelable events for consumers and explicitly cancelable by contract for inheritance cancellation another day.
And remember: The best way to combat an anti-pattern is to stop using it.