26

I'm trying to get the idea, what would be the best way to publish a Readonly List of objects as a public method? From Eric Lippert's Blog, Arrays are kinda bad, because someone could easily add a new Entry. So one would have to pass a new Array every time the method is called. He suggests, to pass IEnumerable<T>, since this is per definition read only (no add, remove methods), which I practiced for quite sometime. But in our new project, people even started to create Arrays of these IEnumerables, because they don't know the DataSource behind, so they get a : Handling warning for possible multiple enumeration of IEnumerable

I'm interested in a technical approach, how one would solve this puzzle. The only solution I came up so far would be to use a IReadOnlyCollection, but this would be way more explicit than an IEnumerable.

What is best practice to publish such lists, which shouldn't be changed, but should be declared as In-Memory Lists?

Community
  • 1
  • 1
Matthias Müller
  • 3,336
  • 3
  • 33
  • 65
  • 3
    List<> already implements IReadOnlyList<>, you might as well use it. That doesn't make the list immutable, but the "don't do it!" ought the be pretty clear to team members. – Hans Passant Nov 22 '15 at 20:18
  • Yeah, in my case it my be sufficent, but I was wondering as well, what if I pass such a List via API. There should be no way someone can edit the List and mess with my internal Code? – Matthias Müller Nov 23 '15 at 16:54

5 Answers5

15

Usually - and since a while - this solved using immutable collections.

Your public properties should be, for example, of type IImmutableList<T>, IImmutableHashSet<T> and so on.

Any IEnumerable<T> can be converted to an immutable collection:

  • someEnumerable.ToImmutableList();
  • someEnumerable.ToImmutableHashSet();
  • ... and so on.

This way you can work with private properties using mutable collections and provide a public surface of immutable collections only.

For example:

public class A
{
     private List<string> StringListInternal { get; set; } = new List<string>();
     public IImmutableList<string> StringList => StringListInternal.ToImmutableList();
}

There's also an alternate approach using interfaces:

public interface IReadOnlyA
{
     IImmutableList<string> StringList { get; }
}

public class A : IReadOnlyA
{
     public List<string> StringList { get; set; } = new List<string>();
     IImmutableList<string> IReadOnlyA.StringList => StringList.ToImmutableList();
}

Check that IReadOnlyA has been explicitly-implemented, thus both mutable and immutable StringList properties can co-exist as part of the same class.

When you want to expose an immutable A, then you return your A objects upcasted to IReadOnlyA and upper layers won't be able to mutate the whole StringList in the sample above:

public IReadOnlyA DoStuff()
{
     return new A();
}

IReadOnlyA a = DoStuff();

// OK! IReadOnly.StringList is IImmutableList<string>
IImmutableList<string> stringList = a.StringList;

Avoiding converting the mutable list to immutable list every time

It should be a possible solution to avoid converting the source list into immutable list each time immutable one is accessed.

Equatable members

If type of items overrides Object.Equals and GetHashCode, and optionally implements IEquatable<T>, then both public immutable list property access may look as follows:

public class A : IReadOnlyA
{
     private IImmutableList<string> _immutableStringList;

     public List<string> StringList { get; set; } = new List<string>();

