Problem with string

[Gaijinco]

I did this function:

void clean(string&s)
{
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
s[i]=tolower(s[i]);
else
s.erase(i,1);
}
return;
}

I'm having a strange behavior with string like these:

"Disneyland " -> Disneyland
".disneylan d." -> .disneyland"

It seems as if there is something going on with the erase function that
i don't know, because with this:

void clean(string&s)
{
string s2;
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
{
s[i]=tolower(s[i]);
s2+=s[i];
}
}
s=s2;
return;
}

Everything goes OK.

What's wrong with the first code?

Thanks.

[benben]

Gaijinco wrote:

I did this function:

void clean(string&s)
{
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
s[i]=tolower(s[i]);
else
s.erase(i,1);
}
return;
}
A call to erase() does not just rub off the content and left a hole in
the string. It moves the subsequent content forward to replace the part
to be removed.

As a result of moving the content, your index i no longer indexes into
the character that you want.

I'm having a strange behavior with string like these:

"Disneyland " -> Disneyland
".disneylan d." -> .disneyland"

It seems as if there is something going on with the erase function that
i don't know, because with this:
....

void clean(string&s)
{
string s2;
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
{
s[i]=tolower(s[i]);
s2+=s[i];
}
}
s=s2;
return;
}

Everything goes OK.
Yep, this is the right way to do it. It is a cleaner, faster and, above
all, correct implementation.

What's wrong with the first code?
Index are much like iterators, they are "invalidate d" when you erase
something from a container.

Thanks.

Ben

[Larry I Smith]

Gaijinco wrote:

I did this function:

void clean(string&s)
{
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
s[i]=tolower(s[i]);
else
s.erase(i,1);
}
return;
}

I'm having a strange behavior with string like these:

"Disneyland " -> Disneyland
".disneylan d." -> .disneyland"

It seems as if there is something going on with the erase function that
i don't know, because with this:

void clean(string&s)
{
string s2;
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
{
s[i]=tolower(s[i]);
s2+=s[i];
}
}
s=s2;
return;
}

Everything goes OK.

What's wrong with the first code?

Thanks.

Well yes, erase() removes chars, so the size has changed.
Here's a quick and dirty example:

#include <iostream>
#include <string>
#include <cctype>

void clean( std::string& s )
{
std::string::it erator it;

// step thru the string using the iterator 'it'
for( it = s.begin(); it != s.end(); )
{
// if the current char is an alpha
if( isalpha(*it) )
{
// change the char to lower case
*it = tolower(*it);

// increment 'it' to the next char
++it;
}
else
{
// delete the current non-alpha char from 's'.
// resets 'it' to point to the first char after
// the one just deleted - or to s.end() if no
// more chars in 's'.
it = s.erase(it);
}
}

return;
}

int main()
{
std::string t1 = "Disneyland ";
std::string t2 = ".disneylan d.";

clean(t1);
clean(t2);

std::cout << t1 << std::endl;
std::cout << t2 << std::endl;

return 0;
}

This program outputs:

disneyland
disneyland

Regards,
Larry

[dj]

Larry I Smith wrote:

Gaijinco wrote:

I did this function:

void clean(string&s)
{
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
s[i]=tolower(s[i]);
else
s.erase(i,1);
}
return;
}

I'm having a strange behavior with string like these:

"Disneyland " -> Disneyland
".disneylan d." -> .disneyland"

It seems as if there is something going on with the erase function that
i don't know, because with this:

void clean(string&s)
{
string s2;
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
{
s[i]=tolower(s[i]);
s2+=s[i];
}
}
s=s2;
return;
}

Everything goes OK.

What's wrong with the first code?

Thanks.

Well yes, erase() removes chars, so the size has changed.
Here's a quick and dirty example:

#include <iostream>
#include <string>
#include <cctype>

void clean( std::string& s )
{
std::string::it erator it;

// step thru the string using the iterator 'it'
for( it = s.begin(); it != s.end(); )
{
// if the current char is an alpha
if( isalpha(*it) )
{
// change the char to lower case
*it = tolower(*it);

// increment 'it' to the next char
++it;
}
else
{
// delete the current non-alpha char from 's'.
// resets 'it' to point to the first char after
// the one just deleted - or to s.end() if no
// more chars in 's'.
it = s.erase(it);
}
}

return;
}

int main()
{
std::string t1 = "Disneyland ";
std::string t2 = ".disneylan d.";

clean(t1);
clean(t2);

std::cout << t1 << std::endl;
std::cout << t2 << std::endl;

return 0;
}

This program outputs:

disneyland
disneyland

Regards,
Larry

In cases like this, i prefer to traverse the container in reverse order.
In that case the part of the container we have already examined is
shifted and presents no problem.

Wouldn't you agree?

[Gaijinco]

Thank you for all your opinions! This was quite insightful!

[Daniel T.]

In article <11************ **********@u72g 2000cwu.googleg roups.com>,
"Gaijinco" <ga******@gmail .com> wrote:

I did this function:

void clean(string&s)
{
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
s[i]=tolower(s[i]);
else
s.erase(i,1);
}
return;
}

I'm having a strange behavior with string like these:

"Disneyland " -> Disneyland
".disneylan d." -> .disneyland"

It seems as if there is something going on with the erase function that
i don't know

Think about what happens when you erase an element of the container. All
the elements that come after are shifted one toward the front of the
container. So, for example, when the first quote is erased, the period
is moved to element 0, then 'i' is incremented to be equal to 1, so the
period is never checked.

There are a couple of ways to fix this bu why don't you take a stab at
it first.

--
Bene disserere est finis logices. -- Aristotle

