Events best practice: subscription and naming conventions and options

[pisacou]

Dear all,
I plan to rewrite a Java library to C#. This library makes heavy use of custom event handling, i. e. it contains lot of classes XYZ that are in fact Observable, XYZChangeEvent, and interface XYZChangeListener that are implemented by Observers. Frequently, an Observable is an Observer at the same time, and propagates change events that it has received from its Oberservables to its own Observers.
This event handling will be implemented using the C# styl way, i. e. by using events instead of interfaces to couple the Observers and Observables.
I have read various threads in this forum (such as http://www.csharpforums.net/showthread.php/3628-understanding-Delegates-and-Events ) and others, and I think that I have grasped the basic concepts. However, I still feel a bit unsure about some implementation details.
Here is a set of classes that I gave written as prototypes to learn the various options:

namespace EventStuff
{
    public static class EventHelper
    {
        public static void Raise<T>(this EventHandler<T> handler, Object sender, T e) where T: EventArgs{
            if(handler != null){
                handler(sender,e);
            }
        }
    }

    public class ItemChangeEvent: EventArgs
    {
        private Item source;
        
        private string message;
        
        public ItemChangeEvent(Item source, string message)
        {
            this.source = source;
            this.message = message;
        }
        public Item Source{
            get{return source;}
        }
        public string Message{
            get{return message;}
        }
    }
    
    public class Item
    {
        public event EventHandler<ItemChangeEvent> ItemChanged;
        
        private string label;
        
        public Item(string label)
        {
            this.label = label;
        }
        public string Label{
            get{return this.label;}
            set{
                this.label = value;
                OnItemChanged("Label changed");
                this.label = value;
            }
        }
        public void OnItemChanged(string message){
            ItemChanged.Raise(this, new ItemChangeEvent(this,message));
        }
    }
    
    public class ItemChangeListenerWithRef
    {
        private string name;
        
        private Item item;
        
        public ItemChangeListenerWithRef(string name, Item item)
        {
            this.name = name;
            this.item = item;
            item.ItemChanged+=this.ItemChanged;
        }
        public void ItemChanged(Object sender, ItemChangeEvent e){
            Console.WriteLine("I am '" + name + "' with an item reference. Change detected for item " +e.Source + " with message '" + e.Message+"'");
        }
    }

    public class ItemChangeListenerExcludingRef
    {
        private string name;
        

        public ItemChangeListenerExcludingRef(string name)
        {
            this.name = name;
        }
        public void ItemChanged(Object sender, ItemChangeEvent e){
            Console.WriteLine("I am '" + name + "'without an item reference. Change detected for item " +e.Source + " with message '" + e.Message+"'");
        }
    }
    public class ItemEventTest
    {
        public static void Main(string[] args)
        {
            Item first = new Item("My first item");
            ItemChangeListenerExcludingRef noRef = new ItemChangeListenerExcludingRef("First logger no ref");
            first.addItemChangeListener(noRef);
            ItemChangeListenerWithRef withRef = new ItemChangeListenerWithRef("First logger with ref", first);
            first.Listeners += new Item.Listener(noRef.itemChanged);
            first.Label = "My first changed item";
            Console.Write("Press any key to continue . . . ");
            Console.ReadKey(true);
        }
    }
}

Some questions:

Obviously, I do not need any interfaces that declare a by definition public method. An observer just needs a method with a suitable signature. In the two event subscriber classes above, the respective event handler methods are public. However, this would not be necessary for the ItemChangeListenerWithRef instance. An ItemChangeListenerWithRef instance only subscribes to event of those items to which it has reference, and is never used as a subscriber by an external class. It would thus be possible to make the ItemChanged method protected or even private. Is such an approach "allowed", or should such an approach be avoided?

The OnItemChanged method of the Item class gets a string parameter. I have read that the event raising method such be declared to have a single parameter that derives from EventArgs. Is this just a recommendation more like a requirement? The benefit of the OnItemChanged(string) method is that it is shorter, since the EventArgs instance does only need to be created in a single place. In a real world, the Item class will have more than just one or a few properties that could be changed, and whose change will have to raise a ChangeEvent.

I have also read the recommendation that I should forget about custom events, and just the interface INotifyPropertyChanged to implement an event notification mechanism. However, this approach only allows to transfer the sender (as object) of the event and the name of the changed property to the subscribers. Using a custom EventArgs offers far more options to to carry additional informations. Is the use of custom EventArgs still considered good practice? We are not talking about a Gui framework.

Finally, is the naming used for the events, the event raising methods, and for the observers ok?
Thanks for any feedback!

 

[jmcilhinney]

Obviously, I do not need any interfaces that declare a by definition public method. An observer just needs a method with a suitable signature. In the two event subscriber classes above, the respective event handler methods are public. However, this would not be necessary for the ItemChangeListenerWithRef instance. An ItemChangeListenerWithRef instance only subscribes to event of those items to which it has reference, and is never used as a subscriber by an external class. It would thus be possible to make the ItemChanged method protected or even private. Is such an approach "allowed", or should such an approach be avoided?

A method that raises an event should be declared 'protected' because it will never be called directly from outside its type but it may well be called by a derived type. A method that handles an event should be declared 'private' because it will never be called directly, even by its own type.

The OnItemChanged method of the Item class gets a string parameter. I have read that the event raising method such be declared to have a single parameter that derives from EventArgs. Is this just a recommendation more like a requirement? The benefit of the OnItemChanged(string) method is that it is shorter, since the EventArgs instance does only need to be created in a single place. In a real world, the Item class will have more than just one or a few properties that could be changed, and whose change will have to raise a ChangeEvent.

It's a convention rather than a requirement but it's one that you should definitely stick to. As you say, if the EventArgs instance is created inside the event-raising method then it only has to be created in one place but that is actually a weakness rather than a strength. As I said above, the method that raises the event should be declared 'protected' and it should also be declared 'virtual'. That's so that derived classes can not only raise the event but also add custom functionality to an event. If you create the EventArgs instance inside the method that raises the event then a derived class cannot change the way that object is created and it also can't get access to it after the base method returns. An example of where such access would be required is when using a CancelEventArgs or some type that inherits it. You may well need to test that object in an overridden method in the derived class to know whether the event was cancelled or not.

The fact is that you can choose to ignore that convention in situations where you think doing so will not cause an issue but I would suggest that it's a good idea to stick to it all the time. That way, you'll never be caught out by a situation that you thought wouldn't be problematic but turns out to be so and also consistent code is always easier to understand.

I have also read the recommendation that I should forget about custom events, and just the interface INotifyPropertyChanged to implement an event notification mechanism. However, this approach only allows to transfer the sender (as object) of the event and the name of the changed property to the subscribers. Using a custom EventArgs offers far more options to to carry additional informations. Is the use of custom EventArgs still considered good practice? We are not talking about a Gui framework.

I doubt that anyone has ever recommended to avoid custom events altogether. It was likely the case that people were recommending that you simply implement INotifyPropertyChanged in preference to adding a large number of custom events that do nothing more than indicate that a specific property has changed. If you need to do more than that then you need an event that can do what you need and you should go ahead and implement that.

Finally, is the naming used for the events, the event raising methods, and for the observers ok?

A class that inherits EventArgs should always have a name that ends with "EventArgs", thus ItemChangeEvent would be better named ItemChangeEventArgs.

Also, if you define an custom EventArgs type that exists specifically to be used by an event named ItemChanged then that class should be named ItemChangedEventArgs, not ItemChangeEventArgs. The names should match to indicate the relationship.

Finally, if you have a class named Item with a property named Label and you have an event that indicates that the value of that property has changed then the event should be named LabelChanged, not ItemChanged.

As a bonus, if you're going to raise an event that indicates that a property has changed then you need to make sure that the property actually has changed. You don't want to raise the event if the property is assigned its current value. That means an 'if' statement in your property setter. You might like to check this out to see if there's any additional useful information:

http://jmcilhinney.blogspot.com.au/2009/11/defining-and-raising-custom-events.html

 

[pisacou]

Many thanks for your answer. I can follow most of your points, except the private event handler method (that is the "ItemChanged"-method, right?). If that is private, how can the subscription process be programmed from the outside of the class, as described above for the class ItemChangeListenerExcludingRef? Or is such an "externally created subscription" bad practice?
[edit] I just thought about that a bit more. It certainly makes senseto declare this method as private since it should not be called from the outside, and it should also not be called directly from within the class. It should be only called in case of an event. But it should still be possible to create the subscription from the outside. What about a public method like this one in a class like ItemChangeListenerExcludingRef:

public void CreateSubscription(Item item){
    item.ItemChanged+=this.ItemChanged;
}

 

[jmcilhinney]

I've never found myself in a situation where one object needed to register another object as a handler for an event of a third and so I've never needed an event handler to be public. If you did find yourself in such a situation though, I guess the method would need to be public. That's increasing the coupling between types though, for the object handling the event to know that it will be registered by some other type.

 

[pisacou]

Thank you again for your response. If the event handling method in the subscriber is indeed private, the subscriber (an instance of the ItemChangeListenerXYZ classes from the example above) needs to get a reference to the publisher (the Item class) to subscribe to the (public) event. My question is what is the preferred way to achieve that? I found a short example here:https://docs.microsoft.com/en-us/do...ents-that-conform-to-net-framework-guidelines, where the Subscribers get a reference to the Publisher in the constructor. However, this approach does not allow to programatically release the subscription.
From your experience, what is the best way to write subscribers that do not need to subscribe to events for their entire lifetime? I do see two basic approaches: either with a public event handling method, or with two public method to create and release a subscription where the publisher is provided as a method parameter.