LINQ Group query - simplifying

Question

Is there a way of writing this correctly working query more efficiently? I'm asking only to learn LINQ better:

    var cpuInfo = edgeLPs
        .GroupBy(k => k.CPU.ID, e => e.CPU)
        .ToDictionary(k => k.Key, v => new
        {
            LogicalProcs = v.First().LogicalProcessorsCount,
            Cores = v.First().Cores.Count,
            SMT = v.First().SMT
        });

Answer 1

I'd write it so you project your result within the grouping. Then when mapping to the dictionary, you can just take the first item in the group. That way, you can save yourself a few calls and have an overall cleaner looking query.

var cpuInfo = edgeLPs
    .GroupBy(e => e.CPU.ID, e => new
    {
        LogicalProcs = e.CPU.LogicalProcessorsCount,
        Cores = e.CPU.Cores.Count,
        SMT = e.CPU.SMT,
    })
    .ToDictionary(g => g.Key, g => g.First());

---

On a stylistic note, you should name your lambda parameters after the object they represent, not after what the entire lambda is supposed to represent.

Judging by the names you chose in the method calls, you used k for key, e for element, and v for value. These would be poor names and would be prone to confusion for other developers.

In the GroupBy() call, I would name the lambda parameter either edgeLP (or whatever is more appropriate) or simply e as the parameter represents the edgeLP that you are grouping. The ToDictionary() call is operating on IGrouping<> objects so I tend to use g in that case.

Answer 2

First, the usual disclaimer about performance: most of the time, you shouldn't worry much about it. If you find that your code is too slow, you should measure to find out which part is causing the slowdown and improve that. Trying to make code that's not performance critical more efficient is often a waste of your time.

In your specific case, I don't think the calls to First() would hurt your performance. But I would understand if you wanted to fix that to make your code more DRY. To do that, you could use Select():

var cpuInfo = edgeLPs
    .GroupBy(e => e.CPU.ID, e => e.CPU)
    .Select(g => new { id = g.Key, cpu = g.First() })
    .ToDictionary(g => g.id, g => new
    {
        LogicalProcs = g.cpu.LogicalProcessorsCount,
        Cores = g.cpu.Cores.Count,
        SMT = g.cpu.SMT
    });

Though the change from .First() to something like .cpu isn't actually much of an improvement.

Also, I think that using First() like this can be a sign of faulty logic. If you know that there will be always at most one CPU with a given ID, then you should use Single() instead, or, in your case, simply use ToDictionary() without the preceding GroupBy(). If there can be multiple CPUs with the same ID, i think you should figure out what to do with the rest of them, and not simply ignore all but the first one.

Answer 3

What are you looking for when you say 'efficient'? There are certainly ways to optimize the performance of the query, which likely exclude LINQ and depend on the nature of your input set.

But if you're just looking for a concise and readable way of expressing the logic, what you have is pretty good. I would modify it slightly to use a temp variable when creating the final dictionary value:

var cpuInfo = edgeLPs
    .GroupBy(k => k.CPU.ID, e => e.CPU)
    .ToDictionary(k => k.Key, v => 
    {
        var cpu = v.First();
        return new
        {
            LogicalProcs = cpu.LogicalProcessorsCount,
            Cores = cpu.Cores.Count,
            SMT = cpu.SMT
        };
    });