5
\$\begingroup\$

The following code is intended to implement a LinFu DynamicProxy interceptor to lazy load given virtual properties from an Umbraco datastore.

My concerns:

  1. Thread safety, Have I covered all the bases, Am I storing variables correctly?
  2. Duplication - Getting the child type, am I safe to store a variable via my Lazy to use later?
  3. Efficiency, If I have everything else correct, am I being wasteful?

The code:

/// <summary>
/// The content interceptor for intercepting virtual content properties.
/// </summary>
internal class ContentInterceptor : IInvokeWrapper
{
    /// <summary>
    /// The reader writer lock slim.
    /// </summary>
    private static readonly ReaderWriterLockSlim ReaderWriterLockSlim 
                            = new ReaderWriterLockSlim();    

    /// <summary>
    /// The lazy content.
    /// </summary>
    private static readonly Lazy<IContent> LazyParent = new Lazy<IContent>(
        () =>
        {
            IContent content = contentService.GetById(id);
            IContent parent = content.Parent();
            return parent;
        });

    /// <summary>
    /// The lazy list content.
    /// </summary>
    private static readonly Lazy<IEnumerable<IContent>> LazyChildren = 
                            new Lazy<IEnumerable<IContent>>(
        () =>
        {
            IContent content = contentService.GetById(id);
            Type childType = returnType.GetGenericArguments().Single();

            AliasAttribute aliasAttribute = childType.FirstAttribute<AliasAttribute>();
            string alias = Conventions
                          .GetPropertyTypeAlias(aliasAttribute, childType.Name);

            IEnumerable<IContent> umbracoChildren = contentService
                                 .GetChildrenByName(content.Id, alias);
            return umbracoChildren;
        });

    /// <summary>
    /// The id.
    /// </summary>
    private static int id;

    /// <summary>
    /// The return type.
    /// </summary>
    private static Type returnType;

    /// <summary>
    /// The content service.
    /// </summary>
    private static IContentService contentService;

    /// <summary>
    /// The target to intercept.
    /// </summary>
    private readonly object target;

    /// <summary>
    /// Initializes a new instance of the <see cref="ContentInterceptor"/> class.
    /// </summary>
    /// <param name="target">
    /// The target to intercept.
    /// </param>
    /// <param name="id">
    /// The id.
    /// </param>
    public ContentInterceptor(object target, int id)
    {
        ContentInterceptor.id = id;
        this.target = target;
        contentService = GlobalServices.Current.ContentService;
    }

    /// <summary>
    /// The event that runs after invocation.
    /// </summary>
    /// <param name="info">
    /// The information.
    /// </param>
    /// <param name="returnValue">
    /// The return Value.
    /// </param>
    public void AfterInvoke(InvocationInfo info, object returnValue)
    {
        Debug.WriteLine("AfterInvoke() called");
    }

    /// <summary>
    /// The event that runs before invocation.
    /// </summary>
    /// <param name="info">The information.</param>
    public void BeforeInvoke(InvocationInfo info)
    {
        Debug.WriteLine("BeforeInvoke() called");
    }

    /// <summary>
    /// Performs the proxy invocation.
    /// </summary>
    /// <param name="info">The <see cref="InvocationInfo"/>.</param>
    /// <returns>The object to replace the original value.</returns>
    public object DoInvoke(InvocationInfo info)
    {
        lock (ReaderWriterLockSlim)
        {
            returnType = info.TargetMethod.ReturnType;

            // Skip by any ignored properties.
            IgnoreAttribute ignoreAttribute = returnType
                            .FirstAttribute<IgnoreAttribute>();

            if (ignoreAttribute != null)
            {
                // Call the original implementation.
                return info.TargetMethod.Invoke(this.target, info.Arguments);
            }

            // Check whether we are looking for children or not.
            if (returnType.IsCollectionType())
            {
                IEnumerable<IContent> children = LazyChildren.Value;

                if (children != null)
                {
                    // TODO: Would rather not be getting this twice.
                    Type childType = returnType.GetGenericArguments().Single();
                    MethodInfo getModelList = this.GetType()
                        .GetMethod("GetModelList")
                        .MakeGenericMethod(childType);

                    return getModelList.Invoke(this, new object[] { children });
                }
            }
            else
            {
                IContent parent = LazyParent.Value;
                MethodInfo getModel = this.GetType()
                                           .GetMethod("GetModel")
                                           .MakeGenericMethod(returnType);

                return getModel.Invoke(this, new object[] { parent });
            }

            return null;
        }
    }

