1

I have the following code:

public interface ISomeObject
{
     IList<ISomeObject> Objects { get; }
}
public class SomeObject : ISomeObject
{
    public SomeObject()
    {
        Objects = new List<SomeObject>();
    }
    public List<SomeObject> Objects
    {
         get;
         set;
    }
    IList<ISomeObject> ISomeObject.Objects
    {    
        get 
        {
            // What to do here?
            // return Objects; // This doesn't work
            return Objects.Cast<ISomeObject>().ToList(); // Works, but creates a copy each time.
         }
    }

SomeObject has a public property Objects that returns a List of class type. Clients knowing that class type can use that to do whatever they want. Clients only knowing about ISomeObject can use the Objects property only to get an IList<ISomeObject>. Because it is not allowed to cast List<SomeObject> to IList<ISomeObject> (due to the apple and banana issue) I need a way of converting that. The default way, using a Cast.ToList() works, but has the downside that it creates a new List each time the property is evaluated, which may be expensive. Changing ISomeObject.Objects to return an IEnumerable<ISomeObject> has the other downside that the client can't use indexing any more (which is quite relevant in my use case). And using Linq's ElementAt() call repeatedly is expensive, when used on an IEnumerable.

Has anybody got an idea on how to avoid either problem? (of course, making SomeObject known everywhere is not an option).

PMF
  • 14,535
  • 3
  • 23
  • 49
  • If indexing is an important use case for the consumers, why doesn't the `ISomeObject` interface also have an indexer as well as/instead of the `Objects` property? – Damien_The_Unbeliever May 25 '18 at 09:52
  • You could/should implement a class similar to `ReadOnlyCollection` to act as a proxy. Considering that it would be read only, it could be covariant, like IEnumerable – xanatos May 25 '18 at 09:57
  • Why do you believe that Linq's ElementAt will be a performance issue? It is optimised to check if the `IEnumerable` is in fact a `IList` and than calls the indexed property. See the source code: https://referencesource.microsoft.com/#System.Core/System/Linq/Enumerable.cs,7db56d44563d8761 – Rand Random May 25 '18 at 09:58
  • @RandRandom (Copied from my comment on NineBerry's answer) `ElementAt` does check if the object behind `IEnumerable` supports `IList`, but it won't support that (it only supports `IList`), so that check is just a waste of time. –  May 25 '18 at 10:09
  • @hvd Already saw your comment, and upvoted it - never thought about it. – Rand Random May 25 '18 at 10:10
  • @RandRandom; I was just thinking about this as well, but unfortunately, it doesn't work... – PMF May 25 '18 at 10:24
  • @Liam: No, the given link explains just the solution I _don't_ want to use. – PMF May 25 '18 at 11:04

3 Answers3

4

You could/should implement a class similar to ReadOnlyCollection<T> to act as a proxy. Considering that it would be read only, it could be "covariant" (not language-side, but logically, meaning that it could proxy a TDest that is a subclass/interface of TSource) and then throw NotSupportedException() for all the write methods.

Something like this (code untested):

public class CovariantReadOlyList<TSource, TDest> : IList<TDest>, IReadOnlyList<TDest> where TSource : class, TDest
{
    private readonly IList<TSource> source;

    public CovariantReadOlyList(IList<TSource> source)
    {
        this.source = source;
    }

    public TDest this[int index] { get => source[index]; set => throw new NotSupportedException(); }

    public int Count => source.Count;

    public bool IsReadOnly => true;

    public void Add(TDest item) => throw new NotSupportedException();

    public void Clear() => throw new NotSupportedException();

    public bool Contains(TDest item) => IndexOf(item) != -1;

    public void CopyTo(TDest[] array, int arrayIndex)
    {
        // Using the nuget package System.Runtime.CompilerServices.Unsafe
        // source.CopyTo(Unsafe.As<TSource[]>(array), arrayIndex);
        // We love to play with fire :-)

        foreach (TSource ele in source)
        {
            array[arrayIndex] = ele;
            arrayIndex++;
        }
    }

    public IEnumerator<TDest> GetEnumerator() => ((IEnumerable<TDest>)source).GetEnumerator();

    public int IndexOf(TDest item)
    {
        TSource item2 = item as TSource;

        if (ReferenceEquals(item2, null) && !ReferenceEquals(item, null))
        {
            return -1;
        }

        return source.IndexOf(item2);
    }

    public void Insert(int index, TDest item)
    {
        throw new NotSupportedException();
    }

    public bool Remove(TDest item)
    {
        throw new NotSupportedException();
    }

    public void RemoveAt(int index)
    {
        throw new NotSupportedException();
    }

    IEnumerator IEnumerable.GetEnumerator()
    {
        return GetEnumerator();
    }
}

Use it like:

IList<string> strs = new List<string>();
IList<object> objs = new CovariantReadOlyList<string, object>(strs);
xanatos
  • 109,618
  • 12
  • 197
  • 280
  • Any reason why `IndexOf` isnt implemented as `return source.IndexOf((TSource)item);` ? – Rand Random May 25 '18 at 10:29
  • @RandRandom You can't upcast an unknown `item` (`item` is a parameter). In the example given: `objs.IndexOf(new object())` – xanatos May 25 '18 at 10:30
  • @RandRandom I could TSource item2 = item as TSource; and use the `is` operator... but then i'll have to check for `null`, and the code will still be unreadable. – xanatos May 25 '18 at 10:33
  • Not entirely sure what you mean, so I just went ahead and tested my question and it seems to work. So my index of looks like this: `public int IndexOf(TDest item) { return source.IndexOf((TSource)item); }` - the declaration of the class looks like this `var x = new CovariantReadOlyList(list);` - and I call `IndexOf` like this: `var index = x.IndexOf(list[1]);` which results in the same index as the `source` list has `list[1]` – Rand Random May 25 '18 at 10:39
  • @RandRandom Changed the code... It is as much ugly as before, but perhaps it will be faster, because the `IndexOf` will be executed totally internally the `source`, instead of going through the roundtrips of the enumerator. – xanatos May 25 '18 at 10:39
  • 1
    @RandRandom Try using the `IList strs = new List(); IList objs = new CovariantReadOlyList(strs); objs.IndexOf(new object());` – xanatos May 25 '18 at 10:40
  • @xanatos: Ah, and then you would create the List and the CovariantReadonlyList both in the constructor of the containing class? – PMF May 25 '18 at 11:09
  • @PMF In general the `List` is the real object, and when needed you return a `return new CovariantReadonlyList(realList)`. The creation cost of `CovariantReadonlyList` is nearly zero, because it only stores a reference to the original `List<>` – xanatos May 25 '18 at 11:17
  • `strs.AsReadOnly()` will return such wrapper already (`ReadOnlyCollection`) so I see no reason to manually implement it. – Evk May 26 '18 at 15:46
  • @evk The question was about IList, not IReadOnlyList. There is another response about that other option. Perhaps he need an IList because his interfaces are written in that way. IReadOnlyCollection in general is a better solution **if** you can use it – xanatos May 26 '18 at 16:44
  • @xanatos List.AsReadOnly method returns ReadOnlyCollection (not interface). This class implements multiple interfaces, including IList (and so - is a built-in analog of what you written in this answer). So if OP wants IList - he can still use that (though OP needs just indexer and so - IReadOnlyList is a better choice, and is also implemented by ReadOnlyCollection). – Evk May 26 '18 at 16:52
  • @evk List.AsReadOnly() returns a IList, the question is about obtaining a read only IList. If the question is XY then your response is good, and you should post it. – xanatos May 26 '18 at 17:04
  • 1
    Well I realize now I'm wrong, because covariance is involved, so you cannot just `return Objects.AsReadOnly()` and leave interface as `IList<>`. Yet I still think all that is not needed and OP should just change interface to `IReadOnlyList` (since he needs indexer and this interface has indexer, and is covariant in T), and do `return Objects.AsReadOnly()`. It's better in all ways. – Evk May 26 '18 at 17:04
3

Changing ISomeObject.Objects to return an IEnumerable<ISomeObject> has the other downside that the client can't use indexing any more (which is quite relevant in my use case).

Indexing isn't just supported by the IList<T> interface, it's also supported by the IReadOnlyList<T> interface. Because IReadOnlyList<T> doesn't allow modification, it can be (and is) covariant just like IEnumerable<T> is.

So, just change the return type to IReadOnlyList<ISomeObject> and return the original list.

Of course, nothing prevents the caller from casting the result to List<SomeObject>, but the caller is supposed to have full access to that list anyway, so there is no security risk.

  • As with some other things in .NET, I have a little sadness that `IReadOnlyList<>` wasn't supported from .NET 2.0. Many methods would have been written with a different signature, making it clear that the collection they receive/return is readonly. – xanatos May 25 '18 at 10:29
  • This solution would work, if List was implementing IReadonlyList (it seems it doesn't), so I couldn't do this without the wrapper from xanatos' answer – PMF May 25 '18 at 14:23
  • @PMF [`List` does implement `IReadOnlyList`](https://msdn.microsoft.com/en-us/library/6sh2ey19(v=vs.110).aspx) though. –  May 25 '18 at 14:24
  • @PMF it indeed implements `IReadOnlyList` in both full .NET and in .NET Core. Even more, `List` has method `AsReadOnly` which will return `ReadOnlyCollection`. This is built-in wrapper similar to what is provided in xanatos answer, so there is no reason to manually implement it. So correct way would be to change interface to `IReadOnlyList`, and then do `return Objects.AsReadOnly()` (this method does no copying). – Evk May 26 '18 at 15:45
  • @Evk: You are right. Looks like I have been looking at an old version of the MSDN. – PMF May 28 '18 at 07:19
  • For reasons unrelated to the question (the actual code has some further goodies, like MarshalByRef behavior etc.), I still prefer xanatos' solution. – PMF May 28 '18 at 07:27
0

You may want try to encapsulate your List<SomeObject> making it an implementation detail and return IReadOnlyList<SomeObject> instead. Then SomeObject to ISomeObject cast want be unnecessary in interface implementation as well due to IReadOnlyList variance — you'll be able to return your Objects as IReadOnlyList<ISomeObject> .

Then just add some operations to mutate your underlying list like Add or Remove to container type if those are required.

Also I should mention that interfaces are not so good for restriction — evil consumer can easily cast your ISomeObject to SomeObject and do everything he wants, probably, you should reconsider your design. You'd better stick to such things as immutability and encapsulation for providing usable api. Explicitly use mutable builders then for immutable classes where it's reasonable.

Uladzislaŭ
  • 1,680
  • 10
  • 13
  • In the real case, ISomeObject is declared in another assembly than SomeObject. The clients don't reference the assembly with SomeObject in it, so they can't do that cast. (And SomeObject should later be internal) – PMF May 25 '18 at 10:59
  • Also, my implementation requires the lists to be changed quite often (by users that know the class interface), but seldom otherwise. – PMF May 25 '18 at 11:02