5
\$\begingroup\$

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
        });
\$\endgroup\$
  • \$\begingroup\$ What is the type of edgeLPs? \$\endgroup\$ – svick Apr 29 '13 at 14:38
  • \$\begingroup\$ It's List<CustomType>. The type comprises a set of 5 other custom types, of which CPU and LogicalProcessor are two. \$\endgroup\$ – IamIC Apr 29 '13 at 16:06
  • \$\begingroup\$ My point with this question is that I'm pulling First() three times. This is the part that looks inefficient to me. \$\endgroup\$ – IamIC Apr 29 '13 at 16:07
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks @Jeff. This is perfect, and helps me understand LINQ better. \$\endgroup\$ – IamIC Apr 30 '13 at 2:50
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\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 Apr 30 '13 at 2:55
1
\$\begingroup\$

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
        };
    });
\$\endgroup\$
  • \$\begingroup\$ Although it certainly gets the job done, I'd say using return 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 Apr 30 '13 at 2:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.