11

I have "best practices" question about OOP in C# (but it sort-of applies to all languages).

Consider having library class with object that is to be exposed to public, say via property accessor, but we do not want the public (people using this library class) to change it.

class A
{
    // Note: List is just example, I am interested in objects in general.
    private List<string> _items = new List<string>() { "hello" }

    public List<string> Items
    {
        get
        {
            // Option A (not read-only), can be modified from outside:
            return _items;

            // Option B (sort-of read-only):
            return new List<string>( _items );

            // Option C, must change return type to ReadOnlyCollection<string>
            return new ReadOnlyCollection<string>( _items );
        }
    }
}

Obviously the best approach is "option C", but very few objects have ReadOnly variant (and certainly no user-defined classes have it).

If you were the user of class A, would you expect changes to

List<string> someList = ( new A() ).Items;

propagate to original (A) object? Or is it okay to return a clone provided that it was written so in comments/documentation? I think that this clone approach might lead to quite difficult-to-track bugs.

I remember that in C++ we could return const objects and you could only call methods marked as const on it. I guess there is no such feature/pattern in the C#? If not why would they not include it? I believe it is called const correctness.

But then again my main question is about "what would you expect" or how to handle Option A vs B.

10

Besides @Jesse's answer, which is the best solution for collections: the general idea is to design your public interface of A in a way so that it does return only

  • value types (like int, double, struct)

  • immutable objects (like "string", or user defined classes which offer only read-only methods, just like your class A)

  • (only if you must): object copies, like your "Option B" demonstrates

IEnumerable<immutable_type_here> is immutable by itself, so this is just a special case of the above.

19

Also note you should be programming to interfaces, and by and large, what you want to return from that property is an IEnumerable<string>. A List<string> is a bit of a no-no because it is generally mutable and I'll go so far to say that ReadOnlyCollection<string> as a implementor of ICollection<string> feels like it violates the Liskov Substitution Principle.

  • 6
    +1, using an IEnumerable<string> is exactly what one wants here. – Doc Brown Jan 29 '13 at 15:28
  • 2
    In .Net 4.5, you can also use IReadOnlyList<T>. – svick Jan 29 '13 at 16:24
  • 3
    Note for other interested parties. When considering the property accessor: if you do just return IEnumerable<string> instead of List<string> by doing return _list (+implicit cast to IEnumerable) then this list can be cast back into List<string> and changed and the changes will propagate into the main object. You can do return new List<string>( _list ) (+subsequent implicit cast to IEnumerable) which will protect the internal list. More on this can be found here: stackoverflow.com/questions/4056013/… – NeverStopLearning Jan 29 '13 at 16:37
  • 1
    Is it better to require that any recipient of a collection who needs to access it by index must be prepared to copy the data into a type which facilitates that, or is it better to require that the code returning the collection supply it in a form that is accessible by index? Unless the supplier is likely to have it in a form which does not facilitate access by index, I would consider that the value of freeing the recipients from the need to make their own redundant copy would exceed the value of freeing the supplier from having to supply a random-access form (e.g. a read-only IList<T>) – supercat Dec 19 '13 at 17:55
  • 1
    One thing I don't like about IEnumerable is that you never know whether it runs as a coroutine or whether it is just a data object. If it runs as a coroutine it could result in subtle bugs so this is good to know sometimes. This is why I tend to prefer ReadOnlyCollection or IReadOnlyList etc. – Steve Vermeulen Sep 14 '16 at 14:04
3

I encountered a similar problem a while back and below was my solution, but it really only works if you control the library too:


Start with your class A

public class A
{
    private int _n;
    public int N
    {
        get { return _n; }
        set { _n = value; }
    }
}

Then derive an interface from this with only the get portion of the properties (and any reading methods) and have A implement that

public interface IReadOnlyA
{
    public int N { get; }
}
public class A : IReadOnlyA
{
    private int _n;
    public int N
    {
        get { return _n; }
        set { _n = value; }
    }
}

Then of course when you want to use the read only version, a simple cast is sufficient. This is just what you solution C is, really, but I thought I'd offer the method with which I achieved it

  • 2
    Problem with this is, that its possible to cast it back to mutable version. And its violiation of LSP, bacause state observable through base class's (in this case interface's) methods can be changed by new methods from subclass. But it at least comunicates intent, even if it does not enforce it. – user470365 Jan 29 '13 at 15:25
1

For anyone who finds this question and still is interested in the answer - now there is another option which is exposing ImmutableArray<T>. These are few points why I think it's better then IEnumerable<T> or ReadOnlyCollection<T>:

  • it clearly states that it is immutable (expressive API)
  • it clearly states that collection is already formed (in other words it's not initialised lazily and it's OK to iterate throught it multiple times)
  • if the count of items is known, it does not hide that knowlegde like IEnumerable<T> does
  • since client doesn't know what is the implementation of IEnumerable or IReadOnlyCollect he/she cannot assume that it never changes (in other words there is no guarantee that items of a collection have not been changed while reference to that collection stays unchanged)

Also if APi needs to notify client about changes in the collection I would expose an event as a separate member.

Note that I'm not saying that IEnumerable<T> is bad in general. I'd still use it when passing collection as an argument or returning lazily initialised collection (in this particular case it makes sense to hide item count because it's not known yet).

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.