A life-saver for one-button developers

http://codebetter.com/blogs/jean-paul_boodhoo/archive/2008/02/12/another-handy-shortcut-combination-shift-f10.aspx

I got used to use the menu system with the keyboard to compensate for the trackpad on my macbook pro not having a usable right click (the double tap behavior is erratic at best and unusable). Shift-F10 is going to make my life so much easier!

[Update: I just realized this shortcut works for any application. I should really increase my keyboard shortcut proficiency, especially as I don't seem to have enough space for my mouse anymore in the office...]

Ads

Follow-up on the OnXxx anti-pattern

[Started updating the previous entry, but the reply is chunky enough to go in its own post]

One person on the dotnet-clr mailing list has highlighted, quite accurately, that the RaiseXxxEvent method should really take EventArgs as a parameter instead of a string to ensure it can be swapped with a class inheriting from EventArgs. If you want that kind of extensibility, then yes, put an EventArgs as a parameter. I should update the sample.

A second person in the comments vehemently disagree with my point, so let's go for a simple rebuttal, as I do not believe the arguments to be accurate.

The first part is that the pattern, to be deemed acceptable, should be OnXxx(object src, EventArgs ea).  I have nothing else to say but to point with every single of my examples as none of them seems to "get" the right way of declaring OnXxx. As for this way to be the right way, I've not found any other reference to support Peter's argument.

The Design Guidelines for Developing Class Libraries does mention the void delegateType(object source, eventargs e) signature for event handlers, not for event raisers. As for event raisers, here's a big quote from the same source.

Do use a protected virtual method to raise each event. This is applicable only to non-static events on unsealed classes, not to structures, sealed classes, or static events.

Complying with this guideline allows derived classes to handle a base class event by overriding the protected method. The name of the protected virtual (Overridable in Visual Basic) method should be the same as the event name prefixed with On. For example, the protected virtual method for an event named "TimeChanged" is named "OnTimeChanged".

Important Note:

Derived classes that override the protected virtual method are not required to call the base class implementation. The base class must continue to work correctly even if its implementation is not called.

Do use a parameter that is typed as the event argument class to the protected method that raises an event. The parameter should be named e.

Let's see:

  • protected virtual method for non-static events on unsealed classes: Checked
  • the virtual method *should* be prefixed by On. This is non normative, so I'm still within the guideline: Checked
  • The last bit is quite clear: to be within the guideline, the raise method would be Raise(EventArgs e), and not take an object as the first argument: Checked (after having updated the previous post)

Indeed, the second part talks about passing a string instead of an EventArg, this came from typing quickly an example rather than the focus of the pattern, but point taken and resolved.

The third part is interesting. The reader is oblivious to the fact that the event raising method is called RaiseXxxEvent. Missing the italics is an easy mistake to make so let's move on. Apparently OnClear is a rat hole because it doesn't follow the right pattern (neither do the others as said before, and worst, neither does the design guideline...) OnClear with any other argument can mean anything but OnClear(object, EventArgs) can only mean one thing. So in this case, using On can have several meanings and it's fine, but in the case of Raise, you may never know if someone cannot confuse RaiseTheMountain and RaiseGoingUpEvent, so it's not acceptable. I'll assume the whole diatribe is based on the EventArgs confusion and the use of a string, and the missing Event suffix, so I think the point is clear.

So I'll summarize the argument I made extensively: OnXxx is implemented widely but implemented widely wrongly across mscorlib, asp.net, winforms and WPF. It has a double meaning as either raising (as per the design guidelines) or handling (as per people's use of it). The examples in the previous post show that without . Furthermore, the asp.net language use OnXxx to autowire events to event handlers, just like html events.

The confusion is enough that I choose to split the pattern in two methods with clear intent: HandleXEvent to handle an event, RaiseXEvent to raise an event. And even by doing so, I still adhere to the design guidelines issued by Microsoft. If you find this more confusing than OnXxx, I'm just lost for words.

Ads

Making 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.

Ads