More Elegant LINQ Alternative to Foreach Extension

Question

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.

Answer 1

The trick with LINQ is just to type return and go from there. Don't create a list and add items to it; there is usually a way to select it all in one go.

public IEnumerable<SelectListItem> ServerItems
{
    get 
    {
        return Servers.Select
        (
            server => 
            new 
            {
                Server = server,
                UserCount = Users.Count( u => u.ServerId = server.Id )
            }
        )
        .Select
        (
            item =>
            new SelectListItem
            {
                Value = item.Server.Id.ToString(),
                Text = string.Format
                (
                    @"{0}{1} ({2} users on)" ,
                    item.Server.InstanceCode,
                    item.Server.ServerName, 
                    item.UserCount
                )
            }
        );
    }
}

In this example there are actually two Select statements-- one to extract the data, and one to do the formatting. In an ideal situation the logic for those two tasks would be separated into different layers, but this is an OK compromise.

Answer 2

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.)