More Elegant LINQ Alternative to Foreach Extension

  • A+
Category:Languages

This is purely to improve my skill. My solution works fine for the primary task, but it's not "neat". I'm currently working on a .NET MVC with Entity framework project. I know only basic singular LINQ functions which have sufficed over the years. Now I'd like to learn how to fancy.

So I have two models

public class Server {     [Key]     public int Id { get; set; }     public string InstanceCode { get; set; }     public string ServerName { get; set; } }  public class Users {     [Key]     public int Id { get; set; }     public string Name { get; set; }     public int ServerId { get; set; } //foreign key relationship } 

In one of my view models I was asked to provide a dropdown list for selecting a server when creating a new user. The drop down list populated with text and value Id as an IEnumerable Here's my original property for dropdown list of servers

public IEnumerable<SelectListItem> ServerItems {     get { Servers.ToList().Select(s => new selectListItem { Value = x.Id.ToString(), Text = $"{s.InstanceCode}@{s.ServerName}" }); } } 

Update on requirements, now I need to display how many users are related to each server selection. Ok no problem. Here's what I wrote off the top of my head.

public IEnumerable<SelectListItem> ServerItems {     get      {         var items = new List<SelectListItem>();         Servers.ToList().ForEach(x => {             var count = Users.ToList().Where(t => t.ServerId == x.Id).Count();             items.Add(new SelectListItem { Value = x.Id.ToString(), Text = $"{x.InstanceCode}@{x.ServerName} ({count} users on)" });         });          return items;     } } 

This gets my result lets say "localhost@rvrmt1u (8 Users)" but thats it.. What if I want to sort this dropdown list by user count. All I'm doing is another variable in the string.

TLDR ... I'm sure that someone somewhere can teach me a thing or two about converting this to a LINQ Query and making it look nicer. Also bonus points for knowing how I could sort the list to show servers with the most users on it first.

 


OK, we have this mess:

    var items = new List<SelectListItem>();     Servers.ToList().ForEach(x => {         var count = Users.ToList().Where(t => t.ServerId == x.Id).Count();         items.Add(new SelectListItem { Value = x.Id.ToString(), Text = $"{x.InstanceCode}@{x.ServerName} ({count} users on)" });     });     return items; 

Make a series of small, careful, obviously-correct refactorings that gradually improve the code.

Start with: Let's abstract those complicated operations to their own methods.

Note that I've replaced the unhelpful x with the helpful server.

int UserCount(Server server) =>    Users.ToList().Where(t => t.ServerId == server.Id).Count(); 

Why on earth is there a ToList on Users? That looks wrong.

int UserCount(Server server) =>    Users.Where(t => t.ServerId == server.Id).Count(); 

We notice that there is a built-in method that does these two operations together:

int UserCount(Server server) =>    Users.Count(t => t.ServerId == server.Id); 

And similarly for creating an item:

SelectListItem CreateItem(Server server, int count) =>    new SelectListItem    {      Value = server.Id.ToString(),      Text = $"{server.InstanceCode}@{server.ServerName} ({count} users on)"    }; 

And now our property body is:

    var items = new List<SelectListItem>();     Servers.ToList().ForEach(server =>      {         var count = UserCount(server);         items.Add(CreateItem(server, count);     });     return items; 

Already much nicer.

Never use ForEach as a method if you're just going to pass a lambda body! There's already a built-in mechanism in the language that does it better! There is no reason to write items.Foreach(item => {...}); when you could simply write foreach(var item in items) { ... }. It's simpler and easier to understand and debug, and the compiler can optimize it better.

    var items = new List<SelectListItem>();     foreach (var server in Servers.ToList())     {         var count = UserCount(server);         items.Add(CreateItem(server, count);     }     return items; 

Much nicer.

Why is there a ToList on Servers? Completely unnecessary!

    var items = new List<SelectListItem>();     foreach(var server in Servers)     {         var count = UserCount(server);         items.Add(CreateItem(server, count);     }     return items; 

Getting better. We can eliminate the unnecessary variable.

    var items = new List<SelectListItem>();     foreach(var server in Servers)         items.Add(CreateItem(server, UserCount(server));     return items; 

Hmm. This gives us an insight that CreateItem could be doing the count itself. Let's rewrite it.

SelectListItem CreateItem(Server server) =>    new SelectListItem    {      Value = server.Id.ToString(),      Text = $"{server.InstanceCode}@{server.ServerName} ({UserCount(server)} users on)"    }; 

Now our prop body is

    var items = new List<SelectListItem>();     foreach(var server in Servers)         items.Add(CreateItem(server);     return items; 

And this should look familiar. We have re-invented Select and ToList:

var items = Servers.Select(server => CreateItem(server)).ToList(); 

Now we notice that the lambda can be replaced with the method group:

var items = Servers.Select(CreateItem).ToList(); 

And we have reduced that whole mess to a single line that clearly and unambiguously looks like what it does. What does it do? It creates an item for every server and puts them in a list. The code should read like what it does, not how it does it.

Study the techniques I used here carefully.

  • Extract complex code to helper methods
  • Replace ForEach with real loops
  • Eliminate unnecessary ToLists
  • Revisit earlier decisions when you realize there's an improvement to be made
  • Recognize when you are re-implementing simple helper methods
  • Don't stop with one improvement! Each improvement makes it possible to do another.

What if I want to sort this dropdown list by user count?

Then sort it by user count! We abstracted that away into a helper method, so we can use it:

var items = Servers   .OrderBy(UserCount)   .Select(CreateItem)   .ToList(); 

We now notice that we're calling UserCount twice. Do we care? Maybe. It could be a perf problem to call it twice, or, horrors, it might not be idempotent! If either are a problem then we need to undo a decision we made before. It's easier to deal with this situation in comprehension mode rather than fluent mode, so let's rewrite as a comprehension:

var query = from server in Servers             orderby UserCount(server)             select CreateItem(server); var items = query.ToList(); 

Now we go back to our earlier:

SelectListItem CreateItem(Server server, int count) => ... 

and now we can say

var query = from server in Servers             let count = UserCount(server)             orderby count             select CreateItem(server, count); var items = query.ToList(); 

and we are only calling UserCount once per server.

Why go back to comprehension mode? Because to do this in fluent mode makes a mess:

var query = Servers   .Select(server => new { server, count = UserCount(server) })   .OrderBy(pair => pair.count)   .Select(pair => CreateItem(pair.server, pair.count))   .ToList(); 

And it looks a little ugly. (In C# 7 you could use a tuple instead of an anonymous type, but the idea is the same.)

Comment

:?: :razz: :sad: :evil: :!: :smile: :oops: :grin: :eek: :shock: :???: :cool: :lol: :mad: :twisted: :roll: :wink: :idea: :arrow: :neutral: :cry: :mrgreen: