3

In the typical event design, you have a public event with a protected virtual event raiser. In my project, though, I have a main class that essentially functions as a manager for several other classes. Is there any reason I shouldn't set the class up like this, so that clients only have to subscribe to the manager class's event and nothing else? (Note: this is typed off-the-cuff based on my actual code, so forgive any possible minor coding errors.)

public class Manager
{
    public event EventHandler<WarningEventArgs> Warning;

    // Could also be internal or protected internal
    // Updated based on comments below.
    public void RaiseWarning(IMessageSource sender, string warning)
    {
        // this.Warning?.Invoke(sender, new WarningEventArgs(warning);
        this.Warning?.Invoke(this, new WarningEventArgs(sender, warning);
    }
}

public class Managed
{
    public Managed(Manager manager)
    {
        this.Manager = manager;
    }

    public Manager Manager { get; }

    public void JustSoYouKnow()
    {
        this.Manager.RaiseWarning(this, "Something happened you should know about.");
    }
}

3 Answers3

0

Technically you can do this, but in my opinion it's a poor design choice. Events are supposed to inform outer subscribers that something happened inside an object of your class. So, only class that defines an event must know and make a decision when to raise the event.

Oleksandr Kobylianskyi
  • 3,281
  • 20
  • 33
  • While that's the traditional thinking, I have to wonder how this differs from simply bubbling up an event from Managed to Manager (apart from the fact that the Managed class doesn't waste time actually raising events of its own that only the Manager is likely ever going to listen to). –  Dec 25 '16 at 20:10
0

I think, this pattern can live but with small modification:

public class Manager: IMessenger, IMessageSender
{
   public event EventHandler<WarningEventArgs> Warning;

   // Could also be internal or protected internal
   public void RaiseWarning(object sender, string warning)
   {
      this.Warning?.Invoke(sender, new WarningEventArgs(warning);
   }
}

public interface IMessenger{
    event EventHandler<WarningEventArgs> Warning;
}    

public interface IMessageSender
{
    void RaiseWarning(object sender, string warning);
}

In this case you can provide your class as a IMessenger to receivers and IMessageSender to message generators. The Manager in this case, plays role of message deliverer to classes that do not (and should not) know about each other.

Yuriy Tseretyan
  • 1,676
  • 16
  • 17
  • If I'm understanding your intent correctly, I could then actually change the event declaration to use a strongly-typed event (http://stackoverflow.com/questions/1046016/event-signature-in-net-using-a-strong-typed-sender) to ensure nobody but designated IMessageSenders actually raises the event, i.e. ```public event StrongEventHandler Warning``` along with ```public void RaiseWarning(IMessageSender sender, string warning)```. –  Dec 25 '16 at 20:01
  • 1
    In your example I would replace `IMessageSender` to `IMessageSource` because `IMessageSender` is used by Manager. – Yuriy Tseretyan Dec 25 '16 at 20:08
0

No that design makes no sense. Imagine this:

Manager mgr = new Manager();

// What if I do this?
mgr.RaiseWarning(null, "Something happened");

Now your Manager class is going to inform every subscriber that "Something happened". So anyone who is using your class can trigger this then what is the point of it?

If we follow your approach, imaging you built a BadButton with a Click event like that. Anyone can call the Click method even though no click occured.

Can I make them internal?

If you make it internal, depending on the caller, they can still pass whatever they want as the sender. If I am using your Manager class and I have subscribed to the event, I would expect sender to be of type Manager. If you let other classes do that and you are sending me null values or XClass and YClass then I will not be too happy. Imagine you have subscribed to the Button click event and you cast the sender as Button and it fails because the sender is Employee...

That is why the convention is to create a proteced OnEvent, for example, OnClick or OnPaint and allow the derived classes to pass the EventArgs to it.

CodingYoshi
  • 25,467
  • 4
  • 62
  • 64
  • That's why I said it could be internal or protected internal, so that only callers who are trusted not to do silly things would actually do so. See Yuri's response for other refinements to the approach. –  Dec 25 '16 at 20:25
  • What I wanted to clarify is that it should not be public or internal. Your code is making it seem like it could be public or internal but it cannot be either of those. As for Yuri's response, I am not sure where you are both going with this. – CodingYoshi Dec 25 '16 at 20:32
  • Why could it not be internal? I'm not understanding your point, I think. If an external client executed the code in your original example, and the RaiseWarning method were internal (or protected internal), then the client raising it's own warnings would no longer be possible unless they were deriving from my class, in which case there's no issue. –  Dec 25 '16 at 20:40
  • Okay, I see what you're saying now. There's no reason the sender couldn't be a Manager object; if needed, I could then add a RealSender to the warning eventargs. In this case, the sender is intended to be informational only (e.g., for reporting to the user). The caller isn't intended to interact with the sender in any way. By making RealSender an interface, similar to what Yuri and I discussed, it doesn't violate the expectation of Sender being a Manager, and the code contract provided by the interface doesn't promise anything more than whatever the real sender provides. –  Dec 25 '16 at 21:45
  • And in anticipation of your next question, the point here is to centralize warnings in a "set it and forget it" fashion, rather than having them be raised by the thousands of individual and collection objects that are likely to be created during the lifetime of the Manager object, and have the client have to worry about subscribing to every last one of them. –  Dec 25 '16 at 21:53
  • 1
    Robin maybe your question should be about what you want to achieve. I am sure there are better ways to achieve it. Sure you can achieve it this way but it just doesn't feel right. – CodingYoshi Dec 25 '16 at 23:07
  • If you can think of any better ways, I'm all ears, but this seems like a reasonable approach to me, given the design of my actual classes (as opposed to the stripped down presentation I've got here just to demonstrate the question). –  Dec 25 '16 at 23:45
  • Let us [continue this discussion in chat](http://chat.stackoverflow.com/rooms/131453/discussion-between-robinhood70-and-codingyoshi). –  Dec 26 '16 at 00:07