Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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.");
}
share|improve this question
    
Tomato solution #1 (s1) - tomatoes solution #2 (s2). S1 results in cleaner code and easier to spot logical errors in since it contains less "manual code". However, say that any of the functions you call (GetDetectedWidgets, GetRelevantWidgets, readSerialNr) contains some logic errors, then I would be much happier with debugging that in s2. Also s1 requires LINQ knowledge whereas s2 could be read by anyone not familiar with LINQ. If LINQ is important to your company and for everyone who is or ever will be employed, then go for s1, otherwise stick with s2 :) –  Daniel MesSer Feb 6 '13 at 1:09
    
Go for S1, and use unit-tests to make sure that those functions you are calling do not themselves contain logic errors. And for what it's worth, I have no knowledge of LINQ and for me the first example is just about as readable as the second. –  aroth Feb 6 '13 at 1:14
1  
BTW, are you really calling 6 years old features “new”? :-) –  svick Feb 6 '13 at 1:23
    
@svick you'd be surprised how many people are resistant to LINQ for whatever reason. It's goofy, sure, but there are some people who are just happy with iterative patterns. –  Reacher Gilt Feb 6 '13 at 16:30
add comment

2 Answers

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

share|improve this answer
    
Point 1 is the only one I'm not sure what you mean: Assign the intersection to a new variable? On 2: Efficiency is a non-issue here. 3: I do need ToArray() in this particular case, since Items is a ComboBox.ObjectCollection and takes an object[] as parameter to AddRange. 4: I agree that the actual logic of getting the port list should have been a separate method, but adding the default text here is consistent with the old behaviour of this UI component. –  Medo42 Feb 6 '13 at 2:13
    
@Medo42 Re 1.: I clarified that. –  svick Feb 6 '13 at 8:16
add comment

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);
    }
}
share|improve this answer
2  
Thank you for taking the time to respond, but this does not relate to my question. Besides, your advice might apply in some cases, but the question simply doesn't contain the details you would need to decide what a good object structure would be in my case, or whether a certain operation is necessary or not. –  Medo42 Feb 10 '13 at 22:37
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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