2
\$\begingroup\$

I have a helper method to clean up a string and remove pieces that I will be generating again. (defined by the "SpecialKey" Constants) but I've never been real happy with the code. Although I find it easy to read adding more keys doesn't always seem so clean.

    protected static System.String[] GetStringParts(System.String stringValue)
    {
        System.String[] stringParts = stringValue.Split('!');

        System.Func<System.String, System.Boolean> isConstKey = (System.String key) =>
        {
            return !System.String.IsNullOrWhiteSpace(key) && (
                    key.StartsWith(MyUtility.SpecialStringKey1) ||
                    key.StartsWith(MyUtility.SpecialStringKey2) ||
                    key.StartsWith(MyUtility.SpecialStringKey3) ||
                    key.StartsWith(MyUtility.SpecialStringKey4)
                );
        };
        return stringParts.Where(key => !isConstKey(key)).ToArray();
    }

I have removed specific names and such. I don't usually go by the convention SpecialKey1/2 etc

I'd like any pointers on performance or readability. This code gets hit often but in the last two years has only been changed once (maybe twice).

\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

I have a couple of suggestions and observations I'd like to point out:

  • Fully qualified System types reduce readability without adding information.
    So: use string[] instead of System.String[], bool instead of System.Boolean and so on.

  • Defining a lambda statement and storing it in a variable overcomplicates the code. Use query expression syntax instead.

  • Are you actually using the return value as a string[] (accessing by index, etc.) or could you get away with returning IEnumerable<string>? (Eric Lippert explains why you should.)

  • Your isConstKey looks wrong, but it's hard to tell because of the double negative. Are you trying to remove empty strings? Currently, you're leaving the white space in the result, not removing it. In below code sample, I've corrected the problem.

  • Instead of comparing all the SpecialKeyStrings seperately, put them into a sequence exposed as a property of MyUtility and use LINQ to validate against that:

      public static IEnumerable<string> SpecialStringKeys
      {
          get
          {
              return new[] { SpecialStringKey1, SpecialStringKey2, SpecialStringKey3, 
                             SpecialStringKey4, };
          }
      }
    

Edit: other possibilities would be to avoid creating a new array every time; you could create a private static readonly ReadOnlyCollection<string> field or just a private static readonly string[].

All that would allow your GetStringParts method to be changed to the following:

protected static IEnumerable<string> GetStringParts(string stringValue)
{
    return from part in stringValue.Split('!')
           where !string.IsNullOrWhiteSpace(part) && 
                 !MyUtility.SpecialStringKeys.Any(part.StartsWith)
           select part;
}

(If you really need an array, add the ToArray call and change the return type back).

If you want to go one step further in readability, you could extract the condition to an extension method:

public static bool IsNeitherKeyNorWhiteSpace(this string source)
{
    return !string.IsNullOrWhiteSpace(source) && 
           !MyUtility.SpecialStringKeys.Any(source.StartsWith);
}

Changing your original method thus:

protected static IEnumerable<string> GetStringParts(string stringValue)
{
    return from part in stringValue.Split('!')
           where part.IsNeitherKeyNorWhiteSpace()
           select part;
}
\$\endgroup\$
3
  • \$\begingroup\$ +1 System.String vs string is for some of our "legacy" developers... unfortunately I can't change that. Other code Relies on it being a String[]. These are both things I'd normally do in personal projects. The double negative I believe was due to some re-factoring earlier but I see what you mean now. The only bit I disagree with is the lambda, as having it in the one statement previously seemed like the return .... was too long to read. .Any(source.StartsWith) is not something I knew was even ossible. \$\endgroup\$ Aug 23, 2012 at 2:32
  • 1
    \$\begingroup\$ thanks for the explanation (I had a feeling that System.String was probably not your choice). .Any(source.StartsWith) is called a method group, being equivalent to .Any(key => source.StartsWith(key)). As for the lambda, I wasn't suggesting you should have inlined it; my point was that using query syntax instead of method syntax would make your code more readable (regardless of whether you check the special keys seperately or as an array). \$\endgroup\$
    – Adam
    Aug 23, 2012 at 3:31
  • \$\begingroup\$ Thanks that makes more sense now. I should point out a minor detail is that I don't actually care here if it is null. It was there just as defensive code and in its current form would have allowed null/whitespace through. But that is a tiny detail. \$\endgroup\$ Aug 23, 2012 at 4:29
2
\$\begingroup\$

I assume that the part that you want to improve is the key.StartsWith....

I would replace the anonymous function with an Extension method String.StartsWith(IEnumerable<string>) -- then create a list of special keys (perhaps even load them from persisted storage, depending upon your usage), to be used in the extension method.

Basically you add the over head of list creation, but your code now more precisely reflects the INTENT, making it easier to understand and modify. As middle ground between using persisted storage and creating the list in the middle of the your function, you could create a static function that returns your specialKeysList.

As for performance, you should always keep performance in mind, but don't worry about it unless you have to -- you should only worry about performance when you have to ask "how can I improve the performance when doing this". At that point you have an identified problem, and you need a solution. If you don't have a problem, then don't do things that you know will cause a problem, but don't spend time trying to make it faster, that time is better spent either writing more code or making it more readable.

\$\endgroup\$
3
  • \$\begingroup\$ The Keys are static const string so they can be used elsewhere. Is creating a list really worth it? \$\endgroup\$ Aug 23, 2012 at 2:35
  • 1
    \$\begingroup\$ Absolutely. What you now have is a random number of strings, defined by unrelated const values, with at best a shared naming convention. Their relationship in this function is defined by their usage. Imagine that you have a new hire, and he's told to work on this on the first day. Are those all of the special keys? Are there more special keys that exist but aren't relvant to this process? Does the naming convention define what should be checked, or is that just a concidence. Looking at this line of code in isolation, you can't know any of that. \$\endgroup\$
    – jmoreno
    Aug 23, 2012 at 2:45
  • \$\begingroup\$ Do you have special keys in there that shouldn't be? You really don't know what those strings represent when looking at this code. If OTOH, if you split out the strings into a function that returns a private static list, when you look at this code, it will be quite clear what is happening. Your performance isn't impacted, because the list is, only created once, and then returned, but when used, it is clear that it is "the list of strings that are to be used for X" \$\endgroup\$
    – jmoreno
    Aug 23, 2012 at 2:57

Your Answer

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

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