CodeSOD: All the News You Need

Alexandar works with a veteran software architect. It's important to note here that a veteran is someone who has had experience. It certainly doesn't mean that they learned anything from that experience.

This veteran was given a task to write a C# method to populate a user's news feed. The goal was to find the five most recent news articles and add them to the list. Now, this is a large scale CMS, so those articles need to be fetched from ElasticSearch.

This was their solution:

[JsonIgnore]public List<NewsPage.NewsPage> AllNews{ get { var pages = SearchClient.Instance.Search<NewsPage.NewsPage>() .ExcludeDeleted() .GetContentResult(); if (this.NewsType == null) { return new List<NewsPage.NewsPage>(); } var returnedPages = pages.ToList().Where(_ => !string.IsNullOrEmpty(_.NewsType) && _.NewsType.Contains(this.NewsType)) .OrderByDescending(_ => _.PublicationDate).Take(5).ToList(); return returnedPages; }}

I'll let Alexandar summarize:

I find this snippet almost magical, every line is wrong, sometimes on multiple levels.

Alexandar overstates the wrong-ness- the lines that are just curly brackets or the word get are fine. It's all the other lines which are wrong.

First, let's start with the method name, AllNews. This does not fetch all news. It's supposed to fetch the five most recent stories. We're off to a good start.

But what about our fetch?

 var pages = SearchClient.Instance.Search<NewsPage.NewsPage>() .ExcludeDeleted() .GetContentResult();

So, we search for all the news articles, excluding the deleted ones. There are a few problems with this. First, in addition to deleted articles, there are articles that are in draft states or articles which are embargoed until a certain date. Articles which we shouldn't show the users- which this method doesn't exclude.

There's also no ordering here, or a number of items we're requesting. Which means that, by default, this is going to return the first ten items it fetches. With no ordering, that means it's essentially ten random items- the first ten things ElasticSearch serves up.

So, once we have made a request and fetched ten random items which may or may not be appropriate to show our end users, we then check to see if there should be any news at all. If there's not, we return an empty page. We could have done that before sending a request to ElasticSearch, but who doesn't like throwing unnecessary requests into your processing pipeline?

Finally, we get to the LINQ calls:

 var returnedPages = pages.ToList().Where(_ => !string.IsNullOrEmpty(_.NewsType) && _.NewsType.Contains(this.NewsType)) .OrderByDescending(_ => _.PublicationDate).Take(5).ToList();

So, we start with an unnecessary call to ToList because we like iterating over our results. That's fine, there's only ten of them. Then we have our Where which takes a lambda, and in this lambda we use the "discard operator" as our variable- the _ variable is meant to be used as a nonsense variable whose value we don't care about.

In the Where, we discard any entries that have no NewsType, whose NewsType doesn't contain the NewsType we're displaying. We started with ten random articles we might want to show the users, and after the filtering, we're down to... some number? Ten or less, depending on how lucky we are.

Finally, we sort by publication date, take the first five, and throw in one more ToList for good measure. I mean, the method signature says we're returning a List, so we need that ToList, because we certainly couldn't return one of the many other more abstract enumerable types that C# supports.

The end result is we have a method that returns between 0 and 5 randomly selected news articles, all sharing the same NewsType and sorted by PublicationDate. We call this AllNews.

[Advertisement] Continuously monitor your servers for configuration changes, and report when there's configuration drift. Get started with Otter today!