4

So in my class I have this private readonly member ICollection<IMusicItem> playlist. I would prefer to use the interface ICollection<T>.

I would like to use the List<T>.AddRange(IEnumerable<T> items). In my method would it be dangerous to cast the ICollection to a List<T> even if I instantiate the ICollection<T> as a new List<T>() in the constructor.

Is this bad practice, is there a better way of doing this?

Or is it just better to have a List<T>

Callum Linington
  • 14,213
  • 12
  • 75
  • 154
  • Use an extension method perhaps, like in [this SO answer](http://stackoverflow.com/a/1667634/996081)? – cbr Jul 09 '15 at 20:28
  • You can create an extension method `AddRange` for any `T` collection. A generic one. – AgentFire Jul 09 '15 at 20:29
  • 5
    What are your reasons to insist on the interface type on a **private** member, especially when you are instantiating a concrete type that would let you do what you want? – nodots Jul 09 '15 at 20:29
  • That is a very good point actually – Callum Linington Jul 09 '15 at 20:29
  • @nodots I like the use of an interface, feels safer to me :P – Callum Linington Jul 09 '15 at 20:30
  • I've had a really long day so my mind is a bit dead :P – Callum Linington Jul 09 '15 at 20:34
  • 1
    @CallumLinington Interfaces are powerful tools, but it's not universally perferable to be using them. The purpose of interfaces is to allow you to write code that only relies on the behavior of the interface, and that can be given multiple different concrete implementations that are transparent to it. Here the interface doesn't provide the needed behavior, and you're relying on the behavior of the concrete type. You should reflect that reliance in the type of the variable, rather than removing static typing and claiming you don't have a reliance on something you are in fact relying on. – Servy Jul 09 '15 at 20:36

2 Answers2

9

It's bad practice, because it breaks encapsulation. Using an interface is good, but it's pointless if you have to cast the object back to a concrete type. Act as if you didn't know the concrete type, or it's a future bug waiting to happen if you decide to switch to another type later on.

Use an extension method instead:

public static void AddRange<T>(this ICollection<T> collection, IEnumerable<T> items)
{
    foreach (var item in items)
        collection.Add(item);
}

Note: It's better to expose interfaces in a public API so you're free to change the implementing object later, but it's a matter of style whether to do it on private fields. You may as well use the concrete class.

Lucas Trzesniewski
  • 50,214
  • 11
  • 107
  • 158
  • 3
    Note that this is notably reducing the performance over `LIst.AddRange`, as `List.AddRange` can prevent multiple resizes of the backing array unlike this method. – Servy Jul 09 '15 at 20:37
  • @Servy that's true, but only *if* `items is ICollection`, also in that case `List.AddRange` allocates an array for the new items, fills it with the sequence and then copies it back to the list, so you get 2 copies of the data. – Lucas Trzesniewski Jul 09 '15 at 20:40
3

It's not dangerous (when done right), just pointless.

if (playlist is IList<IMusicItem>)
{
  (playList as IList<IMusicItem>).AddRange(items);
} 
else
{
   // still need a foreach here
}

The real issue is "I would prefer to use the interface ICollection<T>".

Why exactly? Your question suggests it always is a List, so why not expose it as such?

Exposing it as a more general ICollection<> only makes sense when other implementations than List might exist, and then the casting is useless.

H H
  • 263,252
  • 30
  • 330
  • 514
  • 1
    Yeah, that was more what I was looking for. So my understanding is, expose interfaces to clients, but internally just use the concrete implementation. – Callum Linington Jul 09 '15 at 20:51