views:

175

answers:

3

Do not expose generic lists

IF all my methods, need to expose a collection, then I need to user the Linq Extension .ToList(), almost everywhere I need to use lists, or user Collections in all my code.

If that’s the case, .ToList() is ignoring the rule right? Or is there a technique like copying the list o something to fix the violation and still return a list?

+5  A: 

I disable that rule because I don't feel like it's a valid one. In you want to return a collection which contains a O(1) count and is not a direct reference to an internal field, List<T> is the best choice.

I don't deeply understand your case here but it sounds like you have a method which returns a LINQ query over some internal data. If that's the case then using a .ToList() on the data is appropriate since you likely don't want future modifications of your internal fields to affect the return value of a method. In that case there is no reason to not expose it as a List<T>.

JaredPar
Disable rules is like cheating no?... the rules exists for a reason.
Fraga
@Fraga, Nope it's not cheating and a I frequently disable certain rules for varying reasons. They are not universal truths and often get in the way of designing quality libraries.
JaredPar
They're more like "guidelines" anyway.
Anthony Pegram
@Fraga: Having read the justification for the rule, I would diable it too. The justification is "System.Collections.Generic.List<T> is a generic collection designed for performance not inheritance and, therefore, does not contain any virtual members." What inheritance has to do with anything is beyond me.
R. Bemrose
`IList<T>` is preferable as the return type if you do want to return a list. I generally still use `IEnumerable<T>` as the return type (even if I call `.ToList()` or `.ToArray()`), as this gives the implementing method more flexibility. Most LINQ functions are smart enough to optimize access when the underlying object is `IList<T>`. In particular, `.Count()` and `.ElementAt()` are both still O(1) operations, so you don't take a major performance hit by returning `IEnumerable<T>`.
Dan Bryant
@ R. Bemrose, The concern, I believe, is that you're exposing an implementation detail if you return a `List<T>`. If your function later uses some different data structure, changing the return type would be a breaking change, so you could end up creating a copy or wrapper in order to maintain the public interface. You have to decide how much information you wish to guarantee in your public contract and the less, the better.
Dan Bryant
Microsoft: "Do not suppress a warning from this rule unless the assembly that raises this warning is not meant to be a reusable library", but I believe you guys!, I'll disable it... with all the pain of my heart
Fraga
@Fraga, Microsoft's own assemblies don't obey this rule (see System.Core) so it's definitely not an immutable truth.
JaredPar
@Dan Bryant: That brings up the question "Why `ObjectModel.Collection<T>` instead of `Generics.ICollection<T>`?"
R. Bemrose
@R Bemrose, That's a good question. I'm wondering if some of these warning descriptions should be updated for the LINQ era, as my understanding of the generally accepted best practice these days is to use `IEnumerable<T>` in almost all cases and let LINQ deal with messy implementation details.
Dan Bryant
@R. Bemrose `Collection<T>` is compatible with the older `ICollection` and `IList` classes in addition to the generic ones. The `IList<>` and `ICollection<>` do not inherit from them, so if you were writing to be compatible with aged code, it'd be suitable to use the `Collection<>`. The `Collection<>` is just a silly wrapper class around the common generic ones though - its default constructor instantiates a `List<>` :p. I'm with the rest that this is a silly rule and should be ignored, although I'd take Dan's advice and make everything return an `IList<>`, `ICollection<>` or `IEnumerable<>`.
Mark H
@Mark H Collection<T> is not just a silly wrapper. See my example below.
Nick Guerrera
@JaredPar I agree that `List<T>` is used correctly in Linq. I also agree that the rule is not an immutable truth. However, there are plenty of places in the .NET Framework where rules have been violated with dubious justification. The rules are not infallible, but neither is the framework.
Nick Guerrera
I upgraded SubSonic once and they had changed all their usages of List<T> into the little known BindingList<T>. Urgh. Ever since then I have always programmed to IList<T> unless performance is a consideration.
cbp
+3  A: 

Remember that all these rules were written for framework developers. Many of them are likely to be unsuitable unless you're also writing a framework.

You'll have to make a judgement call for every rule to see if it's valid for your circumstances. I like to use the analysis since it does find some bugs sometimes, but I always end up disabling certain rules (for example, I quite often have a catch Exception as a final catch-all just because I need to log all kinds of errors even if they can't be handled).

ho1
+3  A: 

This rule can indeed be noisy, but there are some very valid reasons to avoid List<T> in library code. It all depends on the context. Here are a few things to consider before disabling the rule or suppressing a given occurrence:

  • List<T> is often a poor choice for input parameters as it forces callers to copy data unnecessarily. I have seen lots of code that declares parameters as List<T> or T[] when IEnumerable<T> would suffice.

  • List<T> can be a poor choice for properties as well. Consider the following alternatives:

    public class Course {
        public List<Course> Prerequisites { get; }
    }
    public class Course {
        public Collection<Course> Prerequisites { get; }
    }
    

    The intention is that the caller can change a course's prerequistes by modifying the collection. In that case, if we use List<Course>, there is no way for the Course class to be notified when the prerequisites change since List<T> does not provide any modification callbacks. As such, using List<T> in this context is like having arbitrarily many public fields. On the other hand, we can subclass Collection<T> and override its virtuals to be notified of changes.

List<T> works best as a return value when complete ownership of the collection is transferred to the caller. This is why Enumerable.ToList() is actually perfectly reasonable and it does not violate the spirit of the rule.

Now that I think about it, allowing List<T> as a return value from methods, but continuing to flag List<T> properties and parameters would probably greatly improve the rule's signal to noise ratio...

Nick Guerrera
If `Course` simply wants to be notified when its prerequisites change, then `BindingList<Course>` would be a good option. It has events you can subscribe to, instead of having to derive a new class as with `Collection<Course>`
Ben Voigt
@Ben Voigt Indeed. There is also `ObservableCollection<T>`. My point was not that `Collection<Course>` is necessarily the best choice, just that `List<Course>` is a poor choice in this context.
Nick Guerrera