views:

1168

answers:

3

I'd like to get some feedback on a class I wrote for encapsulating the lazy-load pattern. Usually, when I do lazy-loading, it looks something like this:

private SomeClass _someProperty;
public SomeClass SomeProperty
{
    get
    {
        if(this._someProperty == null)
        {
            // run code to populate/instantiate _someProperty
        }
        return this._someProperty;
    }
}

The way the class works is pretty basic - it accepts a generic type parameter and one or more method delegates using Func<T>. If more than one delegate is specified, the first one to return a non-default value is used (this particular feature may not be useful to everyone, but it was necessary in the domain I was using it).

Here is a brief excerpt that I believe shows pretty much what the class does:

/// <summary>
/// A container for an object of type <typeparamref name="T"/> whose value is not known until the <see cref="Value" /> property is accessed.
/// </summary>
/// <typeparam name="T">The type of the underlying object.</typeparam>
public class Lazy<T>
{
    /// <summary>
    /// A value representing whether the delegates have been invoked.
    /// </summary>
    private Boolean _wereValueDelegatesInvoked;

    /// <summary>
    /// Initializes a new instance of the <see cref="Lazy{T}" /> class with the specified <paramref name="valueDelegates"/>.
    /// </summary>
    /// <param name="valueDelegates">A list of delegates that can potentially return the value of the underlying object.</param>
    public Lazy(params Func<T>[] valueDelegates)
    {
        this.ValueDelegates = valueDelegates;
    }

    /// <summary>
    /// Gets the delegate that returns the value of the underlying object.
    /// </summary>
    public IEnumerable<Func<T>> ValueDelegates { get; protected set; }

    private T _value;
    /// <summary>
    /// Gets the value of the underlying object. If multiple delegates are specified, the first delegate to return a non-default value will be used.
    /// </summary>
    public T Value
    {
        get
        {
            if (!this._wereValueDelegatesInvoked)
            {
                this._value = this.GetValue();
            }
            return this._value;
        }
    }

    /// <summary>
    /// Return the value of the underlying object. If multiple delegates are specified, the first delegate to return a non-default value will be used.
    /// </summary>
    /// <returns>The value of the underlying object.</returns>
    protected T GetValue()
    {
        T value = default(T);

        if (this.ValueDelegates != null)
        {
            foreach (Func<T> valueDelegate in this.ValueDelegates)
            {
                value = valueDelegate.Invoke();
                if (!Lazy<T>.IsNullOrDefault(value)) break;
            }
        }

        this._wereValueDelegatesInvoked = true;
        return value;
    }
}

...and for those for whom this will not evoke a response of "tl;dr", here is the class in its entirety:

/// <summary>
/// A container for an object of type <typeparamref name="T"/> whose value is not known until the <see cref="Value" /> property is accessed.
/// </summary>
/// <typeparam name="T">The type of the underlying object.</typeparam>
public class Lazy<T>
{
    /// <summary>
    /// A value representing whether the delegates have been invoked.
    /// </summary>
    private Boolean _wereValueDelegatesInvoked;

