Thread: Loop crashes program

Code:

void prodRemove(string name, vector<product> vec){
	for(vector<product>::size_type i = 0; i != vec.size(); ++i){
		if(vec[i].name == name){
			vec.erase(vec.begin() + i);
		}
	}
}

That for loop crashes the program... Need help!
Thanks in advance

Ahh... So I set i to 0 again? Or can I use break; in For loops?

EDIT: nvm, tried myself :P

Yeah, thought about that too, but the user inputs a name, and I only want the first found product having that name, incase the user has inputted the same product twice, so break suits me fine

Unless the vector is passed by reference that code will have no effect once the function ends as the vector operated upon will be the local copy and not the original... or is that just a simple omission in the posted code?

Yeah, I noticed that :P But it's changed now

This is how the loop should look like:

Code:

void prodRemove(string name, vector<product>& vec){
    for(vector<product>::size_type i = 0; i != vec.size(); ){
        if(vec[i].name == name){
            vec.erase(vec.begin() + i);
        }
        else {
            ++i;
        }
    }
}

That is, you only move on to the next element if you didn't remove anything.

However, it is recommended to use existing algorithms instead of coding loops yourself. Sadly, since the condition for removal is not trivial (compare against a member) and if you cannot use lambda functions, you'll need a function object. The following code is a bit longer, but everything there is simpler and easier to get right than in the original code. It is also more efficient performance wise since it erases from the end of the vector and remove_if doesn't move anything more than once.

Code:

class NameEquals
{
    std::string name;
public:
    NameEquals(const std::string& n): name(n) {}
    bool operator()(const product& prod) const
    {
        return prod.name == name;
    }
};

void prodRemove(const string& name, vector<product>& vec)
{
    vec.erase(
        remove_if(vec.begin(), vec.end(), NameEquals(name)),
        vec.end()
    );
}

Why is break; not good enough o.O? It's also way clearer, easier to understand, or atleast I think so xP

Perhaps if there is only one product by the name and you want to remove just that.

Again, it could be done like this:

Code:

class NameEquals
{
    std::string name;
public:
    NameEquals(const std::string& n): name(n) {}
    bool operator()(const product& prod) const
    {
        return prod.name == name;
    }
};

void prodRemove(const string& name, vector<product>& vec)
{
    vec.erase(find_if(vec.begin(), vec.end(), NameEquals(name)));
}

Well I want it to be like, let's say some store used this and added a product called Soda, then later, maybe after adding loads of other products, adds Soda again, 'cause the user forgot he already had inputted it, so he wants to remove it, then he only has to type /remove, and then Soda, and it only deletes one of the Soda's
break; does that fine enough, and it's blue in VC :P

If you want one of each kind at any time, you can use a std::set<product>. Then any time you add a Soda, any previous Soda if any will be replaced.

You'll need to overload operator< or provide a comparison function.

Originally Posted by anon

Then any time you add a Soda, any previous Soda if any will be replaced.

Insertion of duplicates in a std::set container will simply fail, the old one does not get replaced by the new.