Stitching Linq with if blocks

Question

I recently learned about Linq's deferred execution and a little clearer understanding of IEnumerable. I came back to some code that I had written and decided to change with Linq extension methods. (The method is a helper function for some custom Excel functions that I use.)

Overall the code kind of feels wrong readability-wise having to tack on values = values.exension like that. When I step through the debugger it seems to be behaving like I expect.

Is there a more readable way to express this function with or without linq extension methods? I fear I'm making things less readable and possibly micro-optimizing.

private static string Concatenate(IEnumerable<string> values
    , string between, string begin, string end
    , bool excludeDuplicates, bool includeBlanks)
{
    if (!includeBlanks)
        values = values.Where(v => v != string.Empty);

    if (excludeDuplicates)
        values = values.Distinct();

    values = values.Select(v => begin + v + end);

    return string.Join(between, values);            
}

Here is what I previously used in case it is of interest.

private static string Concatenate(IEnumerable<string> Values, string BetweenString
    , string BeginString, string EndString, bool ExcludeDuplicates, bool IncludeBlanks)
{
    IEnumerable<string> IncludedValues = (
        from Value in Values
        where IncludeBlanks || Value.Length > 0
        select BeginString + Value + EndString
        );

    IEnumerable<string> StringValues;

    if (ExcludeDuplicates)
        StringValues = new HashSet<string>(IncludedValues);
    else
        StringValues = new List<string>(IncludedValues);

    return string.Join(BetweenString, StringValues);
}

EDIT:

To give a little more context for this function (I'll hedge a little here and say that I haven't changed these functions to be more readable yet, so I beg forgiveness now. :) )

The purpose of this function is that it gets used by two other functions RA.VCONCATENATE and RA.HCONCATENATE that will be Excel functions (using ExcelDNA). (I do plan on changing this to be more readable as well and make Values IEnumerable instead of IList). Note: I can't have optional "missing" parameters in Excel or overloading so I made extension methods for string, int, double and bool that convert unspecified parameters to a default.

[ExcelFunction(Category = "Extra Range Functions", Name = "RA.VCONCATENATE", 
    Description = "Vertically Concatenates all values in the Range starting from Top to Bottom, then Left to Right.")]
public static string VerticalConcatenate(object[,] Range, object BetweenString, object BeginString, object EndString, object ExcludeDuplicates, object IncludeBlanks)
{

    List<string> Values = RangeValuesToList(Range, RangeEnumerateFirstDirection.Down); //Empty cells replaced with String.Empty
    string sBetweenString = BetweenString.Optional("");
    string sBeginString = BeginString.Optional("");
    string sEndString = EndString.Optional("");
    bool bExcludeDuplicates = ExcludeDuplicates.Optional(false);
    bool bIncludeBlanks = IncludeBlanks.Optional(false);

    return Concatenate(Values, sBetweenString, sBeginString, sEndString, bExcludeDuplicates, bIncludeBlanks);
}

RA.HCONCATENATE is the same except is uses RangeEnumerateFirstDirection.Right. The expected output of =RA.VCONCATENATE(A1:A5, ",", "^", "$", TRUE, FALSE) (if Range [A1:B3] values are { 1, 2, 2 }, {3, EMPTY, EMPTY} ) is: "^1$,^2$,^3$". RA.HCONCATENATE output is: "^1$,^3$,^2$".

Answer 1

It's not bad already (save for the weird formatting of parameters, what's with commas at the beginning of the line? : ) But that's a matter of taste, and beyond the point).

You could return to the trick you used in your previous implementation to avoid breaking the chain and get rid of value reassignments by pushing includeBlanks into the predicate, like so:

values = values.Where(v => includeBlanks || v != string.Empty);

We've cut on verbosity, but it's debatable to me whether it improves readability.

It's harder to achieve the same thing for Distinct, although you could pull it through if you added a helper extension:

private static IEnumerable<T> If(this IEnumerable<T> enumerable, bool condition, IEnumerable<T> ifMet)
{
    return condition ? ifMet : enumerable;
}

Then you could turn all of it into a single statement:

return String.Join(
    between,
    values
        .Where(v => includeBlanks || v != string.Empty)
        .If(excludeDuplicates, values.Distinct())
        .Select(v => begin + v + end); 

Is it worth it, or over-engineered? This is subjective and a matter of taste. Some find verbosity more readable, others prefer terseness/expressiveness.

I would let both solutions slide in a peer review, although creating a single-use extension method (If) looks like a minor code smell.

Another way to avoid repetitive overwriting of values, which you're uncomfortable with, would be to introduce temporary variables:

var blanksHandled = includeBlanks ? values : values.Where(v => v != String.Empty);
var duplicatesHandled = excludeDuplicates ? blanksHandled.Distinct() : blanksHandled;
var withAffixes = duplicatesHandled.Select(v => begin + v + end);

We're not overwriting values now, but when I look at it, I feel it sacrificed too much readability just for that, and what's worse, it got error-prone in process: you now have to remember to always use the last variable so as not to skip a step.

If another parameter was to be added, there's a risk of a bug slipping in:

var blanksHandled = includeBlanks ? values : values.Where(v => v != String.Empty);
var duplicatesHandled = excludeDuplicates ? blanksHandled.Distinct() : blanksHandled;
// oops! will everyone spot right away what went wrong here?
var dirtyWordsHandled = safeMode ? blanksHandled.Except(dirtyWords) : blanksHandled;
var withAffixes = duplicatesHandled.Select(v => begin + v + end);

The last trick I can think of would be to convert these conditionals into Func<IEnumerable<T>, IEnumerable<T>> instances, and pass them to the LINQ chain.

I haven't got a C# IDE on this computer, so this code could be syntactically incorrect, but you get the idea:

Func<IEnumerable<String>, IEnumerable<String>> handleDuplicates = (entries) => excludeDuplicates ? entries.Distinct() : entries;
// etc.

And use them as selectors:

values
   .Select(handleBlanks)
   .Select(handleDuplicates)
   // ...

Again, is it worth it? Sometimes the clever thing is not to be too clever, and keeping it simple beats trying to make it pretty.

Answer 2

Code is all about readability and maintainability.

As such, your code should adhere to a standard and be broken up into small readable parts. A mega-monstrous Linq-statement that does it all is fun code-golf but terrible code!

I would probably rewrite your method as

private static string Concatenate(IEnumerable<string> values, string betweenString
, string beginString, string endString, bool excludeDuplicates, bool includeBlanks)
{
    var q = values.AsQueryable();

    if (!IncludeBlanks)
        q = q.Where(s => s.Any());

    if (excludeDuplicates)
        q = q.Distinct();

    if (!q.Any())
        return "";

    var separator = endString + betweenString + beginString;
    var joinedValues = beginString + string.Join(separator, q) + endString;
    return joinedValues;
}

Then again, I would consider if I didn't want different methods for implementing the different types of concatenations, since readability of

Concatenate(listOfStrings, ",", "<", ">", false, true);

isn't great...