string manipulation

[cppnoob25]

How can this code be made more better? Besides removing the namespace.

I wrote this and understand everything except how the reference works here. That reference was mostly place upon trial and error on my end. I get that is is passing the actual string cars but how isn't it being interpreted as &car when used below.

#include <iostream>
#include <string>

using namespace std;

void removeChars(string &str, string rem)
{
    for(int i = 0; i < str.length(); i++)
    {
        for(int j = 0; j < rem.length(); j++)
        {
            if(str[i] == rem[j])
            {
                for(int k = i; k < str.length(); k++)
                {
                    str[k] = str[k+1];
                }
            }
        }
    }

}

int main()
{
    string car = "red blue cars";
    string r = "aeiou";

    removeChars(car, r);

    cout << car;
}

[BlatantlyX]

There is a problem in your code where you do not decrease the the string length by 1 after shifting all characters. You are also reading past the length of the string with str[k+1], you should consider using k < str.length() - 1 instead.
Another problem is i needs to be decremented every time you find a match to produce the correct output. You are outputting

rd ble crs

instead of

rd bl crs

.

There are also several other things you could do since you are using std::string, such as using find and erase.

If you want to be pedantic:
You are comparing between int and size_type. std::string::length() returns a std::string::size_type.

This is what I would propose:

void removeChars(string &str, string rem)
{
	std::string::size_type pos = std::string::npos;
	while( ( pos = str.find_first_of(rem) ) != std::string::npos){
			str.erase(pos, 1);
	}

}

[coder777]

You may consider a reference as an alias: Access the same variable just with another name.

[cppnoob25]

Thanks for the feedback. I didn't notice the incorrect output either. I'll work on this.

[dhayden]

Consider a pathological case where str is a million 'X' characters and rem is "X". Your solution will find the first X is str, and copy 999,999 characters down one position. Then it finds the second X and copies 999,998 characters down one position.

A few days later, the function finishes :).

A better approach is to create a destination string. Then for each character in str, if it isn't in rem, then copy it to the destination string. At the end, assign str = destination.

Another change would help for large rem and str strings, but would be slower for small strings: create an array of 256 bools to represent the 256 characters. Initialize it to false, then for each char in rem, set the value in the array to true. Now use the above algorithm, but when deciding if a character is in rem, just look it up the array rather than searching through rem.