🔒 Locked

C++ Removal of char from string

Started by Obs Aug 21, 2011 at 3:06 PM 6 replies 2.3k views

Original Post

Aug 21, 2011 at 3:06 PM

I was wondering if anyone could shed some light on a problem I'm having.

I have a string that I am removing unwanted characters from. The characters I want removed are:

(
)
:
,
space

The first four characters are no problem, but any conditional statements based upon the space do not fire when tested at the same time as the first four.

E.g. the following doesn't work. All characters apart from space are removed. If I test only for the space, then spaces are removed.


 string removeCharacters(string modified) 
 {
 int tempSize = modified.size(); 
 
 for (int it = ( tempSize-1); it > 0; it-- ) 
 {
 if ( ( modified[it] == ' ' ) || ( modified[it] == '(') ) || ( modified[it] == ')' ) || ( modified[it] == ':' ) || ( modified[it] == ',' ) )
 {
 modified.erase(it, 1); 
 it--; 
 }
 }
 return modified;
 }

I've tried case statements, too. Same behaviour (understandable). However, if I seperate the space condition from the if statement and put its own case statement, it works. Likewise if I seperate it into its own if statement from a main case statement it works. Seperating into the same kind of statement does not work.

I was originally using string::rfind() and searching for each character to be removed in turn and that worked. I don't know why I started playing around with case/if statements, but this behaviour has been bothering me for a while and I refuse to go back to rfind() without understanding what is wrong here.

Wooh

Aug 21, 2011 at 3:26 PM

The code you posted has a syntax error. You have one extra ) after ( modified[it] == '('). You should also remove it--; because that will make you skip the next char in the string.

EvilTesla-RG

Aug 21, 2011 at 3:31 PM

First off, it is generaly considered bad practice to modify a loop inside of itself. It tends to produce unpredictable results.

Wooh

Aug 21, 2011 at 3:40 PM

In these cases I think it quite nice to use a switch to test the values. So I made a version of your function using switch just so you can see, I also use iterator instead of an index.

string removeCharacters(string modified) 
 { 
 string::iterator it = modified.begin();
 while (it != modified.end()) 
 {
 switch (*it)
 {
 case ' ':
 case '(':
 case ')':
 case ':':
 case ',':
 it = modified.erase(it);
 break;
 default:
 ++it;
 }
 }
 return modified;
 }

Trienco

Aug 21, 2011 at 4:19 PM

Quick and not flexible:


 bool filter(char c) { return std::string(" ():,").find(c) != std::string::npos; };
 
 str.erase( remove_if( str.begin(), str.end(), filter ), str.end() );

Reusable for any list of characters:


 struct CharFilter
 {
 CharFilter(const std::string& filter) : chars(filter) {}
 
 bool operator()(char c) { return chars.find(c) != std::string::npos; }
 
 std::string chars;
 };
 
 str.erase( remove_if( str.begin(), str.end(), CharFilter(" (),:") ), str.end() );

Obs

Aug 21, 2011 at 5:17 PM

@wooh:

The extra ) is not in my code as it wasnt a straight copy/paste, but well spotted. As for skipping the next character with it--, that's my problem right there - so thank you! It's was left over from my rfind() implementation to save an uneccessary iteration as the string is 'shortened' by one and I never even considered if I needed it in this implementation (which uses a different iteration). A space ONLY occurs between two other 'forbidden' characters so it was always skipped.

The switch route was my preferred solution, too. I switched to an if statement when it wasn't working (I had left it-- in that too). It also makes more sense to pass string::iterator to erase, although I'm going to stick with working backwards through the string.

Thank you again for the reply.

Trienco, thank you for the reply, too - I'll take a look at your code, though I wasn't explicitly looking for a new solution.

L. Spiro

Aug 22, 2011 at 8:24 AM

You also never scan the first character of the string.

Try this instead:
for (int it = tempSize; --it >= 0; )

L. Spiro

Trienco

Aug 22, 2011 at 7:53 PM

You also never scan the first character of the string.

Try this instead:
for (int it = tempSize; --it >= 0; )

Ieek... My first reaction was "now your first access is invalid". Then I saw the rather disturbing use of side effects in the condition. I might be tempted to kick someone at work for throwing that kind of curve ball just to avoid writing a -1 (and not at least making it a more readable while-loop).

Though generally the bugs pointed out are one more reason to just use the convenient methods of the standard library and avoiding any potential for bugs in the first place.

Topic Locked

This topic has been locked by a moderator. New replies are not allowed.