     IImmutableList<string> IReadOnlyA.StringList
     {
         get
         {
             // An intersection will verify that the entire immutable list
             // contains the exact same elements and count of mutable list
             if(_immutableStringList.Intersect(StringList).Count == StringList.Count)
                return _immutableStringList;
             else
             {
                  // the intersection demonstrated that mutable and
                  // immutable list have different counts, thus, a new
                  // immutable list must be created again
                 _immutableStringList = StringList.ToImmutableList();

                 return _immutableStringList;
             }
         }
     }
}
Matías Fidemraizer
  • 63,804
  • 18
  • 124
  • 206
  • Even if the approach is totaly fine, i don't like the implementation for a couple of reasons. As example ImmutableArray should be simpler and should instead having a method "ToList". – Felix K. Nov 22 '15 at 20:21
  • @FelixK. `ImmutableArray` implements `IEnumerable`, so you'll get `ToList` as extension method already as long as you have `using System.Linq`. – MarcinJuraszek Nov 22 '15 at 20:25
  • @FelixK. As MarcinJuraszek, there's nothing more simpler than immutable collections! – Matías Fidemraizer Nov 22 '15 at 20:29
  • I guess making it immutable creates new clone every time. That would be a little overkill for this scenario. – Antonín Lejsek Nov 22 '15 at 20:42
  • @MarcinJuraszek I'm more taking about the huge number of methods of `ImmutableArray` would be much clearer if there where not all thoose methods implemented which copy the data each time. – Felix K. Nov 22 '15 at 20:47
  • @AntonínLejsek `System.Collections.Immutable` are implemented in a way that let's you reuse most of the items thought the modification. So it's not that you have to copy the entire thing every time you add a new item or anything. – MarcinJuraszek Nov 22 '15 at 20:49
  • @AntonínLejsek I don't find it an overkill. It might be a performance issue if it's an actual performance issue. OP should use a profiler if he/she thinks that calling `.ToImmutableXXXX` every time is the bottle neck of his/her app/service. – Matías Fidemraizer Nov 22 '15 at 21:59
  • Many thanks for the explained answer. Do i get this right: With ImmutableCollections I can add/remove items, but always get a ne List, while with ReadOnlyCollection I just can't do that? To be fair, I can't get a reason why I would ever want to publish such a Collection over the ReadOnlyCollection, but I guess both protect the inner List from being modified kindahow. – Matthias Müller Nov 23 '15 at 18:06
  • @MatthiasMüller Read-only collection exposes the same collection but in read-only mode. If the source list changes, the new items are accessible. Immutable collections, as you've said in your comment, create a new collection but it's not read-only but immutable. Most of the times you want to return a list of object and leave it *as is*. You don't want to get 10 items and later have 24 because the underlying collection has changed. Don't you find this a good reason to go with the immutable collections approach? ;) – Matías Fidemraizer Nov 24 '15 at 20:16
6

I do not think immutable is the way to go

int[] source = new int[10000000];//uses 40MB of memory
var imm1 = source.ToImmutableArray();//uses another 40MB
var imm2 = source.ToImmutableArray();//uses another 40MB

List behaves the same way. If I want to make full copy every time, I do not have to care about what user does with that array. Making it immutable does not protect content of objects in the collection either, they can be changed freely. @HansPassant suggestion seems to be best

public class A
{
    protected List<int> list = new List<int>(Enumerable.Range(1, 10000000));
    public IReadOnlyList<int> GetList
    {
        get { return list; }
    }
}
Antonín Lejsek
  • 6,003
  • 2
  • 16
  • 18
  • It's you who thinks that everyone is using an array of 10M of indexes... And when you use immutable collections you're not looking to also expose members as immutable objects. It would be wonderful, but at least the collection is provided *as is* and there's no way to modify its content, order, and other factors depending on the collection semantics. – Matías Fidemraizer Nov 22 '15 at 22:03
  • In addition, most apps/services won't use arrays anymore but higher level collections. I know `List` and other implementations use an array internally, but it's just an implementation detail, and again, most solutions won't manage milions of collection items in a single collection. In that edge case, you're going to optimize your code. Don't convert an exception into the rule – Matías Fidemraizer Nov 22 '15 at 22:05
  • 2
    @MatíasFidemraizer "Arrays are kinda bad, because someone could easily add a new Entry. So one would have to pass a new Array every time the method is called." It is OP who wants to avoid this creation on every call, not me. So question is if it can be avoided without problems with user changing the collection and You keep saying, that not avoiding it is not always such a problem. – Antonín Lejsek Nov 22 '15 at 22:32
  • 1
    I agree with this answer. If you are doing the immutable thing you can as well do `return Array.Clone` with the same effect. – Andrew Savinykh Nov 23 '15 at 04:54
  • @AntonínLejsek But OP states that he's returning `IEnumerable`, and now team mates are converting the `IEnumerable` to a collection because they ignore what would be the implementation of `IEnumerable` behind the reference. This is when immutable collections come to play: they're still an actual collection interface, but they're immutable, and they implement interfaces like `IList`, `ISet` and so on... – Matías Fidemraizer Nov 23 '15 at 08:34
  • 1
    At the moment I really don't see theuse of ImmutableCollections to be honest, that's why I would favorize ReadOnlyLists as well. Is there a easy description arround waht was the purpose of this ImmutableCollections? Cant' really see one – Matthias Müller Nov 24 '15 at 09:51
5

For a collection that you don't intend to modify, IEnumerable<T> is still probably the safest option, plus it allows any collection type to be pased in, not just arrays. The reason for that warning is because of the possibility that the IEnumerable represents a query that uses deferred execution, meaning that a potentially expensive operation could be executed multiple times.
Note that there's not an interface that distinguish in-memory collections versus potentially deferred-execution wrappers. That question has been asked before.

