patterncsharpMinor

WeakAction Implementation

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

implementationweakactionstackoverflow

Problem

Here's my first attempt at creating a WeakAction using Expressions and Delegates. I would like it to be clear that this code was written after reading serveral blogs and taking snippets from various sources.

The Program class at the bottom shows a few examples of how to use it.

Does anyone have any performance improvements when it comes to creating delegates? I'm still trying to get my head around them. All suggestions and any constructive feedback is welcome.

```
using System;
using System.Linq.Expressions;
using System.Reflection;

namespace WeakAction
{
public class WeakAction where T : class
{
private readonly WeakReference weakRef;
private readonly Type ownerType;
private readonly string methodName;

public WeakAction(object instance, Expression expression)
{
var method = GetMethodInfo(expression);
methodName = method.Name;

if (instance == null)
{
if (!method.IsStatic)
{
throw new ArgumentException("instance cannot be null for instance methods");
}
this.ownerType = method.DeclaringType;
}
else
{
this.weakRef = new WeakReference(instance);
}
}

public T Delegate
{
get { return this.GetMethod(); }
}

public bool HasBeenCollected
{
get
{
return this.ownerType == null &&
(this.weakRef == null || !this.weakRef.IsAlive);
}
}

public bool IsInvokable
{
get { return !HasBeenCollected; }
}

private static MethodInfo GetMethodInfo(Expression expression)
{
LambdaExpression lambda = expression as LambdaExpression;
if (lambda == null)
{
throw new ArgumentException("expression is not Lambd

Solution

Design

Reading the code it looks to me as if there are two separate things mixed. Effectively you have two different code paths through your class - one for static method calls and one for instance method calls which do not have much in common. I'd consider to refactor this into separate classes to simplify some of the logic and separate the concerns. Something along these lines:

public abstract class WeakAction where T : class
{
    protected readonly MethodInfo _Method;

    private class WeakActionStatic : WeakAction
    {
        private readonly Type _Owner;

        public WeakActionStatic(MethodInfo method)
            : base(method)
        {
            if (!method.IsStatic)
            {
                throw new ArgumentException("static method expected", "method");
            }
            _Owner = method.DeclaringType;
        }

        public override bool HasBeenCollected
        {
            get { return false; }
        }

        protected override T GetMethod()
        {
            return System.Delegate.CreateDelegate(typeof(T), _Owner, _Method.Name) as T;
        }
    }

    private class WeakActionInstance : WeakAction
    {
        private readonly WeakReference _WeakRef;

        protected WeakActionInstance(MethodInfo method)
            : base(method)
        {
        }

        public WeakActionInstance(object instance, MethodInfo method)
            : this(method)
        {
            if (instance == null)
            {
                throw new ArgumentNullException("instance must not be null", "instance");
            }
            _WeakRef = new WeakReference(instance);
        }

        public override bool HasBeenCollected
        {
            get { return !_WeakRef.IsAlive; }
        }

        protected override T GetMethod()
        {
            if (HasBeenCollected) { return null; }

            object localTarget = _WeakRef.Target;
            if (localTarget == null) { return null; }

            return System.Delegate.CreateDelegate(typeof(T), localTarget, _Method.Name) as T;
        }
    }

    protected WeakAction(MethodInfo method)
    {
        _Method = method;
    }

    public static WeakAction Create(Expression expression)
    {
        return new WeakActionStatic(GetMethodInfo(expression));
    }

    public static WeakAction Create(object instance, Expression expression)
    {
        return new WeakActionInstance(instance, GetMethodInfo(expression));
    }

    protected abstract T GetMethod();

    public T Delegate
    {
        get { return GetMethod(); }
    }

    public abstract bool HasBeenCollected { get; }

    public bool IsInvokable
    {
        get { return !HasBeenCollected; }
    }

    private static MethodInfo GetMethodInfo(Expression expression)
    {
        LambdaExpression lambda = expression as LambdaExpression;
        if (lambda == null)
        {
            throw new ArgumentException("expression is not LambdaExpression");
        }

        MethodCallExpression outermostExpression = lambda.Body as MethodCallExpression;

        if (outermostExpression == null)
        {
            throw new ArgumentException("Invalid Expression. Expression should consist of a Method call only.");
        }

        return outermostExpression.Method;
    }
}

Seems slightly more OO style as it has less ifs related to what kind of WeakAction you actually have (static or instance).

That leads me to the point of questioning the purpose of static weak actions. Unless I'm missing something those are not really weak and you could just use an Action to store them for later execution.

Potential Bugs/Problems

The point of a weak action is to effectively weakly bind to an object so you can call a method on it if it is still alive if I'm not mistaken. Your examples show this basic usage:

if (weakAction.IsInvokable) weakAction.Delegate();

However between checking and actually obtaining the delegate the reference could have gone away and now you get a NullReferenceException. You could avoid it by doing this I guess:

var func = weakAction.Delegate;
if (func != null) func();

But somehow I have the feeling there could be a nicer interface which doesn't require the caller to have all that implicit knowledge. I'm just not sure how to effectively deal with argument passing while still retaining the static compiler type checking. Just wanted to throw this out there.

Style

-
I would consider renaming Delegate into Execute. While it's a property and not a function somehow weakAction.Execute() reads slightly nicer than weakAction.Delegate().

-
Usual C# naming convention for class members seems to be _camelCase or _PascalCase reserving camelCase for local variables. This way you can also get rid of the this. which mostly just clutters the code.

Code Snippets

public abstract class WeakAction<T> where T : class
{
    protected readonly MethodInfo _Method;

    private class WeakActionStatic : WeakAction<T>
    {
        private readonly Type _Owner;

        public WeakActionStatic(MethodInfo method)
            : base(method)
        {
            if (!method.IsStatic)
            {
                throw new ArgumentException("static method expected", "method");
            }
            _Owner = method.DeclaringType;
        }

        public override bool HasBeenCollected
        {
            get { return false; }
        }

        protected override T GetMethod()
        {
            return System.Delegate.CreateDelegate(typeof(T), _Owner, _Method.Name) as T;
        }
    }

    private class WeakActionInstance : WeakAction<T>
    {
        private readonly WeakReference _WeakRef;

        protected WeakActionInstance(MethodInfo method)
            : base(method)
        {
        }

        public WeakActionInstance(object instance, MethodInfo method)
            : this(method)
        {
            if (instance == null)
            {
                throw new ArgumentNullException("instance must not be null", "instance");
            }
            _WeakRef = new WeakReference(instance);
        }

        public override bool HasBeenCollected
        {
            get { return !_WeakRef.IsAlive; }
        }

        protected override T GetMethod()
        {
            if (HasBeenCollected) { return null; }

            object localTarget = _WeakRef.Target;
            if (localTarget == null) { return null; }

            return System.Delegate.CreateDelegate(typeof(T), localTarget, _Method.Name) as T;
        }
    }

    protected WeakAction(MethodInfo method)
    {
        _Method = method;
    }

    public static WeakAction<T> Create(Expression expression)
    {
        return new WeakActionStatic(GetMethodInfo(expression));
    }

    public static WeakAction<T> Create(object instance, Expression expression)
    {
        return new WeakActionInstance(instance, GetMethodInfo(expression));
    }

    protected abstract T GetMethod();

    public T Delegate
    {
        get { return GetMethod(); }
    }

    public abstract bool HasBeenCollected { get; }

    public bool IsInvokable
    {
        get { return !HasBeenCollected; }
    }

    private static MethodInfo GetMethodInfo(Expression expression)
    {
        LambdaExpression lambda = expression as LambdaExpression;
        if (lambda == null)
        {
            throw new ArgumentException("expression is not LambdaExpression");
        }

        MethodCallExpression outermostExpression = lambda.Body as MethodCallExpression;

        if (outermostExpression == null)
        {
            throw new ArgumentException("Invalid Expression. Expression should consist of a Method call only.");
        }

        return outermostExpression.Method;
    }
}

if (weakAction.IsInvokable) weakAction.Delegate();

var func = weakAction.Delegate;
if (func != null) func();

Context

StackExchange Code Review Q#8807, answer score: 2

Revisions (0)

No revisions yet.