HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

To LINQ or not to LINQ

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
linqnotstackoverflow

Problem

As a fairly new C# programmer I am a unsure about when and how to best make use of LINQ (and also when to choose the expression method syntax vs. the query syntax). This question often comes up for me at work, because I heavily lean toward making full use of new language features and a functional style, while my colleagues seem to prefer a more conservative style. This means I have to think a lot about when using LINQ is actually worth it.

Now here is where the review part comes in. This is an anonymized piece of code that this question came up for, once coded in a LINQ style and once in imperative style. Which version do you believe is preferable from a readability / maintainability viewpoint?

void updateItems()
{
    Items.Clear();
    Items.AddRange(Widget.GetDetectedWidgets()
        .Intersect(GetRelevantWidgets())
        .Select(widget => readSerialNr(widget))
        .Where(serial => serial != null)
        .DefaultIfEmpty("No relevant widgets found.")
        .ToArray());
}

void updateItems()
{
    Items.Clear();
    List relevantWidgets = GetRelevantWidgets().ToList();
    foreach(string widget in Widget.GetDetectedWidgets())
    {
        if(relevantWidgets.Contains(widget))
        {
            string serial = readSerialNr(widget);
            if(serial != null)
                Items.Add(serial);
        }
    }
    if(Items.Count == 0)
        Items.Add("No relevant widgets found.");
}

Solution

Some notes:

-
I think the LINQ version would be more readable if you extracted a variable for the collection. Something like:

var serials = /* the whole LINQ query here */

Items.Clear();
Items.AddRange(serials);


-
The LINQ version might be more efficient if there were lots of widgets, because Intersect() uses hashing to achieve O(n+m) time complexity, while your imperative code is O(n·m).

  • You don't need ToArray() in your LINQ version. (Assuming Items is List.)



  • I really don't think the string "No relevant widgets found." should be the default value if the list is empty. List of widget serial numbers should contain widget serial numbers, nothing else. And you should separate your business logic and presentation.



With the mentioned changes, I think the LINQ version will be more readable, primarily because Intersect() explains what you want to do better.

Also don't forget that you don't have to use only LINQ or only imperative code, you can combine the two in a single method (e.g. improve the imperative approach by using Intersect()).

Code Snippets

var serials = /* the whole LINQ query here */

Items.Clear();
Items.AddRange(serials);

Context

StackExchange Code Review Q#21359, answer score: 7

Revisions (0)

No revisions yet.