4

I have read this question: IEnumerable vs IReadonlyCollection vs ReadonlyCollection for exposing a list member and this question: ReadOnlyCollection or IEnumerable for exposing member collections?. I am concious that these answers were written years ago.

The answer by Jon Skeet suggests that I should be using a IReadOnlyCollection rather than an IEnumerable as I am very cautious to prevent code like this:

ICollection<Order> evil = (ICollection<Order>)customer.Orders;
evil.Add(order3);

Now see the code below:

public class Customer
{
    private readonly List<Order> _orders = new List<Order>();

    public string FirstName { get; set; }
    public string LastName { get; set; }
    public IEnumerable<Order> Orders
    {
        get { foreach (var order in _orders) yield return order; }
    }

    internal void AddOrder(Order order)
    {
        _orders.Add(order);
    }

    public void AddOrders(IEnumerable<Order> orders)
    {
        this._orders.AddRange(orders);
    }
}

I can use the code like this:

Order order1 = new Order();
Order order2 = new Order();
Order order3 = new Order();

Customer customer = new Customer();
customer.FirstName = "Fred";
customer.LastName = "Bloggs";
customer.AddOrder(order1);
customer.AddOrder(order2);

and like this:

Customer customer2 = new Customer();
customer2.FirstName = "Average";
customer2.LastName = "Joe";

List<Order> orders = new List<Order>();
Order order4 = new Order();
Order order5 = new Order();
orders.Add(order4);
orders.Add(order5);
customer2.AddOrders(orders);

Are there any risks using this code? I am trying to make it as water tight as possible as it is part of a public api. Is this more water tight than an IReadOnlyCollection? Is it less efficient? Please note that this is the final version of the code so there are no other scenarios to consider.

The concerns I have are:

  1. Exposing an IEnumerable instead of a readonly list.

  2. Using a List to hold the orders privately.

I am trying to follow the advice described here as best I can: https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/guidelines-for-collections.

3
  • 2
    IMHO anyone "evil" enough could also just use reflection to get the internal list.
    – Dirk
    Apr 16, 2018 at 12:39
  • @Dirk, thanks. I guess there is nothing I can do to stop that?
    – w0051977
    Apr 16, 2018 at 13:26
  • Nope @w0051977. Apr 16, 2018 at 13:27

2 Answers 2

2

Is this more water tight than an IReadOnlyCollection?

I don't think so. I would say they serve the same purpose in another way.

Are there any risks using this code? Is it less efficient?

Risks? Not that I am aware of. Since the yield return compiles to an internal state machine, you lose the direct exposure of the list. That is a benefit of your approach, just like with a read-only collection.

Your code does have a performance impact though: getting the count and other relevant properties have a complexity of O(n) instead of O(1). This might hurt your performance.

3
  • Could you clarify what you mean by 0(n) and 0(1)? I guess you would choose an IReadOnlyCollection in my specific case as Evk did? Thanks.
    – w0051977
    Apr 16, 2018 at 13:19
  • Yes, as I said, use IReadOnlyCollection. And O(n) is called the Big O notation. Read more. Apr 16, 2018 at 13:22
  • for Big O notation. +1
    – w0051977
    Apr 16, 2018 at 13:46
1

I'd say just do:

public IReadOnlyList<Order> Orders => _orders.AsReadOnly();

It wraps your List into read-only wrapper, so "evil" code like this:

ICollection<Order> evil = (ICollection<Order>)customer.Orders;
evil.Add(order3);

Will just throw an exception that colleciton is read-only. Using IEnumerable has both performance and usability implications for this scenario, without providing much benefits (at all I'd say).

For example I guess it's a common operation to get a number of customer orders. If you return IEnumerable - that operation is both less perfomant compared to IReadOnlyList and less convenient for the caller.

Less perfomant is because with IEnumerable the only way to get count of orders is Count() LINQ method, and this method, in your implementation with yield return, will have to go through the whole collection and count how much items are there. With IReadOnlyList there is Count property which just contains that information.

Less convenient, because multiple enumerations of single enumerable is discouraged and VS will warn user of your api about that. Suppose user of your api has code like this:

IEnumerable<Order> orders = customer2.Orders;
// just an example, there is actually no need to check for count
// before enumerating
if (orders.Count() > 0) {
    foreach (var order in orders) {

    }
}

On foreach line, VS will warn him about "Possible multiple enumerations of IEnumerable". Only reasonable thing caller can do with IEnumerable is, well, enumerate it. Once. So to avoid this warning, he will have to write some ugly code, or do var orders = customer.Orders.ToList(), creating unnecessary copy of your list and working with that.

There is no reason to limit user of your api like that, unless enumerating just once is the only correct way of using that api.

10
  • @w0051977 _orders is still List<T> like you have now, so you use AddRange as usual. Just instead of IEnumerable<Order> you now return IReadOnlyCollection<Order> as shown in answer.
    – Evk
    Apr 16, 2018 at 12:54
  • Could you also confirm what the "performance and usability implications " you are referring to are? Thanks.
    – w0051977
    Apr 16, 2018 at 13:29
  • @w0051977 An IEnumerable<T> has no information whatsoever of how many items it might contain, this is one of the compromises of its laziness. In order to know how many items your enumeration has you need to enumerate it; in other words, you need to count the items contained within, and unless you cache the result, you'll need to count the items every time. An IList doesn't have this problem because it always knows how many items it contains.
    – InBetween
    Apr 16, 2018 at 13:35
  • @w0051977 I've expanded a bit on that.
    – Evk
    Apr 16, 2018 at 13:42
  • One more thing - would I pass an IEnumerable to AddOrders (as I have done in the question)? Thanks.
    – w0051977
    Apr 16, 2018 at 14:09

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.