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
    \$\begingroup\$ Your title says "collection of two objects". Do you mean "two collections of objects"? \$\endgroup\$ – 200_success May 30 '15 at 9:51
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\$
  • 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\$ – Mathieu Guindon May 30 '15 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\$ – Three Value Logic May 31 '15 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 '15 at 17:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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