Filter out string sections

Question

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

Answer 1

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;
}

Answer 2

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.