HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Having children call parent's public events with data generated from a protected event

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
protectedchildrengeneratedeventspublicwithhavingparentcallfrom

Problem

I don't know that my title describes the code I want reviewed. Hopefully the code is explanatory.

I have an abstract parent class with two children. The parent listens to some hardware notifications and, when the appropriate hardware is connected/removed/fried in butter etc.., raises an event to its children who perform some processing with the data from the change.

The children then raise a public event on the parent with the processed data, which is what I'm unsure of. Here are the (stripped down) classes.

public abstract class DeviceDetector
{
    public event Action DeviceArrived;
    public event Action DeviceRemoved;
    protected void OnDeviceChange(TEventArgs e)
    {
         if (e.Arrival && DeviceArrived != null)
             DeviceArrived(e);
         if (!e.Arrival && DeviceRemoved != null)
             DeviceRemoved(e);
    }

    protected event Action DeviceArrival;
    protected event Action DeviceRemoval;

}


Each of the two children is defined as follows:

public class SomeDescriptiveChildName: DeviceDetector
{
    public SomeDescriptiveChildName()
    {
        this.DeviceArrival += new Action(HandleProtectedEvent);
        this.DeviceRemoval += new Action(HandleProtectedEvent);
    }
    private void HandleProtectedEvent(string e)
    {
         //Class names have been changed to protect the innocent.
         DeviceChild1EventArgs e = new DeviceChild1EventArgs(); 
         base.OnDeviceChange(e);
    }
}

public class AnotherDescriptiveChildName: DeviceDetector
{
    public AnotherDescriptiveChildName()
    {
        this.DeviceArrival += new Action(HandleProtectedEvent);
        this.DeviceRemoval += new Action(HandleProtectedEvent);
    }
    private void HandleProtectedEvent(string e)
    {
         DeviceChild2EventArgs e = new DeviceChild2EventArgs(); 
         base.OnDeviceChange(e);
    }
}


The DeviceChild1EventArgs and DeviceChild2EventArgs are classes inheriting from EventArgs to represent the data from each ch

Solution

The events in the base class don't follow the standards:

public event Action DeviceArrived;
public event Action DeviceRemoved;

protected event Action DeviceArrival;
protected event Action DeviceRemoval;


I'd be expecting something like this:

public event EventHandler AfterDeviceArrived;
public event EventHandler AfterDeviceRemoved;

protected event EventHandler BeforeDeviceArrived;
protected event EventHandler BeforeDeviceRemoved;


Couple points:

  • Events can be declared with any delegate type (Action works), but the convention is to use an EventHandler or EventHandler delegate.



  • Convention for naming events that come in pairs (raised before and after such or such "thing" happens) is to prefix the event names with "Before" and "After", and use past tense as you've done.



  • OnDeviceChange should be called OnDeviceChanged.. but it's not clear why the method needs to raise both ...Arrived and ...Removed events without [apparently] doing anything in-between.



A base class shouldn't know, or care, about whether it has derived classes or not. You've got that right, but a derived class that's listening for events raised in the base class, doesn't make sense to me.

I don't think events are the appropriate way to accomplish what you're trying to do here. That said I'm not sure exactly what you're trying to accomplish (trimmed code does that ;) - but I'd venture to suggest templated methods instead of events.

In the base class:

// todo: remove notion of "event args"
protected abstract void OnDeviceArrived(TEventArgs e);
protected abstract void OnDeviceRemoved(TEventArgs e);


The base class can call these methods even though they're not implemented - abstract methods must be implemented in the derived classes for the code to even compile.

So instead of this:

if (e.Arrival && DeviceArrived != null)
         DeviceArrived(e);
     if (!e.Arrival && DeviceRemoved != null)
         DeviceRemoved(e);


You just have that:

if (e.Arrival) 
    {
        OnDeviceArrived(e);
    }
    else
    {
        OnDeviceRemoved(e);
    }


And the derived classes have the implementations:

protected override void OnDeviceArrived(TEventArgs e)
{
    // ...
}

protected override void OnDeviceRemoved(TEventArgs e)
{
    // ...
}

Code Snippets

public event Action<TEventArgs> DeviceArrived;
public event Action<TEventArgs> DeviceRemoved;

protected event Action<string> DeviceArrival;
protected event Action<string> DeviceRemoval;
public event EventHandler<TEventArgs> AfterDeviceArrived;
public event EventHandler<TEventArgs> AfterDeviceRemoved;

protected event EventHandler<SomeEventArgsClass> BeforeDeviceArrived;
protected event EventHandler<SomeEventArgsClass> BeforeDeviceRemoved;
// todo: remove notion of "event args"
protected abstract void OnDeviceArrived(TEventArgs e);
protected abstract void OnDeviceRemoved(TEventArgs e);
if (e.Arrival && DeviceArrived != null)
         DeviceArrived(e);
     if (!e.Arrival && DeviceRemoved != null)
         DeviceRemoved(e);
if (e.Arrival) 
    {
        OnDeviceArrived(e);
    }
    else
    {
        OnDeviceRemoved(e);
    }

Context

StackExchange Code Review Q#48174, answer score: 3

Revisions (0)

No revisions yet.