3
\$\begingroup\$

Our monolithic WinForm application is getting a face lift. One current challenge that we are restructuring how we setup events. I rolled my own event manager class to handle the subscriptions and unsubscriptions to prevent leaks whenever developers write code.

I ran into several challenges with storing generic values in non-generic types. So I came up with a pattern that allowed me to do it. Not sure if I am creating a code smell while doing it so I wanted someone to review and ask questions.

Base View

public abstract class View : Form, IView
{
    protected readonly EventManager EventManager;

    public View()
    {
        EventManager = new EventManager(this);
    }

    protected abstract void RegisterEvents();
}

Concrete Class

public class MyView : View
{
    protected override void RegisterEvents()
    {
        EventManager.Attach(button1_Click, handler => button1.Click += handler, button1.Click -= handler); 
    }
}

Event Viewer

public class EventManager : IDisposable
{
    private IList<TrackedEvent> Events = new List<TrackedEvent>();
    private IDisposable Owner;

    public EventManager(IDisposable owner)
    {
        Owner = owner;
    }

    public void Attach<TEventArgs>(EventHandler<TEventArgs> handler, Action<EventHandler<TEventArgs>> addEvent, Action<EventHandler<TEventArgs>> removeEvent = null)
    {
        Events.Add(GenericTrackedEvent<TEventArgs>.Register(@delegate, handler.Method.Name, removeEvent));            
    }
            
    public void Attach(EventHandler handler, Action<EventHandler> addEvent, Action<EventHandler> removeEvent = null)
    {           
        Events.Add(TrackedEvent.Register(@delegate, handler.Method.Name, removeEvent));           
    }

    public void Dispose() { ... }

    private class TrackedEvent : IDisposable
    {
        protected EventHandler Handler { get; set; }
        protected string MethodName { get; set; }
        protected Action<EventHandler> Unsubscription { get; set; }

        protected TrackedEvent() { }

        private TrackedEvent(EventHandler handler, string methodName, Action<EventHandler> removeEvent)
        {
            Handler = handler;
            MethodName = methodName;
            Unsubscription = removeEvent;
        }

        internal virtual void Remove()
        {
            Unsubscription(Handler);
        }

        internal static TrackedEvent Register(EventHandler handler, string methodName, Action<EventHandler> removeEvent)
        {
            return new TrackedEvent(handler, methodName, removeEvent);
        }

        public void Dispose()
        {
            Dispose(true);
        }

        protected virtual void Dispose(bool disposing)
        {
            Handler = null;
            Unsubscription = null;
        }
    }

    private class GenericTrackedEvent<T> : TrackedEvent
    {
        protected new EventHandler<T> Handler { get; private set; }
        protected new Action<EventHandler<T>> Unsubscription { get; private set; }

        internal override void Remove()
        {
            Unsubscription(Handler);
        }

        private GenericTrackedEvent(EventHandler<T> handler, string methodName, Action<EventHandler<T>> removeEvent)
        {
            Handler = handler;
            MethodName = methodName;
            Unsubscription = removeEvent;
        }

        internal static TrackedEvent Register(EventHandler<T> handler, string methodName, Action<EventHandler<T>> removeEvent)
        {
            return new GenericTrackedEvent<T>(handler, methodName, removeEvent);
        }

        protected override void Dispose(bool disposing)
        {
            Handler = null;
            Unsubscription = null;

            base.Dispose(disposing);
        }
    }
}

So several things to point out and as to why:

  1. Some methods have been shortened for brevity.
  2. TrackedEvent holds onto the unsubscription e.g: the removeEvent handler.
  3. Previously implementations I struggled with storing generic types in a non-generic class without constant boxing/unboxing.
  4. The two static methods "Register" will return a TrackedEvent class because its constructors are private. I don't want anyone creating one outside of my implementation.
  5. TrackedEvent only handles non-generics.
  6. GenericTrackedEvent handles generics by hiding our base properties and overriding key properties.
  7. Unexpectedly, inheritance is working correctly when calling the remove method. My initial expectation was the GenericTrackedEvent was going to use the underlying TrackedEvent.Unsubscription and TrackedEvent.Handler properties but I was wrong - and that was okay.
  8. Someone is going to ask - why are you passing over the MethodName? Because I actually wrap my handlers in a new delegate so I can capture when event fire and log them to any source I choose. When I wrap my delegates in the attach method it carries that name over instead of the original one passed in the handler parameter. Instead of masking that method, I figured I'd keep it. Again some code was remove because I do not deem it relevant to the initial question.

Seems like we've implemented a duck typing but its usage is abstracted away from developers so I think its okay but wanted several other opinions to weight in.

Thoughts?