The fix for that is do not enumerate the source multiple times. If the code needs perform multiple iteartions (which may be legitimate) then hydrate the collection to a List<T> before iterating.

Community
  • 1
  • 1
D Stanley
  • 149,601
  • 11
  • 178
  • 240
  • For me, the issue with `IEnumerable` is that it can be a collection but it mustn't be a collection after all. It's an iterable sequence of objects/value objects. It's not a read-only collection. It's just a sequence. Before immutable collection came out I was going with this approach. – Matías Fidemraizer Nov 22 '15 at 22:07
  • My problem is just the Fact the Client does not know, what's behind the IEnumerable. In my example, it's an internal process, but what, if I publish an API? I was just really wondering, why Lippert suggests this, since you either have to always load it in the Memory via .ToList or you have to risk it and just work with the IEnumerable – Matthias Müller Nov 23 '15 at 16:52
  • @MatthiasMüller Why do you "always need to load it into memory"? If you are just enumerating once then there's no harm in just enumerating it without hydrating it. It's only when you need to enumerate multiple times that you have to weight the risk. – D Stanley Nov 23 '15 at 17:01
  • Well, this would mean I would ahv to enumerate once, which, I think at least, is the most of the time unnecessary, since it's already an InMemory-List. Sure that peformance issue will never matter, but I see your point. The Clien should decide, if he wants to use an InMemory-List and not a defered one. – Matthias Müller Nov 24 '15 at 09:54
5

IEnumerable is a read-only interface which hides implementation from user. Some can argue, that user may cast IEnumerable to list and add new items, but that means two things:

  • User violates provided API
  • You can't stop user from reflection usage

IEnumerable describes behavior, while List is an implementation of that behavior. When you use IEnumerable, you give the compiler a chance to defer work until later, possibly optimizing along the way. If you use ToList() you force the compiler to prepare the results right away.

I use IEnumerable Whenever working with LINQ expressions, because by only specifying the behavior, I give LINQ a chance to defer evaluation and possibly optimize the program.

To prevent any modifications to the List<T> object, expose it only through the ReadOnlyCollection<T> wrapper which does not expose methods that modify the collection. However, if changes are made to the underlying List<T> object, the read-only collection reflects those changes as described in MSDN.

Take a look at The New Read-Only Collections in .NET 4.5 please.

Amirhossein Mehrvarzi
  • 18,024
  • 7
  • 45
  • 70
  • 1
    `IEnumerable` isn't *the behavior*. It's just an iterable providing an iterator, and, in addition, an `IEnumerable` can be implemented by other classes not being collections at all. It's just a sequence of objects/value types. – Matías Fidemraizer Nov 22 '15 at 21:57
0

I've been programming for longer than I care to remember, and never had such a requirement to protect a list from getting modified. I'm not saying it's not a possible requirement, I'm just saying it is very rare to need such a requirement. If you have a list circulating around in your app, then most likely you have to fix your design. If you need help with that, let us how you're using the list.

The examples you're giving in your question and comments are not good examples for when to require an immutable or read-only list. Let's discuss them one by one.

  1. You mentioned publishing it as an API. By definition, anything you return from an API is no yours anymore and you shouldn't care how it is used. In fact, once it leaves your API, it is now in the API client's domain, and they can do whatever they want with it. Aside from the fact that you cannot protect it once it is in their domain, you should not dictate how they will use it. More importantly, you should never accept anything as input in your API, even if it is the same list that you returned earlier and you think that you protected. All input MUST be validated appropriately.

  2. Perhaps, you did not really mean API, but more like a DLL library that you share in your projects. Whether it is a DLL or just a class in your project, the same principle applies. When you return something, it is up to the user how to use it. And you should never accept the same thing (list or whatever) back without validation. Similarly, you should never expose an internal member of your class, whether it's a list or a single value. After all, that's the whole idea behind using properties instead of marking the members as public. When you create a property for a single-value member, a copy of the value is returned. Similarly, you should create a property for your list and return a copy, not the list itself.

If you really need a list that is globally accessible, and you want to load it only once, expose it to other classes, protect it against modification, and it is too big to make a copy of it. You can look into some designs like wrapping it in a Singleton and/or make it read-only as per Antonín Lejsek's answer. In fact, his answer can be easily converted to a Singleton by marking the constructor as private, thanks to the simplified Singleton implementation in C#.

Community
  • 1
  • 1
Racil Hilan
  • 24,690
  • 13
  • 50
  • 55