Cleaning address line data sent to couriers API

Question

I've developed a client library to integrate with a couriers shipping API. The courier only accepts a limited set of characters for addresses, so I've created a helper class to strip out invalid characters and optionally replace them (with for example a ?).

public static class StringCleaner
{
    private static readonly char[] ValidCharacters =
    {
        ' ', '#', '&', '\'', '(', ')', '+', ',', '-', '.', '/',
        '0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
        ':', '?', '@',
        'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N',
        'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
        '[', ']', '_', '`',
        'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n',
        'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z',
        '{', '|', '}', '~',
    };

    public static string CleanString(string stringToClean, string replacement = "")
    {
        int length = stringToClean.Length;
        var newString = new StringBuilder();
        for (var i = 0; i < length; i++)
        {
            if (ValidCharacters.Contains(stringToClean[i]))
            {
                newString.Append(stringToClean[i]);
            }
            else
            {
                newString.Append(replacement);
            }
        }

        return newString.ToString();
    }
}

Answer 1

First I'd think about names. Isn't Clean() enough for a method inside a class named StringCleaner? Do you need to repeat String?

Still in the prototype you should consider that you're iterating char but replacement is a string. It might be (or not) something to consider, it depends on your requirements.

More about char and string: you're iterating char (which is an UTF-16 code unit). UTF-16 is not fixed size then you're breaking for code points which are not encoded as a single code unit. More about Unicode: not every character is a single code point: you're breaking for surrogates. String might be (or not) normalized, do you need to take care about it? One grapheme may be represented using two or more code points: think about, for example, ộ Vietnamese character. You may read How can I perform a Unicode aware character by character comparison?

For now let's assume that to iterate char is perfectly fine. If not then read an almost ready to use example in Split a string into chunks of the same length.

In .NET strings are immutable then reading String.Length in a loop is perfectly fine and compiler is capable to optimize it. You already know the maximum length of the string then you may preallocate the StringBuilder buffer to the maximum capacity:

var newString = new StringBuilder(stringToClean.Length);

foreach will help to make code shorter and easier to follow:

foreach (var c in stringToClean)
{
    if (ValidCharacters.Contains(c))
    {
        newString.Append(c);
    }
    else
    {
        newString.Append(replacement);
    }
}

If replacement can be a char it's even easier:

foreach (var c in stringToClean)
    newString.Append(ValidCharacters.Contains(c) ? c : replacement);

You may want to see a LINQ version (in case you find it more readable):

new String(stringToClean.Select(c => ValidCharacters.Contains(c) ? c : replacement));

Performance

In general you should be careful with parameters with a default value, in this case also because of performance. If you add an overload for the common case then you can write:

public static string CleanString(string stringToClean)
    =>  new String(stringToClean.Where(ValidCharacters.Contains(c)));

Searching each character inside the array is something terribly slow. First approach: HashSet<char>. Build the list and search inside the hashed collection. It's WAY faster.

We're talking about true/false values and all valid characters are inside the 7 bit ASCII range...if performance really matters you may use a BitVector. Unfortunately default implementation throws ArgumentOutOfRangeException if you ask for a value outside the assigned range; if you do not want to create a vector of 2^16/8 bytes then you may write your own implementation (see for example Count items existing in 2 Lists for a quick and dirty comparison).

Answer 2

Use hashset for O(1)
String will enumerate characters

private static readonly HashSet<char> ValidCharacters = new HashSet<char> 
{   ' ', '#', '&', '\'', '(', ')', '+', ',', '-', '.', '/', 
    '0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
    ':', '?', '@',
    'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N',
    'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
    '[', ']', '_', '`',
    'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n',
    'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z',
    '{', '|', '}', '~',
};

public static string CleanString(string stringToClean, string replacement = "")
{
    var newString = new StringBuilder();
    foreach (char c in stringToClean)
    {
        if (ValidCharacters.Contains(c))
        {
            newString.Append(c);
        }
        else
        {
            newString.Append(replacement);
        }
    }
    return newString.ToString();
}

Answer 3

I cannot teach you anything about encoding, like @Adriano Repetti did in his great answers but maybe I can save you from more typing...

You don't have to create a char[] yourself, strings are already arrays so let the compiler concatenate them and initialize the collection from a string:

new HashSet<char>(
    " #&" + 
    "01234"
);

Answer 4

I'd use Regex for this.

public static class StringCleaner
{
    private const Pattern = @"[^a-zA-Z0-9 \#\\\'\(\)\+\,\-\.\/\[\]_`\{\|\}\~\:\?@]";

    public static string Clean(string address, string replacement = "") =>
        Regex.Replace(address, Pattern, replacement);
}