views:

54

answers:

2

Hello,

Can you see downsides to this one-liner other than the fact that multiple uses of it would violate the DRY principle? It seems straightforward but the fact that I haven't seen others propose it makes me wonder if there's a downside to it.

This bit of code creates a WeakReference to a method and then registers an event handler that invokes the reference's target.

SomeEvent += (sender, e) => ((Action)(new WeakReference((Action)ProcessEvent)).Target)();

Thanks,
Ben

+1  A: 

Its not very readable. And if you have to debug it by stepping through, any one of those actions could fail, but that single line would fail. Also, you'd only get that single line referenced in a stack trace.

You generally want to avoid doing too many things in a single line for maintainability.

Tim Coker
+1  A: 

I don't think that pattern does what you expect. Are you trying to prevent the event from holding a reference to the current object so as to prevent memory leaks? The lambda expression will capture the value of this in order to evaluate ProcessEvent (assuming ProcessEvent is an instance method), so you will still have the leak. This code is the same as doing SomeEvent += (sender, e) => ProcessEvent();.

You may be trying to do something more like this (which also isn't what you want):

var reference = new WeakReference((Action)ProcessEvent);
SomeEvent += (sender, e) => ((Action)reference.Target)();

Now the lambda expression will capture the WeakReference, so you won't have a strong reference to this. Unfortunately, nothing else is referencing the delegate created from ProcessEvent, so it will be removed on the next GC even if this is still alive. (This also doesn't check for Target being null).

You could try something like this:

public EventHandler MakeWeakHandler(Action action, Action<EventHandler> remove)
{
    var reference = new WeakReference(action.Target);
    var method = action.Method;
    EventHandler handler = null;
    handler = delegate(object sender, EventArgs e)
    {
        var target = reference.Target;
        if (target != null)
        {
            method.Invoke(target, null);
        }
        else
        {
            remove(handler);
        }
    };
    return handler;
}

and then use it like this:

SomeEvent += MakeWeakHandler(ProcessEvent, h => SomeEvent -= h);

That will keep a weak reference to the receiver of ProcessEvent, and will automatically remove the event handler from the event after it has been collected, which should prevent memory leaks as long as the event is raised regularly.

Quartermeister