Is an IEnumerable now more water tight than an IReadOnlyList?

Question

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.

Answer 1

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.

Answer 2

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.