9
\$\begingroup\$

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();
    }
}
\$\endgroup\$
  • 6
    \$\begingroup\$ Use a hashset for O(1) lookup \$\endgroup\$ – paparazzo Mar 29 '18 at 15:39
  • \$\begingroup\$ If this code is to deal with addresses from non-English speaking countries (and even in UK, you have Welsh, Irish, Gaelic), replacing everything you consider invalid with a single symbol is not a great idea. For example, few years back I lived on street named Křížová, and once got a letter addressed to "K ov 17" -- kudos to the post office for deciphering that, but I wouldn't rely on that. A better approach would be just stripping the accents -- for example ICU provides methods to perform such transformations (and more). \$\endgroup\$ – Dan Mašek Mar 30 '18 at 16:12
9
\$\begingroup\$

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

\$\endgroup\$
  • 1
    \$\begingroup\$ I need to read like about twenty times your answers about unicode, characters and all the encoding stuff... currently I don't get anything ;-) \$\endgroup\$ – t3chb0t Mar 29 '18 at 15:59
  • \$\begingroup\$ @t3chb0t tnx, fixed! I know...it doesn't matter how hard I try...I won't ever learn how to explain things :( :( :( not even in my native language! All my respect and admiration to Teachers and Educators out there \$\endgroup\$ – Adriano Repetti Mar 29 '18 at 16:07
  • 2
    \$\begingroup\$ No! Sorry, I didn't mean they are badly explained! Not at all. I meant this topic appears to be so complex to me at the moment that it'll take several readings until it starts makings sense ;-) \$\endgroup\$ – t3chb0t Mar 29 '18 at 16:10
  • \$\begingroup\$ I didn't take it as an offence (well not even as a criticism...). That it's badly explained is what I was saying in my comment. I strongly think that if someone can't understand it at first glance then the problem is with the explanation. I think that the best explanation I saw has been written by tchrist (I don't know if he's active on CR but for sure in SO) in one blog post (or book? can't even remember) \$\endgroup\$ – Adriano Repetti Mar 29 '18 at 16:22
  • \$\begingroup\$ Oh, the answers are well written but they require some additional background knowledge like reading about what are code points, grapheme, or the many other links you posted here and there (which is a great help because it saves readers a lot of searching). You simply cannot include all that in an answer so after reading the other articles you read the main one again, but this time equipped with the new knowledge. \$\endgroup\$ – t3chb0t Mar 29 '18 at 16:33
6
\$\begingroup\$

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();
}
\$\endgroup\$
6
\$\begingroup\$

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"
);
\$\endgroup\$
  • \$\begingroup\$ I didn't even think about it! That's a great tip \$\endgroup\$ – Adriano Repetti Mar 29 '18 at 16:09
3
\$\begingroup\$

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);
}
\$\endgroup\$
  • \$\begingroup\$ The special characters might look better if left as string and escaped with Regex.Escape, this wouldn't require that many "\", I think ;-) \$\endgroup\$ – t3chb0t Mar 29 '18 at 19:23
  • \$\begingroup\$ @t3chb0t Unfortunately not all characters will be escaped. Tested this. For example ] and - won't be escaped. \$\endgroup\$ – Vadim Ovchinnikov Mar 29 '18 at 19:24
  • \$\begingroup\$ Oh, I guess I should have looked at the docs before writing it... I see it's not that useful here Escapes a minimal set of characters and this is just the half of the above character list... :-| \$\endgroup\$ – t3chb0t Mar 29 '18 at 19:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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