WeakAction Implementation

Question

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<T> 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 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;
        }

        private T GetMethod()
        {
            if (this.ownerType != null)
            {
                return System.Delegate.CreateDelegate(typeof(T), this.ownerType, this.methodName) as T;
            }

            if (this.weakRef != null && this.weakRef.IsAlive)
            {
                object localTarget = this.weakRef.Target;
                if (localTarget != null)
                {
                    return System.Delegate.CreateDelegate(typeof(T), localTarget, this.methodName) as T;
                }
            }

            return null;
        }
    }
}

using System;
using System.Linq.Expressions;

namespace WeakAction
{
    public class Program
    {
        public static void Main()
        {
            var testClass = new TestClass();

            // instance method
            Expression<Func<int>> instanceExpression = () => testClass.Stuff();
            var weakStuff = new WeakAction<Func<int>>(testClass, instanceExpression);

            if (!weakStuff.HasBeenCollected)
            {
                var result = weakStuff.Delegate();
            }

            // static method
            Expression<Func<int>> staticExpression = () => TestClass.MoreStuff();
            var staticWeakRef = new WeakAction<Func<int>>(null, staticExpression);
            if (!staticWeakRef.HasBeenCollected)
            {
                var result = staticWeakRef.Delegate();
            }

            // instance method taking an arg
            Expression<Action<string>> argExpression = (str) => testClass.TakesAnArg(str);
            var instanceWithArg = new WeakAction<Action<string>>(testClass, argExpression);
            if (instanceWithArg.IsInvokable)
            {
                instanceWithArg.Delegate("hello bert!");
            }

            Console.Read();
        }

        public class TestClass
        {
            public int Stuff()
            {
                Console.WriteLine("Stuff invoked!");
                return 1;
            }

            public void TakesAnArg(string msg)
            {
                Console.WriteLine(msg);
            }

            public static int MoreStuff()
            {
                Console.WriteLine("More stuff invoked!");
                return 1;
            }
        }
    }
}

Answer 1

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<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;
    }
}

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<T> 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

1. 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().

2. 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.