patterncsharpMinor
Having children call parent's public events with data generated from a protected event
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.
Each of the two children is defined as follows:
The
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 chSolution
The events in the base class don't follow the standards:
I'd be expecting something like this:
Couple points:
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:
The base class can call these methods even though they're not implemented -
So instead of this:
You just have that:
And the derived classes have the implementations:
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 (
Actionworks), but the convention is to use anEventHandlerorEventHandlerdelegate.
- 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.
OnDeviceChangeshould be calledOnDeviceChanged.. 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.