To LINQ or not to LINQ

Question

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<string> 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.");
}

Answer 1

Some notes:

1. 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);
   

2. 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).

3. You don't need ToArray() in your LINQ version. (Assuming Items is List<string>.)
4. 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()).

Answer 2

The greater issue here is not to LINQ or not to LINQ, but rather that of code smells - specifically with the lack of specialized classes and responsibility.

I would assume that the relevant widgets would already be a subset of the detected widgets, hence it would not be necessary to do an intersection at all.

I would definitely use a class instead than just a string to represent a widget. The responsibility of deriving the serial number should lay with the widget class and not with an external method. I would also abstract the responsibility of determining the relevant widgets to an external class and perhaps even the method to return the serial numbers of the relevant widgets.

I believe once you got your basic OOP down, it won't really matter what you use. As long as the code is simple and readable.

internal class Program
{
    private static void Main(string[] args) {
        var items = new List<string>();
        var widgetStore = new WidgetStore();

        var relevantWidgetSerialNumbers = widgetStore.GetRelevantWidgetSerialNumbers()
                                                     .ToArray();

        if (relevantWidgetSerialNumbers.Length == 0) {
            items.Add("No relevant widgets found.");
        } else {
            items.AddRange(relevantWidgetSerialNumbers);
        }
    }
}

public class Widget
{
    public string SerialNo {
        get {
            //Implement the logic here to derive the serial number or implement get; set; instead if it's predefined
            return null;
        }
    }
}

public class WidgetStore
{
    public IEnumerable<Widget> GetRelevantWidgets() {
        yield break;
    }

    public IEnumerable<string> GetRelevantWidgetSerialNumbers() {
        return GetRelevantWidgets().Select(widget => widget.SerialNo);
    }
}