Stitching Linq with if blocks

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 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 Values, string BetweenString
    , string BeginString, string EndString, bool ExcludeDuplicates, bool IncludeBlanks)
{
    IEnumerable IncludedValues = (
        from Value in Values
        where IncludeBlanks || Value.Length > 0
        select BeginString + Value + EndString
        );

    IEnumerable StringValues;

    if (ExcludeDuplicates)
        StringValues = new HashSet(IncludedValues);
    else
        StringValues = new List(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)

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 If(this IEnumerable enumerable, bool condition, IEnumerable 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> 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> 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.

StackExchange Code Review Q#126654, answer score: 2