To Linq or not to Linq - which version is prettier?

Question

I just wrote the following function:

public string Ebnf {
    get {
        var props = GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance);
        var ruleProps = from p in props where p.PropertyType.IsSubclassOf(typeof(ARule)) select p;
        var rules = from p in ruleProps select (ARule)p.GetValue(this, null);
        var ebnfs = from r in rules select r.Name + " = " + r.Ebnf + ".";
        return string.Join("\n", ebnfs.ToArray());
    }
}

I started wondering if Linq actually saved me space, or whether I was using Linq just for the sake of it:

public string EbnfNonLinq {
    get {
        var ebnfs = new List<string>();
        var props = GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance);
        foreach (var p in props) {
            if (p.PropertyType.IsSubclassOf(typeof(ARule))) {
                var r = (ARule)p.GetValue(this, null);
                ebnfs.Add(r.Name + " = " + r.Ebnf + ".");
            }
        }
        return string.Join("\n", ebnfs.ToArray());
    }
}

7 lines of code vs. 5: it's a savings. But I wonder if the density of the first function is too much. (This is not performance critical code, so I am not concerned about that.)

Which do you think is prettier, more maintainable, comprehensible, better?

Answer 1

I find the non-linq version slightly more readable.

Answer 2

Fluffy syntax is fluffy. Terse syntax is terse.

public string Ebnf
{
get {
    var props = GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance);
    string[] ebnfs = props
      .Where(prop => prop.PropertyType.IsSubclassOf(typeof(ARule)))
      .Select(prop => (ARule)prop.GetValue(this, null))
      .Select(rule => rule.Name + " = " + rule.Ebnf + ".")
      .ToArray();
    return string.Join("\n", ebnfs);
}
}

Answer 3

I like the first one a bit better. I think the linq is prettier, but would probably make it the prettieriest by getting entire name in one shot. Something like this, if possible:

var ebnfs = from p in props 
            where p.PropertyType.IsSubclassOf(typeof(ARule)) 
            let rule = (ARule)p.GetValue(this, null)
            select rule.Name + " = " + rule.Ebnf + ".";

 return String.Join(",", ebnfs.ToArray());

^^ warning, untested :)

I don't like using .Where or .Select much, so I stick to naked linq and really just use those when it is the only thing I am doing to a collection.

As the commenter noted, this does make debugging a bit harder in some cases. However, if it is unlikely that there is something odd going on in rule.Name/Ebnf, then that probably won't be a big problem.

Answer 4

I like the LINQ version because it makes it clear what you are trying to acomplish. Each line is dependent on only the line before it and nothing else.

One comment though, you may want to execute them line by line. Right now each of your varaibles is a query that won't be executed until the last line. That means tracking down bugs may be harder than necessary.

The change I am proposing is to simply add ToList after each query. This executes it immedately.

    var props = GetType().GetProperties(BindingFlags.Public | BindingFlags.Instance);
    var ruleProps = (from p in props where p.PropertyType.IsSubclassOf(typeof(ARule)) select p).ToList();
    var rules = (from p in ruleProps select (ARule)p.GetValue(this, null)).ToList();
    var ebnfs = (from r in rules select r.Name + " = " + r.Ebnf + ".").ToList();
    return string.Join("\n", ebnfs.ToArray());

Pros: Easier debugging. You can reuse the results without rerunning the query. Cons: If you are using where clauses, sticking to raw queries can sometimes be faster.