20
\$\begingroup\$
void removeForbiddenChar(string* s)
{
    string::iterator it;

    for (it = s->begin() ; it < s->end() ; ++it){
        switch(*it){
        case '/':case '\\':case ':':case '?':case '"':case '<':case '>':case '|':
            *it = ' ';
        }
    }
}

I used this function to remove a string that has any of the following character: \, /, :, ?, ", <, >, |. This is for a file's name. This program runs fine. It simply change a character of the string to a blank when the respective character is the forbidden character. However, I have a feeling against this use of switch statement. I simply exploit the case syntax here, but this, somehow nags me. I just don't like it. Anybody else got a better suggestion of a better implementation in this case?

\$\endgroup\$
  • \$\begingroup\$ If a char isn't forbidden, then we leave it be. \$\endgroup\$ – Karl Jan 27 '11 at 17:22
  • 1
    \$\begingroup\$ Name it replaceForbiddenChars, as it replaces instead of removes and handles multiple characters. \$\endgroup\$ – Fred Nurk Jan 27 '11 at 17:30
  • \$\begingroup\$ @Fred: If none of the cases in a switch-statement match, the control flow continues after the switch statement. That behavior is perfectly defined. \$\endgroup\$ – sepp2k Jan 27 '11 at 17:33
  • \$\begingroup\$ @sepp2k: Thanks, it is well-defined. I'm not sure why I thought that, but I'll blame it on articles I've been reading about (micro-)optimizing. \$\endgroup\$ – Fred Nurk Jan 27 '11 at 17:37
  • \$\begingroup\$ Seems you forgot * symbol (asterisk). It is forbidden also. \$\endgroup\$ – user4124 May 6 '11 at 23:28
21
\$\begingroup\$

Declare a string containing the illegal characters: "\\/:?"<>|". All you need to do is check if the char is in the array, so use a native function for that, or write a method CharInString(char* needle, string* haystack) which loops through the contents of the provided haystack to check if the needle is inside it.

Your loop should end up looking like this:

string illegalChars = "\\/:?\"<>|"
for (it = s->begin() ; it < s->end() ; ++it){
    bool found = illegalChars.find(*it) != string::npos;
    if(found){
        *it = ' ';
    }
}

It's more maintainable and readable. You can tell if you've duplicated a character quite easily and since you can do it with any target string and any string of illegalChars you've just created for yourself a generic RemoveIllegalChars(string* targetString, string* illegalChars) method usable anywhere in your program.

I may be using those pointers wrong. My C++fu is weak... for now.

\$\endgroup\$
  • \$\begingroup\$ +1, I was also going to recommend using a string to store the forbidden characters. I would add that this change makes it very easy to add the forbidden characters as a parameter to the removeForbiddenChars function, so that if the need should ever arise, it can be used in situations where different sets of characters are forbidden. Also you can use the find method to find out whether a character is in a string, so you don't necessarily need to write a CharInString function (or you could write is a simple wrapper around find). \$\endgroup\$ – sepp2k Jan 27 '11 at 16:24
  • \$\begingroup\$ @sepp2k: We seem to be on the same wavelength here! :) I'll update my answer with the find method. \$\endgroup\$ – doppelgreener Jan 27 '11 at 16:25
  • \$\begingroup\$ Maybe the file names are short and we don't call this function much, but please be aware that the proposed solution is O(n*m) on the number of characters in the string (n) and the number of illegal characters in the string (m). \$\endgroup\$ – WilliamKF Feb 9 '11 at 2:04
16
\$\begingroup\$

you could always use transform

#include <algorithm>
#include <string>
#include <iostream>

const std::string forbiddenChars = "\\/:?\"<>|";
static char ClearForbidden(char toCheck)
{
    if(forbiddenChars.find(toCheck) != string::npos)
    {
         return ' ';
    }

    return toCheck;
}

