The more you know – Why List<T> is bad with query results

I’ve done a lot of work with LINQ to Data lately (both LINQ to SQL and others).  I spend more then the better part of a day checking my Google Reader checking for blog posts and/or articles from the .NET community.  There’s a lot you can learn going through some of these sites and a lot you shouldn’t.

One thing I’ve noticed a lot of sites doing (won’t name any) is that when they show off their DAL/BLL I see them returning a List<T> from their query results.  Am I the only one that sees the problem there?  Returning an actual List<T> object allows the consumer of your query results to actually mess with the order, remove items and do whatever they want with the resulting item list.  This is a very bad thing.

When you return your query results you should be returning the results they asked for and nothing more.  You should try to hold as much responsibility for the integrity of the data being returned and try to not allow data results to be messed with.  If the consumer of your service/repository wants to make it into a list then that’s their responsibility, not yours.  If you’re wanting to respond with that with something like “But, I want to filter the data down more.”  Then please, add a method with some parameters onto your service/repository.  The results you obtain should be just that, results.  They should need no filtering, sorting, aggregating or changing.

Here are some examples.

public List<Product> GetProducts()
{
    return ProductsRepository.GetProducts()
                             .ToList();
}

This is, bad.  Doing this allows the consumer to manipulate the results.  What should happen is something like this.

public IEnumerable<Product> GetProducts()
{
    return ProductsRepository.GetProducts()
                             .ToList();
}

The simple change here was the return type.  Now we’re just returning an IEnumerable<T> instead.  If you really wanted to you could return a System.Collections.ObjectModel.ReadOnlyCollection<T> but I’ve heard of people having some performance issues with it in the past so I would just stick with IEnumerable<T>.

So as you can see when you return an IEnumerable<T> the only thing they can do with the returning results without doing some LINQ against it is enumerate them.  Which is probably what you want to return.

The more you know – and shooting star!

8 Comments

  • > the only thing they can do ... is enumerate them

    ... or cast back to a List!

    If you're really bothered about this (and you probably only need to be if the DAL/BLL is caching the list being returned), it's better to return a ReadOnlyCollection using .ToList().AsReadOnly() - and probably in this case the items in the list would want to be immutable.

    In any case I think it's often better to return an IList than IEnumerable. Gives the consumer explicit access to the count of items returned.

  • @Joe

    Yeah you're right I was thinking about that as I wrote it. Again though they could easily convert enumerable object into a list if they wanted to. I just think it's important to make sure who is responsible for what.

  • I totally disagree with you here, why should you lock the consumer into what they can do with the returned results?

    I personally think returning an IList is the way to go and I will continue doing this. I see nothing wrong with this as I am returning a list of items, and I do not care what the consumer (UI) does with these results.

    If the UI decides to do some filter on this itself, this should not really be a concern anyway.


    Just my 2 cents :)


    Cheers

  • I disagree as well. Returning List is bad when the list is a pointer to a internal list of your class. If this is the case, it is indeed very bad because you're giving the users of your class a loophole around your checks, effectively breaking the encapsulation.

    If, however, the returned class is just a list that is built and returned back to the user without keeping a reference I see nothing wrong with returning a List. I would even say that returning a List would be better than IList. Unfortunately the IList interface doesn't have all the nice functions found in List and it's sometimes useful / necessary to typecast to List; you may as well just return List. Are you really planning on building your own IList class? If this is the case, fine, use IList, otherwise it's just another case of over engineering.

  • I've always been dogmatic so I guess this is just me trying to try and separate concerns further. After completing a small ASP.NET MVC blog on my own I realized you guys are right and I'll re-write this post.

    Thanks guys.

  • Cant you use .Count(), I've favoured the IEnumerable route for a long time now and not encountered any issues, its encapsulate the type of collection should you ever refactor.

    For Nhibernate mappings any IList is a private variable mapped by private field accessor, the IEnumerbale public route just get the list as IEnumerable. I dont want consumers adding to list or messing with it, the collection (not the items) should be immutable in that respect, they can always create a new List by passing the IEnumerable to the new List.


  • Agree with Xtek, returning List is bad almost always, in my entities i use this in my getters:
    get { return new List(_reports).AsReadOnly()}

    For me, it is inadmissible that the consumer of such entities can get direct access to the private member collection, even, for critical cases i just return a ReadOnlyCollection of DTOs:
    get { return allReportDTOs()}

    Where ReportDTO hav only primitive members

    In fact, i'm using the DTOs way intensively today, because currently the UI guys are using a Presentation Model that get binded the entities properties collections to grids. Sure they can not Add/Remove items, but they can change the child properties directly from the GUI. That is a true problem, because many often those changes requieres some validation at the parent entity level.

    Elio

  • Wow! I could not even guess about it)) Not bad.

Comments have been disabled for this content.