views:

1305

answers:

6

Hi

I'm learning about Events / Delegates in C#. Could I ask your opinion on the naming/coding style I've chosen (taken from the Head First C# book)?

Am teaching a friend about this tomorrow, and am trying to come up with the most elegant way of explaining the concepts. (thought the best way to understand a subject is to try and teach it!)

class Program
    {
        static void Main()
        {
            // setup the metronome and make sure the EventHandler delegate is ready
            Metronome metronome = new Metronome();

            // wires up the metronome_Tick method to the EventHandler delegate
            Listener listener = new Listener(metronome);
            metronome.OnTick();
        }
    }


public class Metronome
    {
        // a delegate
        // so every time Tick is called, the runtime calls another method
        // in this case Listener.metronome_Tick
        public event EventHandler Tick;

        public void OnTick()
        {
            while (true)
            {
                Thread.Sleep(2000);
                // because using EventHandler delegate, need to include the sending object and eventargs 
                // although we are not using them
                Tick(this, EventArgs.Empty);
            }
        }
    }


public class Listener
    {
        public Listener(Metronome metronome)
        {
            metronome.Tick += new EventHandler(metronome_Tick);
        }

        private void metronome_Tick(object sender, EventArgs e)
        {
            Console.WriteLine("Heard it");
        }
    }

n.b. Code is refactored from http://www.codeproject.com/KB/cs/simplesteventexample.aspx

A: 

Looks good, aside from the fact that OnTick doesn't follow the typical event invocation model. Typically, On[EventName] raises the event a single time, like

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

Consider creating this method, and renaming your existing "OnTick" method to "StartTick", and instead of invoking Tick directly from StartTick, call OnTick(EventArgs.Empty) from the StartTick method.

Adam Robinson
Cheers..appreciated
Dave
+6  A: 

There are a few points that I would mention:

Metronome.OnTick doesn't seem to be named correctly. Semantically, "OnTick" tells me it will be called when it "Tick"s, but that isn't really what's happening. I would call it "Go" instead.

The typically accepted model, however would be to do the following. OnTick is a virtual method that raises the event. This way, you can override the default behavior in inherited classes easily, and call the base to raise the event.

class Metronome
{
    public event EventHandler Tick;

    protected virtual void OnTick(EventArgs e)
    {
        //Raise the Tick event (see below for an explanation of this)
        var tickEvent = Tick;
        if(tickEvent != null)
            tickEvent(this, e);
    }

    public void Go()
    {
        while(true)
        {
            Thread.Sleep(2000);
            OnTick(EventArgs.Empty); //Raises the Tick event
        }
    }
}


Also, I know this is a simple example, but if there are no listeners attached, your code will throw on Tick(this, EventArgs.Empty). You should at least include a null guard to check for listeners:

if(Tick != null)
    Tick(this, EventArgs.Empty);

However, this is still vulnerable in a multithreaded environment if the listener is unregistered between the guard and the invocation. The best would be to capture the current listeners first and call them:

var tickEvent = Tick;
if(tickEvent != null)
    tickEvent(this, EventArgs.Empty);
lc
Thanks for a detailed answer
Dave
Alternative to guard is to add "= delegate {};" to Tick declaration (see http://stackoverflow.com/questions/231525/raising-c-events-with-an-extension-method-is-it-bad/231536#231536)
Benjol
Note that the vulnerability is not _limited_ to multithreaded environments. It's possible (if sociopathic) for an event handler to remove all handlers from an event, resulting in a crash when the handler completes and the event invocation attempts to run the next (now non-existent) event.
Greg D
+2  A: 

A point I have found after using events in .Net for many years is the repetitive need to check the event for a null handler on every invocation. I'm yet to see a piece of live code that does anything but not call the event if it is null.

What I have started doing is to put a dummy handler on every event I create to save the need to do the null check.

public class Metronome
{
    public event EventHandler Tick =+ (s,e) => {};

    protected virtual void OnTick(EventArgs e)
    {
        Tick(this, e);  // now it's safe to call without the null check.
    }
}
sipwiz
Good thoughts... cheers.
Dave
One thing to point out is that this does not work with serialization. The empty delegate idea is discussed thoroughly at http://stackoverflow.com/questions/9033/hidden-features-of-c/9282#9282
emddudley
+4  A: 

Microsoft has actually written extensive set of naming guidelines and put it in the MSDN library. You can find the articles here: Guidelines for Names

Aside from the general capitalization guidelines, here is what it has for 'Events':

Do name events with a verb or a verb phrase.

Do give event names a concept of before and after, using the present and past tense. For example, a close event that is raised before a window is closed would be called Closing and one that is raised after the window is closed would be called Closed.

Do not use Before or After prefixes or suffixes to indicate pre and post events.

Do name event handlers (delegates used as types of events) with the EventHandler suffix.

Do use two parameters named sender and e in event handler signatures.

The sender parameter should be of type Object, and the e parameter should be an instance of or inherit from EventArgs.

Do name event argument classes with the EventArgs suffix.

YotaXP
+1  A: 

I would say the best guide to events in general, including naming conventions, is here.

It is the convention I have adopted, briefly:

  • Events names are typically terminated with a verb ending with -ing or -ed (Closing/Closed, Loading/Loaded)
  • The class which declares the event should have a protected virtual On[EventName] which should be used by the rest of the class for raising the event. This method can be also used by subclasses to raise the event, and also overloaded to modify the event-raising logic.
  • There is often confusion about the use of 'Handler' - for coherence, all delegates should be postfixed with Handler, try to avoid calling the methods which implement the Handler 'handlers'
  • The default VS naming convention for the method which implements the handler is EventPublisherName_EventName.
Benjol
+1  A: 

Interesting how Microsoft seem to break their own naming conventions with Visual Studio generated event handler names.

http://msdn.microsoft.com/en-us/library/h0eyck3s(VS.71).aspx

Dan