5
\$\begingroup\$

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<TEventArgs>
{
    public event Action<TEventArgs> DeviceArrived;
    public event Action<TEventArgs> DeviceRemoved;
    protected void OnDeviceChange(TEventArgs e)
    {
         if (e.Arrival && DeviceArrived != null)
             DeviceArrived(e);
         if (!e.Arrival && DeviceRemoved != null)
             DeviceRemoved(e);
    }

    protected event Action<string> DeviceArrival;
    protected event Action<string> DeviceRemoval;

}

Each of the two children is defined as follows:

public class SomeDescriptiveChildName: DeviceDetector<DeviceChild1EventArgs>
{
    public SomeDescriptiveChildName()
    {
        this.DeviceArrival += new Action<string>(HandleProtectedEvent);
        this.DeviceRemoval += new Action<string>(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<DeviceChild2EventArgs>
{
    public AnotherDescriptiveChildName()
    {
        this.DeviceArrival += new Action<string>(HandleProtectedEvent);
        this.DeviceRemoval += new Action<string>(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 child.

My question is this: Is there a better way to represent the data passed to the children through DeviceArrival and DeviceRemoval (in this example I used string, it's not a string) without the use of generics etc...

To me this seems somewhat like overkill, but it's the only way I could think of doing this.

\$\endgroup\$
  • \$\begingroup\$ Are DeviceChild1 and DeviceChild2 actual class names? \$\endgroup\$ – Mathieu Guindon Apr 25 '14 at 17:56
  • 3
    \$\begingroup\$ Gods no. I had a feeling someone would mention that. I'll edit it... \$\endgroup\$ – Brandon Apr 25 '14 at 17:57
  • \$\begingroup\$ @Brandon these names are definitely more descriptive :D, you sir made my day. \$\endgroup\$ – Vogel612 Apr 25 '14 at 17:59
3
\$\begingroup\$

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

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

protected event Action<string> DeviceArrival;
protected event Action<string> DeviceRemoval;

I'd be expecting something like this:

public event EventHandler<TEventArgs> AfterDeviceArrived;
public event EventHandler<TEventArgs> AfterDeviceRemoved;

protected event EventHandler<SomeEventArgsClass> BeforeDeviceArrived;
protected event EventHandler<SomeEventArgsClass> BeforeDeviceRemoved;

Couple points:

  • Events can be declared with any delegate type (Action<T> works), but the convention is to use an EventHandler or EventHandler<TEventArgs> 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)
{
    // ...
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.