    /// <summary>
    /// Gets a lazily invoked converted list of type from the content service.
    /// </summary>
    /// <param name="content">
    /// The list of children to convert from.
    /// </param>
    /// <typeparam name="T">
    /// The type to convert to.
    /// </typeparam>
    /// <returns>
    /// The <see cref="T:System.Lazy{List{T}}"/> representing the content.
    /// </returns>
    private T GetModel<T>(IContent content) where T : class
    {
        return content.ToType<T>();
    }

    /// <summary>
    /// Gets a list of the given type from an <see cref="IEnumerable{IContent}"/>.
    /// </summary>
    /// <param name="collection">
    /// The list of children to convert from.
    /// </param>
    /// <typeparam name="T">
    /// The type to convert to.
    /// </typeparam>
    /// <returns>
    /// The <see cref="T:System.Lazy{List{T}}"/> representing the content.
    /// </returns>
    // ReSharper disable once UnusedMember.Local
    private List<T> GetModelList<T>(IEnumerable<IContent> collection) where T : class
    {
        List<T> typedCollection = new List<T>();
        typedCollection.AddRange(collection.Select(this.GetModel<T>));

        return typedCollection;
    }
}
\$\endgroup\$
6
\$\begingroup\$

I'm not familiar with Umbraco, and I could not find any documentation on LinFu, I do, however, have some remarks on your code:

Comments - your code is more than 40% comments, and un-useful comments at that - a comment that tells me that id is /// The id is a waste of space and reduces readability of your code - I actually needed to copy your code locally and remove all the comments to read it.

Static vs. instance scopes - you chose to put some of the state in the instance, and some in the class, but you manage both from the class. This means that if you create two instances of your class, their state will be unexpected, as they will change each-other's state. If you this class is meant to be a Singleton, make it a Singleton.

Use locks properly - you chose to use ReaderWriterLockSlim as your lock object. That's OK, though I don't see why you would need a reader-writer lock.

The problem is that you misuse it - the way you use it is as if it was just a regular object. Again, a simple lock object is fine, but declaring it as a fancy lock type, you confuse the code reader to look for (and assume) its use.

\$\endgroup\$
  • 1
    \$\begingroup\$ Thanks! I comment code heavily for documentation generation, I should have stripped it out for brevity. As regards to the scoping, yeah, it's messed up. The class is an instance class but when you initialize a Lazy<T> the function is static so any non-instance properties can't be referenced. I didn't realise you could simply initialize it from the constructor. I have since moved everything to an instance scope. In all honesty I saw the ReaderWriterLockSlim class used in a similar fashion within the source for entity framework. I'll use lock. \$\endgroup\$ – James South Feb 23 '14 at 1:01
3
\$\begingroup\$
new Lazy<IContent>(
() =>
{
    IContent content = contentService.GetById(id);
    IContent parent = content.Parent();
    return parent;
});

Sometimes, extracting subexpressions into locals and giving them a name can improve the readability of code. But other times, it just makes the code longer for no good reason. I think this is the latter case, so I would rewrite this as:

new Lazy<IContent>(() => contentService.GetById(id).Parent());

Also, why is Parent() a method? It sounds like it should be a property. If you want to keep it as a method, I think a better name would be something like GetParent().


List<T> typedCollection = new List<T>();
typedCollection.AddRange(collection.Select(this.GetModel<T>));

return typedCollection;

Again, unnecessarily verbose, this could be just:

return collection.Select(this.GetModel<T>).ToList();
\$\endgroup\$
  • \$\begingroup\$ Thanks for the input! You're right on the code. I have an overly verbose style at times which can be a bad habit. Nice use of a method group in the last example :) I've updated my source to match. Also you're right, Parent() is icky. It's not mine though, it come from the Umbraco codebase. \$\endgroup\$ – James South Feb 23 '14 at 1:05
  • 1
    \$\begingroup\$ “Nice use of a method group in the last example” I copied that directly from your code :-) \$\endgroup\$ – svick Feb 23 '14 at 3:32
  • \$\begingroup\$ I must code in a trance half the time! :) \$\endgroup\$ – James South Feb 23 '14 at 11:22

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.