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

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 2

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 3

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.