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 usedk for key,e for element, andv for value. These would be poor names and would be prone to confusion for other developers.

In theGroupBy() call, I would name the lambda parameter eitheredgeLP (or whatever is more appropriate) or simplye as the parameter represents theedgeLP that you are grouping. TheToDictionary() call is operating onIGrouping<> objects so I tend to useg in that case.

• \$\begingroup\$Thanks @Jeff. This is perfect, and helps me understand LINQ better.\$\endgroup\$

  IamIC

  – IamIC

  2013-04-30 02:50:55 +00:00

  CommentedApr 30, 2013 at 2:50

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 toFirst() would hurt your performance. But I would understand if you wanted to fix that to make your code moreDRY. To do that, you could useSelect():

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 usingFirst() 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 useSingle() instead, or, in your case, simply useToDictionary() without the precedingGroupBy(). 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.

• \$\begingroup\$Thanks @svick. In this case, "LogicalProcs", "Cores", and "SMT" is identical for each instance (CPU.ID), so First() is fine. I did feel this is not ideal, but it seems there is no other way to do it.\$\endgroup\$

  IamIC

  – IamIC

  2013-04-30 02:55:50 +00:00

  CommentedApr 30, 2013 at 2:55

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

• \$\begingroup\$Although it certainly gets the job done, I'd say usingreturn within queries is a a bad approach. You're making the queries look less functional and more procedural. Using return statements within lambdas should be reserved for callbacks or other uses of the sort.\$\endgroup\$

  Jeff Mercado

  – Jeff Mercado

  2013-04-30 02:08:25 +00:00

  CommentedApr 30, 2013 at 2:08

• c#
• linq