Loop quitting for no reason

Question

I have a question regarding C++. This is my current function:

string clarifyWord(string str) {
    //Remove all spaces before string
    unsigned long i = 0;
    int currentASCII = 0;

    while (i < str.length()) {
        currentASCII = int(str[i]);
        if (currentASCII == 32) {
            str.erase(i);
            i++;
            continue;
        } else {
            break;
        }
    }

    //Remove all spaces after string
    i = str.length();
    while (i > -1) {
        currentASCII = int(str[i]);
        if (currentASCII == 32) {
            str.erase(i);
            i--;
            continue;
        } else {
            break;
        }
    }
    return str;
}

Just to get the basic and obvious things out of the way, I have #include <string> and using namespace std; so I do have access to the string functions.

The thing is though that the loop is quitting and sometimes skipping the second loop. I am passing in the str to be " Cheese " and it should remove all the spaces before the string and after the string.

In the main function, I am also assigning a variable to clarifyWord(str) where str is above. It doesn't seem to print that out either using cout << str;.

Is there something I am missing with printing out strings or looping with strings? Also ASCII code 32 is Space.

http://stackoverflow.com/questions/216823/whats-the-best-way-to-trim-stdstring — leeor_net, Aug 28 '11 at 22:45
It's a lot clearer to write `' '` if you mean a space. And it also works on (rare) non-ASCII machines. — MSalters, Aug 28 '11 at 23:07

Darcy Rayner · Accepted Answer · 2011-08-28T22:58:54.003

Okay so the erase function you are calling looks like this:

string& erase ( size_t pos = 0, size_t n = npos );

The n parameter is the number of items to delete. The npos means, delete everything up until the end of the string, so set the second parameter to 1.

str.erase(i,1)

[EDIT]

You could change the first loop to this:

while (str.length() > 0 && str[0] == ' ')
{
   str.erase(0,1);
}

and the second loop to this:

while (str.length() > 0 && str[str.length() - 1] == ' ')
{
   str.erase(str.length() - 1, 1);
}

score 1 · Answer 2 · answered Aug 28 '11 at 22:49

1

In your second loop, you can't initialize i to str.length().

str[str.length()] is going to be after the end of your string, and so is unlikely to be a space (thus triggering the break out of the second loop).

answered Aug 28 '11 at 22:49

Richard Campbell

3,591
23
18

Hmmm... Okay, I think I understand. In saying that, how would I remove the spaces at the end of the string? Reverse the string then repeat the first loop then after that, reverse it back? – Brandon Aug 28 '11 at 23:01

Diego Sevilla · Answer 3 · 2011-08-28T23:04:46.083

You're using erase (modifying the string) while you're in a loop checking its size. This is a dangerous way of processing the string. As you return a new string, I would recommend you first to search for the first occurrence in the string of the non-space character, and then the last one, and then returning a substring. Something along the lines of (not tested):

size_t init = str.find_first_not_of(' ');
if (init == std::string::npos)
    return "";
size_t fini = std.find_last_not_of(' ');
return str.substr(init, fini - init + 1);

You see, no loops, erases, etc.

score 0 · Answer 4 · answered Aug 28 '11 at 22:48

0

unsigned long i ... while (i > -1) Well, that's not right, is it? How would you expect that to work? The compiler will in fact convert both operands to the same type: while (i > static_cast<unsigned long>(-1)). And that's just another way to write ULONG-MAX, i.e. while (i > ULONG_MAX). In other words, while(false).

answered Aug 28 '11 at 22:48

MSalters

173,980
10
155
350

Okay, I think I have fixed that up :) I have replaced `while (i > -1)` to be `while (i >= 0)` because I want the loop to run if it is equal to 0, to check the first character in the string. – Brandon Aug 28 '11 at 23:03
@Brandon: Is `i` still unsigned? Because an unsigned is by defintion always >= 0. You just turned it into a `while (true)` – MSalters Aug 28 '11 at 23:05

hookenz · Answer 5 · 2011-08-28T23:00:02.577

You're using erase incorrectly. It'll erase from pos to npos.
i.e. string& erase ( size_t pos = 0, size_t n = npos );

See: http://www.cplusplus.com/reference/string/string/erase/

A better way to do this is to note the position of the first non space and where the spaces occur at the end of the string. Then use either substr or erase twice.

You also don't need to go to the trouble of doing this:

 currentASCII = int(str[i]);
       if (currentASCII == 32) {

Instead do this:

if (str[i] == ' ') {

Which I think you'll agree is a lot easier to read.

So, you can shorten it somewhat with something like: (not tested but it shouldn't be far off)

string clarifyWord(string str) {
  int start = 0, end = str.length();
  while (str[start++] == ' ');
  while (str[end--] == ' ');
  return str.substr(start, end);
}

Loop quitting for no reason

5 Answers5