更优雅的LINQ替代方案：不使用Foreach扩展

Question

这是为了提升我的技能。我的解决方案对于主要任务来说很好，但不够“简洁”。我目前正在开展一个.NET MVC with Entity framework项目。多年以来，我只知道基本的单数LINQ功能就足够了。现在我想学习如何使用高级的LINQ函数。

所以我有两个模型。

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
}

在我的一个视图模型中，我被要求为创建新用户时提供一个下拉列表以选择服务器。下拉列表使用文本和值ID作为IEnumerable进行填充。 以下是我原始的服务器下拉列表属性。

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

需求更新，现在我需要显示与每个服务器选择相关的用户数量。好的，没问题。以下是我脑海中想到的。

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;
    }
}

这会给我返回结果，比如说“localhost@rvrmt1u（8个用户）”，但仅此而已... 如果我想按用户数对这个下拉列表进行排序，该怎么办？ 我只需要在字符串中添加另一个变量。

简而言之...我相信某个地方的人可以教我如何将其转换为LINQ查询并使其更美观。另外，如果您知道我如何将列表按最多用户的服务器显示在首位，那就更好了。

- Cameron Trey Heilman

---

var items = Servers.Select(x => { var count = Users.Where(t => t.ServerId == x.Id).Count(); return new SelectListItem { Value = x.Id.ToString(), Text = $"{x.InstanceCode}@{x.ServerName} ({count} users on)" }); }).ToList(); 这段代码可能会让你走上正确的轨道。 - mjwills

你确定你想在Where之前调用ToList吗？你可能希望它在SQL端运行，而不是将整个表下载到内存中，然后进行过滤。你应该考虑完全删除ToList，因为它只会减慢速度而没有任何好处。 - Bradley Uffner

我希望你意识到自己有多幸运，能够得到像Eric下面这样出色的回应... - Jonathan

@BradleyUffner 你是正确的，而且那其实就是最初的写法。我匆忙中提出了问题，感谢你指正。 - Cameron Trey Heilman

Answer 1

好的，我们有这个混乱的东西：

    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;

进行一系列小心谨慎、显然正确的重构，逐步改进代码。

首先要做的是：将那些复杂的操作抽象成它们自己的方法。

请注意，我已经用有帮助的server替换了无用的x。

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

为什么Users上会有ToList方法？看起来不太对啊。

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

我们注意到有一个内置方法可以同时执行这两个操作：

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

同样地，创建一个项目也是如此:

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

现在我们的属性体是：

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

已经好多了。

如果你只是要传递一个lambda表达式，请不要将ForEach作为方法使用！语言中已经有更好的内置机制！当你可以简单地写foreach(var item in items) { ... }时，就没有理由写items.Foreach(item => {...});。这样做更简单、更易于理解和调试，并且编译器可以更好地进行优化。

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

好多了。

为什么在Servers上有一个ToList？完全没有必要！

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

变得更好了。我们可以消除不必要的变量。

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

嗯，这使我们了解到 CreateItem 可能正在进行计数。让我们重新编写它。

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

现在我们的属性主体是：

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

这应该很熟悉。我们重新发明了 Select 和 ToList：

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

现在我们注意到 lambda 可以被方法组替换：

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

我们把这整个混乱的过程简化为一行代码，清晰明了地展示出它所完成的功能。它做什么？它为每个服务器创建一个条目，并将它们放入列表中。代码应该像它所完成的功能那样读起来，而不是它是如何实现的。 仔细研究我在这里使用的技巧：

• 将复杂的代码提取到帮助方法中
• 用真正的循环替换ForEach
• 消除不必要的ToList
• 当您意识到可以进行改进时，请重新审视之前的决策
• 认识到何时在重新实现简单的帮助方法
• 不要止步于一个改进！每一个改进都使得另一个改进成为可能。

---

如果我想按用户计数对此下拉列表进行排序怎么办？

那就按用户计数排序！我们将其抽象为帮助方法，因此我们可以使用它：

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

我们现在注意到我们调用了 UserCount 两次。我们在意吗？也许。调用两次可能会导致性能问题，或者更糟糕的是，可能不是幂等的！如果有任何一个问题，那么我们需要撤销之前做出的决定。在理解模式下处理这种情况比流畅模式更容易，所以让我们将其重写为一个理解：

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

现在我们回到之前的内容：

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

现在我们可以说，

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

我们每台服务器只调用一次UserCount。

为什么要回到理解模式？因为在流畅模式下这样做会弄乱:

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

看起来有点丑。（在C# 7中，您可以使用元组而不是匿名类型，但思路是一样的。）

- Eric Lippert

---

我无法形容我有多么感激你以如此教导和帮助的方式纠正了我一些最糟糕的习惯。事实上，你回答了我一直有些害羞问的几个问题。我会采纳你的建议，好好学习这些技巧。谢谢！ - Cameron Trey Heilman

@CameronTreyHeilman：你的代码不是“错”的；正确的代码很棒。而且你有良好的品味，认识到它“不整洁”。但真正的挑战是让它变得优美。这不是一种学术练习；优美的代码更易读，更易更改，更易识别性能问题，更易解决它们。我经常问自己“是否有方法使这段代码更清晰地类似于它的意义，而不是它的工作方式？” - Eric Lippert

Answer 2

使用LINQ的技巧就是输入return，然后从那里开始。不要创建列表并将项目添加到其中；通常有一种方法可以一次性选择所有内容。

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
                )
            }
        );
    }
}

在这个例子中，实际上有两个 Select 语句——一个用于提取数据，另一个用于格式化。在理想情况下，这两个任务的逻辑应该分别分层处理，但这是一个可以接受的妥协方案。

- John Wu