    /// <summary>
    /// Initializes a new instance of the <see cref="Lazy{T}" /> class with the specified <paramref name="valueDelegate"/>.
    /// </summary>
    /// <param name="valueDelegate">A delegate that returns the value of the underlying object.</param>
    public Lazy(Func<T> valueDelegate)
    {
        this.ValueDelegates = new Func<T>[] { valueDelegate };
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="Lazy{T}" /> class with the specified <paramref name="valueDelegates"/>.
    /// </summary>
    /// <param name="valueDelegates">A list of delegates that can potentially return the value of the underlying object.</param>
    public Lazy(IEnumerable<Func<T>> valueDelegates)
    {
        this.ValueDelegates = valueDelegates;
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="Lazy{T}" /> class with the specified <paramref name="valueDelegates"/>.
    /// </summary>
    /// <param name="valueDelegates">A list of delegates that can potentially return the value of the underlying object.</param>
    public Lazy(params Func<T>[] valueDelegates)
    {
        this.ValueDelegates = valueDelegates;
    }

    public Lazy(T value)
    {
        this._value = value;
        this._wereValueDelegatesInvoked = true;
    }

    /// <summary>
    /// Gets the delegate that returns the value of the underlying object.
    /// </summary>
    public IEnumerable<Func<T>> ValueDelegates { get; protected set; }

    private T _value;
    /// <summary>
    /// Gets the value of the underlying object. If multiple delegates are specified, the first delegate to return a non-default value will be used.
    /// </summary>
    public T Value
    {
        get
        {
            if (!this._wereValueDelegatesInvoked)
            {
                this._value = this.GetValue();
            }
            return this._value;
        }
    }

    /// <summary>
    /// Return the value of the underlying object. If multiple delegates are specified, the first delegate to return a non-default value will be used.
    /// </summary>
    /// <returns>The value of the underlying object.</returns>
    protected T GetValue()
    {
        T value = default(T);

        if (this.ValueDelegates != null)
        {
            foreach (Func<T> valueDelegate in this.ValueDelegates)
            {
                value = valueDelegate.Invoke();
                if (!Lazy<T>.IsNullOrDefault(value)) break;
            }
        }

        this._wereValueDelegatesInvoked = true;
        return value;
    }

    private static Boolean IsNullOrDefault(T value)
    {
        if (value == null) return true;
        else if (value.Equals(default(T))) return true;
        else return false;
    }

    public override Boolean Equals(Object obj)
    {
        if (this.Value == null) return (obj == null);
        return this.Value.Equals(obj);
    }

    public override Int32 GetHashCode()
    {
        if (this.Value == null) return base.GetHashCode();
        else return this.Value.GetHashCode();
    }

    /// <summary>
    /// Returns the value of the <see cref="Lazy{T}" />.
    /// </summary>
    /// <param name="value">A <see cref="Lazy{T}" /> object.</param>
    /// <returns>The value of the <see cref="Value" /> property for the <paramref name="value" /> parameter.</returns>
    public static implicit operator T(Lazy<T> value)
    {
        return value.Value;
    }

    /// <summary>
    /// Returns a <see cref="String" /> that represents the current <see cref="Lazy{T}" />.
    /// </summary>
    /// <returns>A <see cref="String" /> that represents the current <see cref="Lazy{T}" />.</returns>
    public override String ToString()
    {
        if (Lazy<T>.IsNullOrDefault(this.Value)) return null;
        else return this.Value.ToString();
    }
}

The class is used like this:

    [TestMethod]
    public void ABadFakeWorkExample()
    {
        var myLazyObject = new Lazy<Object>((() => { Thread.Sleep(5000); return new Object(); }));

        Assert.IsNotNull(myLazyObject);
    }

And an example using multiple delegates:

[TestMethod]
public void LazyUsesFirstNonDefaultResult()
{
    Lazy<DateTime> lazy = new Lazy<DateTime>(
        (() => { return default(DateTime); }),
        (() => { return DateTime.Now; }),
        (() => { return default(DateTime); }));

    Assert.AreNotEqual(default(DateTime), lazy.Value, "D'OH!");
}

So here's what I'd like to know:

  • Is this overkill? I mean it's easy enough to create a snippet for the first pattern I posted, right? And if it is, is it so much so that this class shouldn't be used?
  • I ran some preliminary tests in ANTS, and this seems to be fairly lightweight, but are there any performance issues I should be worried about?
  • Anything that could be added?
  • Anything that should be taken away?

Note unrelated to the actual question: Based on feedback I got from this question, I'm leaving the wiki box unticked... Please comment if you really think this should be wikified.

+6  A: 

The test method is a bit silly because obviously the myLazyObject reference won't be null - you've just called "new". You're not actually testing any conversions.

I don't think the conversion operator, hash code, equality checking or IsNullOrDefault are needed. The logic of GetValue() is needed, but I wouldn't have a Value property and a GetValue method. Just have the property, and put the logic in there.

I wouldn't expose the delegates - particularly not as a raw array, which means that the outside world can mutate the array. In fact, I wouldn't take an array in the first place - or at least not store it as an array. I'd store it as a single delegate - they're multicast, after all. That will give you the return value of the last delegate in the chain rather than the first one, but I'd hope that wouldn't be a problem in real use anyway. (I hope it's pretty odd to actually have more than one delegate specified.)

I would make the class sealed, possibly implementing an interface (e.g. IProvider<T>) which just has the Value property. That way for other clients who just want to provide a value, you could have FixedProvider<T> which just took an instance of T in the constructor.

Think about whether you need this to be thread-safe or not, too.

Does that lot help?

EDIT: One more thing: make the delegate field readonly. You can't do that while it's an automatic property, but it's easy once you don't expose it :) I'd also group all the fields/autoproperties together at the top of the class, so it's easy to see what the state involved is.

Jon Skeet
Yes, great comments, thanks! About the interface though - there is a constructor that takes a T object which effectively short-circuits the GetValue() method. Is this sufficient, or do you think there should still be a separate, even more lightweight type to back it up?
Daniel Schaffer
Ah, I hadn't seen that. I'd probably use a different type, just because at that point it's really *not* a lazy implementation.
Jon Skeet
+1  A: 

A couple of points:

  • I don't know that I would allow multiple delegates - if there were a case when you might need more than one load function, you could wrap that in a single func!

  • Might be worth just exposing the value itself rather than implementing the hashcode/equality checking etc.

Edit: Just thought of one consideration - how easy/hard is it going to make testing the containing class?

Jennifer
I added an example test using multiple delegates, though I'm not sure I completely understand the question.
Daniel Schaffer
I mean if you are using the your lazy loader for the properties of class A, and you want to unit test class A. Not sure whether it would help or hinder (you could always hook up a fake lazy loader to inject in data or something).
Jennifer
Well I would think that class A would be tested separately from anything else, correct?
Daniel Schaffer
+2  A: 

This looks very similar to the LazyInit<> structure in the Parallel Extensions package that is going to be included in .NET 4.0. The main differences are:

  • Microsoft's implementation doesn't allow multiple initialization delegates.
  • They use Interlocked.CompareExchange and some other tricks to ensure thread-safety.
  • LazyInit is a structure, rather than a class.

The last point bears expanding upon a bit. While making LazyInit a structure rather than a class can lead to bugs if you're not careful (don't store LazyInit in a readonly variable) there's a pretty big potential performance benefit from doing it this way. In the MS implementation, if you don't supply an initialization delegate, it will attempt to create the given class by calling a zero-argument constructor on the class. Because LazyInit itself is a structure, it doesn't need to be instantiated.

private LazyInit<Foo> fooHolder;

If some code calls fooHolder.Value, a single instance of the Foo class will be created. However, in this example if fooHolder.Value is never called, fooHolder itself is never instantiated - no code is run, and no memory is allocated, as I understand it. In cases where you have many LazyInits which might not all be used, this can be a significant savings.

Joel Mueller