Convert two collections of different objects into a single collection

Question

I am combining two lists of separate objects into a single combined list. The below code works perfectly but it is not the most elegant or efficient solution. Is there a better solution?

var articles = context.Articles.Where(a => a.Author.ID == authorID).ToList();
var reviews = context.Reviews.Where(a => a.Author.ID == authorID).ToList();
var items = articles.ConvertAll(x => new ItemViewModel(x.ID, x.Title, x.Summary, "article", x.PublishDate)).Union(reviews.ConvertAll(r => new ItemViewModel(r.ID, r.Title, r.Summary, "review", r.PublishDate))).Take(20).OrderByDescending(i => i.publishDate);

• 200_success: Your title says "collection of two objects". Do you mean "two collections of objects"?

Answer 1

Let's start with some sensible line continuation and indentation so we can see what's actually going on here.

var articles = context.Articles.Where(a => a.Author.ID == authorID).ToList();
var reviews = context.Reviews.Where(a => a.Author.ID == authorID).ToList();
var items = articles.ConvertAll(x => new ItemViewModel(x.ID, x.Title, x.Summary, "article", x.PublishDate))
 .Union(reviews.ConvertAll(r => new ItemViewModel(r.ID, r.Title, r.Summary, "review", r.PublishDate))
 ).Take(20)
 .OrderByDescending(i => i.publishDate));

Not perfect, but better. Remember, it's not necessary to cram all those lambdas onto a single line.

Is there a reason you're calling ToList()? That forces the execution of the query. If this is hitting a database with a lot of records, that could be potentially expensive.

You could also potentially clean up the second query a bit by moving the "converts" to the the prior queries.

var articles = context.Articles.Where(a => a.Author.ID == authorID)
 .ConvertAll(x => new ItemViewModel(x.ID, x.Title, x.Summary, "article", x.PublishDate));
var reviews = context.Reviews.Where(a => a.Author.ID == authorID)
 .ConvertAll(r => new ItemViewModel(r.ID, r.Title, r.Summary, "review", r.PublishDate));
var items = articles.Union(reviews)
 .Take(20)
 .OrderByDescending(i => i.publishDate));

Which shows us a potential opportunity to extract a method to remove the duplication.

Lastly, are you sure this does what you intend it to do? You didn't tell us exactly what you're trying to accomplish, but usually I would want to sort the results prior to selecting the top x results. I can't recall off the top of my head which order of Take and OrderBy would produce the correct results, but you should definitely double check that you're getting the information that you think you are.

*You might need to work on the Union syntax a bit. I'm not sitting in front of my IDE, so I didn't try to compile it.

• Mathieu Guindon: I might be wrong, but I doubt the ConvertAll call would be understood by the underlying provider - +1 for the Take vs OrderBy though; if the first list has more than 20 items the result will only ever contain items from articles AFAIK.
• Three Value Logic: Hi RubberDuck, As Matt says I convert to list to make use of convertall although you point out it is expensive. You are completely correct on switching take and order by.
• RubberDuck: What about .Select(x => new Foo(x.Bar, ...))? I'm not sure exactly how it'd be different from ConvertAll.