int main()
{
    std::string str = "EXAMPLE:";
    std::transform(str.begin(), str.end(), str.begin(), ClearForbidden);
    std::cout << str << std::endl;
    return 0;
}
\$\endgroup\$
  • \$\begingroup\$ Didn't even see this when I was just posting my answer. Yet another way to do it with a different STL algorithm :) \$\endgroup\$ – Mark Loeser Jan 28 '11 at 0:31
  • 2
    \$\begingroup\$ Same thing with lambda: std::transform(str.begin(), str.end(), str.begin(), [&forbidden](char c) { return forbidden.find(c) != std::string::npos ? ' ' : c; } \$\endgroup\$ – Jon Purdy Jan 28 '11 at 2:37
5
\$\begingroup\$

One thing that I would change about your function (in addition to Jonathan's recommendation of using a string to store the forbidden characters), is the argument type of removeForbiddenChar to string& instead of string*. It is generally considered good practice in C++ to use references over pointers where possible (see for example this entry in the C++ faq-lite).

One further, minor cosmetic change I'd recommend is renaming the function to removeForbiddenChars (plural) as that is more descriptive of what it does.

\$\endgroup\$
  • \$\begingroup\$ You are never checking the validity of the string * s, so if a nullptr is passed in the removeForbiddenChar function will attempt to dereference a nullptr. This implies that the caller of removeForbiddenChar should check for nullptr before calling removeForbiddenChar, but the caller won't necessarily be aware of this unless they view the internals of removeForbiddenChar. Requiring the reference to be passed in instead of a pointer conveys that your intention is: "You MUST have a valid string in order to call removeForbiddenChar." \$\endgroup\$ – YoungJohn Dec 12 '13 at 23:06
5
\$\begingroup\$

Or, here's yet another way you could do it by using all stuff from the STL:

#include <algorithm>
#include <string>
#include <iostream>

bool isForbidden( char c )
{
    static std::string forbiddenChars( "\\/:?\"<>|" );

    return std::string::npos != forbiddenChars.find( c );
}

int main()
{
    std::string myString( "hell?o" );

    std::replace_if( myString.begin(), myString.end(), isForbidden, ' ' );

    std::cout << "Now: " << myString << std::endl;
}
\$\endgroup\$
4
\$\begingroup\$

C comes with a helpful function size_t strcspn(const char *string, const char *delimiters) that you can implement this on top of. The ASCII version is pretty fast; it uses a bit vector to test for the delimiter characters.

\$\endgroup\$
  • \$\begingroup\$ If you are looking for performance, this one is hard to beat. \$\endgroup\$ – EvilTeach Jan 28 '11 at 14:45
3
\$\begingroup\$

Solution with no conditional branching.
Swapping space for time optimization.

Simplified algorithm:

void removeForbiddenChar(string* s)
{
    for (string::iterator it = s->begin() ; it < s->end() ; ++it)
    {
        // replace element with their counterpart in the map
        // This replaces forbidden characters with space.
        (*it) = charMap[*it];
    }
}

Or the C++0x version:

void removeForbiddenChar(std::string* s)
{
    std::transform(s->begin(), s->end(), [](char c) => {return charMap[c];});
}

Just need the data:

char    charMap[] =
                            // The majority of characters in this array
                            // map the poistion to the same character code.
                            //  charMap['A']  == 'A'
                            // For forbidden characters a space is in the position
                            //  charMap['<']  == ' '
                            //  Note: \xxx is an octal escape sequence
                            "\000\001\002\003\004\005\006\007"
                            "\010\011\012\013\014\015\016\017"
                            "\020\021\022\023\024\025\026\027"
                            "\030\031\032\033\034\035\036\037"
                            "\040\041 \043\044\045\046\047" // replaced \042(") with space
                            "\050\051\052\053\054\055\056 " // replaced \057(/) with space
                            "\060\061\062\063\064\065\066\067"
                            "\070\071 \073 \075  " // replaced \072(:)\074(<)\076(>)\077(?) with space
                            "\100\101\102\103\104\105\106\107"
                            "\110\111\112\113\114\115\116\117"
                            "\120\121\122\123\124\125\126\127"
                            "\130\131\132\133 \135\136\137" // replaced \134(\)
                            "\140\141\142\143\144\145\146\147"
                            "\150\151\152\153\154\155\156\157"
                            "\160\161\162\163\164\165\166\167"
                            "\170\171\172\173\174\175\176\177"
                            "\200\201\202\203\204\205\206\207"
                            "\210\211\212\213\214\215\216\217"
                            "\220\221\222\223\224\225\226\227"
                            "\230\231\232\233\234\235\236\237"
                            "\240\241\242\243\244\245\246\247"
                            "\250\251\252\253\254\255\256\257"
                            "\260\261\262\263\264\265\266\267"
                            "\270\271\272\273\274\275\276\277"
                            "\300\301\302\303\304\305\306\307"
                            "\310\311\312\313\314\315\316\317"
                            "\320\321\322\323\324\325\326\327"
                            "\330\331\332\333\334\335\336\337"
                            "\340\341\342\343\344\345\346\347"
                            "\350\351\352\353\354\355\356\357"
                            "\360\361\362\363\364\365\366\367"
                            "\370\371\372\373\374\375\376\377";
\$\endgroup\$
1
\$\begingroup\$

Similar to strcspn is strpbrk, but instead of returning offsets, it returns a pointer to the next match and NULL if no more matches. This makes the replacement as simple as:

while ((filename = strpbrk(filename , "\\/:?\"<>|")) != NULL)
    *filename++ = '_';
\$\endgroup\$

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.