C# Auto Property - Is this 'pattern' best practice?

Question

I seem to be using this sort of pattern in my code a lot , I know that it is not a simple Autoproperty any more as that would be:

  public IList<BCSFilter> BCSFilters { get; set; }

The code I have been using is this:

    private IList<BCSFilter> _BCSFilters;

    /// <summary>
    /// Gets or sets the BCS filters.
    /// </summary>
    /// <value>The BCS filters.</value>
    public IList<BCSFilter> BCSFilters
    {
        get
        {
            if (_BCSFilters == null)
            {
                _BCSFilters = new List<BCSFilter>();
            }

            return _BCSFilters;
        }
        set
        {
            _BCSFilters = value;
        }
    }

This is so I can just do MainClass.BCSFilters and not worry about needing to instantiate the List in the consuming code. Is this a 'normal' pattern \ the correct way to do this?

I couldn't find a duplicate question

Answer 1

This is a technique that I use a lot myself. This can also help save memory resources since it doesn't instantiate the List<> object unless the objects property is actually being used within the consuming code. This uses a "Lazy Loading" technique.

Also, the "Lazy Loading" technique that you listed isn't Thread Safe. If there happens to be multiple calls simultaneously to the property you could end up having multiple calls setting the property to a new List<> object, consequentially overwriting any existing List values with a new, empty List<> object. To make the Get accessor Thread Safe you need to use the Lock statement, like so:

private IList<BCSFilter> _BCSFilters;

// Create out "key" to use for locking
private object _BCSFiltersLOCK = new Object();

/// <summary>
/// Gets or sets the BCS filters.
/// </summary>
/// <value>The BCS filters.</value>
public IList<BCSFilter> BCSFilters
{
    get
    {
        if (_BCSFilters == null)
        {
            // Lock the object before modifying it, so other
            // simultaneous calls don't step on each other
            lock(_BCSFiltersLOCK)
            {
                if (_BCSFilters == null)
                }
                    _BCSFilters = new List<BCSFilter>();
                }
            }
        }

        return _BCSFilters;
    }
    set
    {
        _BCSFilters = value;
    }
}

However, if you'll always need the List<> object instantiated it's a little simpler to just create it within the object constructor and use the automatic property instead. Like the following:

public class MyObject
{
    public MyObject()
    {
        BCSFilters = new List<BCSFilter>();
    }

    public IList<BCSFilter> BCSFilters { get; set; }
}

Additionally, if you leave the "set" accessor public then the consuming code will be able to set the property to Null which can break other consuming code. So, a good technique to keep the consuming code from being able to set the property value to Null is to set the set accessor to be private. Like this:

public IList<BCSFilter> BCSFilters { get; private set; }

A related technique is to return an IEnumerable<> object from the property instead. This will allow you to replace the List<> type internally within the object at any time and the consuming code will not be affected. To return IEnumerable<> you can just return the plain List<> object directly since it implements the IEnumerable<> interface. Like the following:

public class MyObject
{
    public MyObject()
    {
        BCSFilters = new List<BCSFilter>();
    }

    public IEnumerable<BCSFilter> BCSFilters { get; set; }
}

Answer 2

Your pattern is a totally reasonable lazy-load pattern, but do be aware that it is not thread safe.

If two threads accessed this property for the first time very close together, your null check pattern would not prevent the condition where one thread evaluates to null but before it gets a chance to initialise the list, the second thread also evaluates to null. In this situation they will both initialise the list.

Furthermore, there is a chance that by the time the property returns, one thread will have one copy of the list, while the second has another.

Not an issue in a single threaded environment, but definitely something to be aware of.

Answer 3

It's the correct pattern as long as what you want is:

• Allow external code to replace the entire list ( instance.BCSFilters = null )
• Have the list magically created on read. It's tricky though as is, since you're allowing the user to set it to null (in the set) but you're not allowing it to remain null (since a later get will yield an empty list).

You can also want to expose the IList in readonly mode (with lazy init if you want), so users can only Add or Remove items in it without being able to overwrite the list itself. If you have a lot of assignements, there may be copying involved though.

Usually I only have a getter to my IList members, and I may even expose IEnumerable or return a copy in the get (but you'd have to provid specific Add and Remove methods, so YMMV)

Answer 4

FYI, a somewhat more concise way of doing the exact same thing you're already doing might look like this:

private IList<BCSFilter> _BCSFilters;

public IList<BCSFilter> BCSFilters
{
    get
    {
        return _BCSFilters ?? (_BCSFilters = new List<BCSFilter>());
    }
    set
    {
        _BCSFilters = value;
    }
}

Answer 5

Your approach is the lazy init version of

public class xyz
{
    public xyz()
    {
        BCSFilters = new List<BCSFilter>();
    }

    public IList<BCSFilter> BCSFilters { get; set; }
}

Answer 6

there's another trick :)

To use Lazy from .Net 4.

i've seen this at Mark Seemann blog i think:

 public class Order
{
    public Order()
    {
        _customerInitializer = new Lazy<Customer>(() => new Customer());
    }

    // other properties

    private Lazy<Customer> _customerInitializer;
    public Customer Customer
    {
        get
        {
            return _customerInitializer.Value;
        }
    }

    public string PrintLabel()
    {
        string result = Customer.CompanyName; // ok to access Customer
        return result + "\n" + _customerInitializer.Value.Address; // ok to access via .Value
    }
}

notice that "_customerInitializer" could never be null so it's very same to use it. and it could be thread safe too! the constructor can get an overload with the LazyThreadSafetyMode Enum! http://msdn.microsoft.com/en-us/library/system.threading.lazythreadsafetymode.aspx

Answer 7

This is an example of the Lazy Load pattern. It's an accepted pattern and perfectly valid.

You could use automatic properties in C# and assign an instance to the property in a constructor. The benefit of the Lazy Load pattern is that you don't initialize the property unless it's called. This can be useful in situations where the initialization is expensive.

I tend to prefer automatic properties with constructor initialization because the syntax is more concise and there's less typing unless initialization is expensive, in which case Lazy Load works well.

Answer 8

We use that pattern at my workplace. It's handy because you avoid possible null reference exceptions in the consuming code, and keeps the consuming code simpler.

Answer 9

Yeah, that's perfectly normal ;-)

Such lazy creation is not uncommon, and makes good sense. The only caveat is you'll have to be careful if you reference the field at all.

Edit: But I'll have to go with Chris and the others: it's a (much) better pattern to use an auto property and initialize the collection in the constructor.

Answer 10

It's an alright pattern. Autoproperties is just shorthand for the very most simple, and arguably most common scenario with properties, and for some scenarios, it simply isn't sensible to use it. What you could do, tho, is to instantiate BCSFilters in the constructor instead. That way you could use autoproperties and you still wouldn't have to worry about null reference exceptions.