3
\$\begingroup\$

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);
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Your title says "collection of two objects". Do you mean "two collections of objects"? \$\endgroup\$ May 30, 2015 at 9:51

1 Answer 1

5
\$\begingroup\$

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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ May 30, 2015 at 23:16
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ May 31, 2015 at 16:58
  • 1
    \$\begingroup\$ What about .Select(x => new Foo(x.Bar, ...))? I'm not sure exactly how it'd be different from ConvertAll. \$\endgroup\$
    – RubberDuck
    May 31, 2015 at 17:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.