[Daniel T.]

In article <dv************ ********@news.s iol.net>,
dj <sm*******@lyco s.com> wrote:

In cases like this, i prefer to traverse the container in reverse order.
In that case the part of the container we have already examined is
shifted and presents no problem.

Wouldn't you agree?

No, still too many assignments and redundant checks when an element is
removed. Better would be:

void clean( string& s )
{
string::size_ty pe insert = 0;
for ( string::size_ty pe index = 0; index != s.size(); ++index )
if ( isalpha( s[index] ) )
s[insert++] = tolower( s[index] );
s.erase( insert, string::npos );
}

That way, each character is only moved once. (strictly speaking that
last line could be "s.erase( insert );" but that doesn't make it clear
that all elements from insert to the end of the string are being erased.)

--
Bene disserere est finis logices. -- Aristotle

[Daniel T.]

In article <44************ ***********@new s.optusnet.com. au>,
benben <be******@yahoo .com.au> wrote:

void clean(string&s)
{
string s2;
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
{
s[i]=tolower(s[i]);
s2+=s[i];
}
}
s=s2;
return;
}

Everything goes OK.

Yep, this is the right way to do it. It is a cleaner, faster and, above
all, correct implementation.

It isn't faster for two reasons. (1) s2 needs to call new[], possibly
multiple times and each time it must copy the contents of the old array
into the new array (2) after s2 is filled in, it is assigned back into
's' (yet another loop.)

See my previous post for a more appropriate implementation (which is
based on the remove_copy_if algorithm.)

--
Bene disserere est finis logices. -- Aristotle

[Larry I Smith]

dj wrote:

Larry I Smith wrote:

Gaijinco wrote:

I did this function:

void clean(string&s)
{
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
s[i]=tolower(s[i]);
else
s.erase(i,1);
}
return;
}

I'm having a strange behavior with string like these:

"Disneyland " -> Disneyland
".disneylan d." -> .disneyland"

It seems as if there is something going on with the erase function that
i don't know, because with this:

void clean(string&s)
{
string s2;
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
{
s[i]=tolower(s[i]);
s2+=s[i];
}
}
s=s2;
return;
}
Everything goes OK.

What's wrong with the first code?

Thanks.

Well yes, erase() removes chars, so the size has changed.
Here's a quick and dirty example:

#include <iostream>
#include <string>
#include <cctype>

void clean( std::string& s )
{
std::string::it erator it;

// step thru the string using the iterator 'it'
for( it = s.begin(); it != s.end(); )
{
// if the current char is an alpha
if( isalpha(*it) )
{
// change the char to lower case
*it = tolower(*it);

// increment 'it' to the next char
++it;
}
else
{
// delete the current non-alpha char from 's'.
// resets 'it' to point to the first char after
// the one just deleted - or to s.end() if no
// more chars in 's'.
it = s.erase(it);
}
}

return;
}

int main()
{
std::string t1 = "Disneyland ";
std::string t2 = ".disneylan d.";

clean(t1);
clean(t2);

std::cout << t1 << std::endl;
std::cout << t2 << std::endl;

return 0;
}

This program outputs:

disneyland
disneyland

Regards,
Larry

In cases like this, i prefer to traverse the container in reverse order.
In that case the part of the container we have already examined is
shifted and presents no problem.

Wouldn't you agree?

I didn't intend to optimize the original 'clean()', just make
it work correctly; otherwise, I might have been tempted to use
'remove_if()' and/or 'translate()'

Regards,
Larry

[dj]

Larry I Smith wrote:

dj wrote:

Larry I Smith wrote:

Gaijinco wrote:
I did this function:

void clean(string&s)
{
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
s[i]=tolower(s[i]);
else
s.erase(i,1);
}
return;
}

I'm having a strange behavior with string like these:

"Disneyland " -> Disneyland
".disneylan d." -> .disneyland"

It seems as if there is something going on with the erase function that
i don't know, because with this:

void clean(string&s)
{
string s2;
for(size_t i=0; i<s.size(); ++i)
{
if(isalpha(s[i]))
{
s[i]=tolower(s[i]);
s2+=s[i];
}
}
s=s2;
return;
}
Everything goes OK.

What's wrong with the first code?

Thanks.

Well yes, erase() removes chars, so the size has changed.
Here's a quick and dirty example:

#include <iostream>
#include <string>
#include <cctype>

void clean( std::string& s )
{
std::string::it erator it;

// step thru the string using the iterator 'it'
for( it = s.begin(); it != s.end(); )
{
// if the current char is an alpha
if( isalpha(*it) )
{
// change the char to lower case
*it = tolower(*it);

// increment 'it' to the next char
++it;
}
else
{
// delete the current non-alpha char from 's'.
// resets 'it' to point to the first char after
// the one just deleted - or to s.end() if no
// more chars in 's'.
it = s.erase(it);
}
}

return;
}

int main()
{
std::string t1 = "Disneyland ";
std::string t2 = ".disneylan d.";

clean(t1);
clean(t2);

std::cout << t1 << std::endl;
std::cout << t2 << std::endl;

return 0;
}

This program outputs:

disneyland
disneyland

Regards,
Larry

In cases like this, i prefer to traverse the container in reverse order.
In that case the part of the container we have already examined is
shifted and presents no problem.

Wouldn't you agree?

I didn't intend to optimize the original 'clean()', just make
it work correctly; otherwise, I might have been tempted to use
'remove_if()' and/or 'translate()'

Regards,
Larry

Going reverse is by no means optimization - it is exactly what you said
you try to achieve - correctness. The only difference is in the "for" line.