Question

Let's say I have IEnumerable<int> property backed with List<int> field, so I can modify the collection from within the class, but it's publicly exposed as read-only.

public class Foo
{
    private List<int> _bar = new List<int>();

    public IEnumerable<int> Bar
    {
        get { return _bar; }
    }
}

But with code like that you can easily cast object retrieved from the property back to List<int> and modify it:

var foo = new Foo();
var bar = (List<int>)foo.Bar;
bar.Add(10);

Question is: what is the best (best readable, easiest to write, without performance loss) way to avoid that?

I can come up with at least 4 solutions, but non of them is perfect:

  1. foreach and yield return:

    public IEnumerable<int> Bar
    {
        get
        {
            foreach (var item in _bar)
                yield return item;
        }
    }
    

    - really annoying to write and to read.

  2. AsReadOnly():

    public IEnumerable<int> Bar
    {
        get { return _bar.AsReadOnly(); }
    }
    

    + will cause exception when someone tries to modify the returned collection
    + does not create a copy of the entire collection.

  3. ToList()

    public IEnumerable<int> Bar
    {
        get { return _bar.ToList(); }
    }
    

    + User can still modify retrieved collection, but it's not the same collection we are modifying from within the class, so we shouldn't care.
    - creates a copy of entire collection what may cause problems when collection is big.

  4. Custom wrapper class.

    public static class MyExtensions
    {
        private class MyEnumerable<T> : IEnumerable<T>
        {
            private ICollection<T> _source;
            public MyEnumerable(ICollection<T> source)
            {
                _source = source;
            }
    
            public IEnumerator<T> GetEnumerator()
            {
                return _source.GetEnumerator();
            }
    
            IEnumerator IEnumerable.GetEnumerator()
            {
                return ((IEnumerable)_source).GetEnumerator();
            }
        }
    
        public static IEnumerable<T> AsMyEnumerable<T>(this ICollection<T> source)
        {
            return new MyEnumerable<T>(source);
        }
    }
    

    usage:

    public IEnumerable<int> Bar
    {
        get
        {
            return _bar.AsMyEnumerable();
        }
    }
    

    + don't need to clone the collection
    - when you use it as LINQ queries source some methods won't use ICollection.Count, because you don't expose it.


Is there any better way to do that?

Was it helpful?

Solution

Question is: what is the best (best readable, easiest to write, without performance loss) way to avoid that?

In general, I don't try to avoid it. The consumer of my API should use the type I expose, and if they don't, any bugs resulting are their fault, not mine. As such, I don't really care if they cast the data that way - when I change my internal representation, and they get cast exceptions, that's their issue.

That being said, if there is a security concern, I would likely just use AsReadOnly. This is effectively self-documenting, and has no real downsides (apart from a small allocation for the wrapper, as there is no copy of the data, you do get meaningful exceptions on modification, etc). There is no real disadvantage to this vs. making your own custom wrapper, and a custom wrapper means more code to test and maintain.

In general, I personally try to avoid copying without reason. That would eliminate ToList() as an option in general. Using an iterator (your first option) is not as bad, though it does not really provide many advantages over a ReadOnlyCollection<T>.

OTHER TIPS

In general I'm in the don't bother camp. If I give you an IEnumerable and you cast it to something else you are the one who broke the contract and its not my fault if something breaks. If I found myself in a position where I really needed to protect a client from corrupting my state I would create an extension method like:

static IEnumerable<T> AsSafeEnumerable<T>(this IEnumerable<T> obj)
{
    foreach (var item in obj)
    {
        yield return item;
    }
}

and never worry about it again.

I think you've sufficiently outlined many of the solutions but one thing I didn't see you take into account was allocation overhead (yet performance was listed as a concern). Allocations matter and having the property allocate a new object on every call for the same list is wasteful. Instead I would prefer an object with lifetime equal to the original list that could be returned without the extra overhead. Essentially a modified version of #4

I agree with Reed Copsey post, if the consumer wishes to cast and manage the data in a way that will corrupt the class data then let them.

You can spend alot of time trying to "stop" a particular action to happen just to have some clever Reflection spoil all your hard word.

Now to answer the question, if you had to do this. The first option seems the most relevant to me, however I would not enumerate through the list when calling the property. I would just return the inner collection such as.

public IEnumerable<int> Bar
{
    get { return _bar; }
}
Licensed under: CC-BY-SA with attribution
Not affiliated with StackOverflow
scroll top