4
\$\begingroup\$

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$".

\$\endgroup\$
2
  • \$\begingroup\$ Hi. Welcome to Code Review! Could you tell us more about what the function is supposed to do? As is, if we rewrite it, we have to analyze the code to determine if the new code does the same as the old code. Also, some changes may impact something that we think is irrelevant but you don't. Titles usually tell us what the code does. Does the code stitch Linq with if blocks? Or is that a description of how the code is written? While values could be something describable as "Linq", it seems more like a generic function that could have other purposes. \$\endgroup\$
    – mdfst13
    Apr 25, 2016 at 15:56
  • \$\begingroup\$ Thanks! I made an edit to answer your question. Hopefully I didn't create more confusion on what I'm doing. You're request is making me reveal code from 6 months ago to give you an indication of how bad it will be the further we go down this rabbit hole. :) \$\endgroup\$ Apr 25, 2016 at 17:12

2 Answers 2

2
\$\begingroup\$

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.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ I picked up the habit of putting commas at the beginning of new lines when I used Notepad++ Poor Man's TSql Formatter. :) I'm not a big fan of using IncludeBlanks in the Linq query. That's actually the way I did it before changing to the two Linq queries and I had a hard time coding it (I had to redo the logic a couple times to make it work right). Looking at some of the options for not using values several times did make me lean more towards keeping it how I have it now. I'm glad to see I'd at least get a sliding pass! \$\endgroup\$ Apr 25, 2016 at 17:28
  • \$\begingroup\$ Commas at the beginning isn't "proper" for c#, but when writing sql it is great since it makes it simple to exclude parts or the code. :) \$\endgroup\$ Apr 25, 2016 at 17:38
  • \$\begingroup\$ @MattiasÅslund That makes sense and I like that reasoning. \$\endgroup\$ Apr 26, 2016 at 13:17
1
\$\begingroup\$

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

\$\endgroup\$
6
  • \$\begingroup\$ You could use named arguments instead of having different methods. So your example could be written like this: Concatenate(listOfStrings, ",", "<", ">", excludeDuplicates: false, includeBlanks: true); \$\endgroup\$ Apr 25, 2016 at 17:11
  • \$\begingroup\$ @matthias Thanks for the answer! The changes you made would actually change the output of what I'm going for. I made an edit that included my inputs and outputs for the calling functions of Concatenate. However, I believe the fault for the way you changed it lies in how I named my parameters and takes away from the understanding of what I'm trying to do. I looked up IQueryable on MSDN and I'm having a hard time understanding how that would help me in this case. \$\endgroup\$ Apr 25, 2016 at 17:27
  • \$\begingroup\$ @Risky absolutely. It's about taste, but I think I prefer ConcatenateWithoutBlanks(values.Distinct()); \$\endgroup\$ Apr 25, 2016 at 17:32
  • \$\begingroup\$ @LockeGarmin IQueryable is just the interface Linq uses. It requires IEnumerable so if you "think" the Linq expression is IEnumerable that is pretty ok. I'm just nitpicky in this case I guess? \$\endgroup\$ Apr 25, 2016 at 17:35
  • \$\begingroup\$ What's the purpose of AsQueryable? \$\endgroup\$ Apr 25, 2016 at 17:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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