ICollection property and encapsulation

Question

Is it bad practice to use interfaces for my properties considering I want to encapsulate them?

public class Order
{
    private readonly ICollection<OrderItem> _orderItems;
    public IReadOnlyCollection OrderItems => _orderItems; // not possible without ToList
}

There's no easy way to expose it as there's no way to convert to IReadOnlyCollection or IEnumerable from ICollection without using ToList first which involves copying entire collection.

Should I define it as:

private readonly Collection<OrderItem> _orderItems;

or

private readonly HashSet<OrderItem> _orderItems;

or

private readonly List<OrderItem> _orderItems;

instead?

Always use the lowest possible denominator. i.e. for parameters in methods or constructors use `IEnmuerable` unless you require something from more specialized interfaces, i.e. `.Count` or need `Add/Remove` methods etc. As return time, the lowest common and depending on your intention. Do you want people to change the result? use `IList` or `ICollection` Don't want user to manipulate it? `IEnumerable` and it may be iterable (i.e. not executed immediately). Or return `T[]` if it should be more explicit that its an array and not any general enumerable/iterator — Tseng, Jun 14 '18 at 14:01
@Tseng yeah the question here was more about using `ICollection` in private fields in order to later expose it as `IReadOnlyCollection`. Other than that I usually do as you say. — Konrad, Jun 14 '18 at 14:16

score 1 · Accepted Answer · answered Jun 14 '18 at 11:50

The rule you should follow is that private members are only visible to the class where they are defined so you can define the type as List<T> or Hashset<T> or Collection<T>, other developpers that use your class's assembly will not be impacted by a breaking changes if you modify the type later.

You can choose one of the three alternatives you put in your question as long as long you know the difference between Hasset<T> vs List<T> and List<T> vs Collection<T>.

So remove the interface ICollection<OrderItem> as a type of your private field and use a concrete class if you don't want to call ToList() in OrderItems implementation.

score 0 · Answer 2 · answered Jun 14 '18 at 11:58

0

You can write your own extension to convert ICollection to IReadOnlyCollection

public static class CollectionExtensions
{
    public static IReadOnlyCollection<T> AsReadOnly<T>(this ICollection<T> source)
    {
        if (source == null)
        {
            throw new ArgumentNullException("source");
        }
        return source as IReadOnlyCollection<T> ?? new ReadOnlyCollectionAdapter<T>(source);
    }
    sealed class ReadOnlyCollectionAdapter<T> : IReadOnlyCollection<T>
    {
        ICollection<T> source;
        public ReadOnlyCollectionAdapter(ICollection<T> source) { this.source = source; }
        public int Count { get { return source.Count; } }
        public IEnumerator<T> GetEnumerator() { return source.GetEnumerator(); }
        IEnumerator IEnumerable.GetEnumerator() { return GetEnumerator(); }
    }
}

answered Jun 14 '18 at 11:58

Bagdan Gilevich

1,231
1
11
17

ToList (that return an IReadOnlyCollection) is already an extension method. So I don't know why you need another extension method? – CodeNotFound Jun 14 '18 at 12:01
I think it's too hackish and it's a code smell to do such thing. – Konrad Jun 14 '18 at 12:06
@CodeNotFound, ToList involves copying entire collection and it solves more generic task, while this extension is focused only on providing IReadOnlyCollection – Bagdan Gilevich Jun 14 '18 at 12:12

ICollection property and encapsulation

2 Answers2