\$\endgroup\$
9
  • 1
    \$\begingroup\$ The View and MyView classes are somehow 'lazy' clases (according to what you are showing us) however if in the future they will contain and provide more operations then it isn't a bad design. \$\endgroup\$ – Miguel Avila Aug 25 '20 at 3:52
  • 1
    \$\begingroup\$ You should correct the names: Neither TrackedEvent.Register, GenericTrackedEvent<TEventArgs>.Register nor @delegate exists. \$\endgroup\$ – user73941 Aug 25 '20 at 6:33
  • \$\begingroup\$ @Henrik right - sorry had some last minute edits before writing this. \$\endgroup\$ – Daniel Aug 25 '20 at 13:15
  • \$\begingroup\$ @HenrikHansen furthermore, @delegate does exist in the full implementation. I wrap the original handler with logging so we can track the usage of the event for debugging purposes Doing this causes the new delegate's MethodName to attach itself to that class, instead of the original handler. void @delegate(object s, TEventArgs e) { Debug.WriteLine($"\t{Owner.GetType()} event {handler.Method.Name}<{typeof(TEventArgs)}> has fired with {e}..."); handler(s, e); } \$\endgroup\$ – Daniel Aug 25 '20 at 13:19
  • 1
    \$\begingroup\$ Please do not update the code in your question after receiving answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. It simply gets too messy if we'd allow it. Feel free to ask a follow-up question instead and to add links to both questions back-and-forth. \$\endgroup\$ – Mast Aug 26 '20 at 17:45
1
\$\begingroup\$

I would question what you intend with this system. In my personal experience, when someone writes something like this it's because they have no actual work assigned to them, but don't want their managers to realize that.

Take a step back and ask yourself what is the improvement you gain from:

  1. Requiring an additional class to be present in every form inheritance tree.
  2. Making it so user controls aren't handled by this system (from the inside I mean, since your base class derives from Form).
  3. Requiring a manual line of code to handle creation and removal of events in one specific function (what if I want to add a handler at run-time? What if I want to use post-2002 lambda expressions instead of 6 lines of code for a simple handler? What if I want to use the designer, which handles renaming for me?)
  4. I don't think you even considered early event removal.

You say you want to avoid forgetting to remove event handlers. Any properly written handler doesn't need to be removed, only handlers that strongly root the form object require this (and in this case it's not a memory leak exactly, it's your form not closing). And this system doesn't solve strong rooting, since it stores a non-weak-referenced list of these unsubscribe handlers, which reference your form, thus rooting the form in your object. You're literally creating the problem you're claiming to solve (refer back to my first statement).

You don't show this, but what triggers the unsubscribe? The form's own dispose call? I hope not, because that won't trigger in a problem scenario with this code, and the fact that you mark this "manager" as IDisposable worries me greatly. The FormClosing event? I hope not, because that would prevent close cancellation. Yet another manual call to something? I hope not, because you're supposedly trying to automate ..something, not throw in more manual calls that always have to happen or BOOM.

As to the actual code presented, it's not much, but at the very least write a bit of smart code to handle subscribing and unsubscribing automatically from a handle to the event and handler. You have expressions to parse code provided to your function and generate changed executable code from it, and you have code emitting, between those two you should be able to call your function like this at least:

EventManager.Attach(button1_Click, button1.Click);

Edit: Oh god I just noticed your new field modifier. Use proper OOP and you won't need a GenericTrackedEvent<T> that's literally twice as large as the thing it "derives" from, since it has the previous fields as well as the new ones. And at the very least don't mark them protected, there's no reason for that.

\$\endgroup\$
11
  • 1
    \$\begingroup\$ You cannot pass an event handler as you suggested. You'll receive an error that events can only be assigned on the left hand side of the operator. That was my first approach but it doesn't work. Playing devils advocate - maybe I missed something. How did you get that to work? \$\endgroup\$ – Daniel Aug 25 '20 at 19:04
  • \$\begingroup\$ It's one thing to ask how it's done, it's another to absolutely say "you cannot do this". I didn't implement this because it's rather trivial, but here you go, event attachment by reference: dotnetfiddle.net/9RLRsN. I trust you can do the unsubscription yourself from this, but if not feel free to ask (though Stack Overflow is really the place to ask how to do things, this is just code review). \$\endgroup\$ – Blindy Aug 26 '20 at 15:11
  • \$\begingroup\$ I probably didn't explain it correctly - trying to do this on an event in the same class works, but on a control's event does not. I should have cleared that part up. Expanding on your current suggestion to show the problem: dotnetfiddle.net/7L9wcV That's why I suggested that it couldn't be done. \$\endgroup\$ – Daniel Aug 26 '20 at 15:35
  • \$\begingroup\$ Still doable with very minimal changes: dotnetfiddle.net/zZ0R0W \$\endgroup\$ – Blindy Aug 26 '20 at 15:56
  • 1
    \$\begingroup\$ Might be best to move this to chat. Till then I checked out the code and close calls dispose directly. referencesource.microsoft.com/#System.Windows.Forms/winforms/… \$\endgroup\$ – Daniel Aug 26 '20 at 18:56
-4
\$\begingroup\$

Look into the C# dynamic type which indicates that the variables type can change at runtime.

For example,

dynamic event= "get_credentials";
...
event= 5;
...
event= someFunction;
\$\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.