Loop crashes program

This is a discussion on Loop crashes program within the C++ Programming forums, part of the General Programming Boards category; 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); ...

  1. #1
    Hail to the king, baby. Akkernight's Avatar
    Join Date
    Oct 2008
    Location
    Faroe Islands
    Posts
    717

    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
    Currently research OpenGL

  2. #2
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,185
    Suppose i = 7 and vec.size = 8 (so we're looking at the last element on the list) and we happen to want to erase it. So we erase it, making vec.size 7; incrementing i gives us 8; i != vec.size, so we try to access vec[8] and bang!

  3. #3
    Hail to the king, baby. Akkernight's Avatar
    Join Date
    Oct 2008
    Location
    Faroe Islands
    Posts
    717
    Ahh... So I set i to 0 again? Or can I use break; in For loops?

    EDIT: nvm, tried myself :P
    Currently research OpenGL

  4. #4
    and the Hat of Guessing tabstop's Avatar
    Join Date
    Nov 2007
    Posts
    14,185
    You can break if you know there's only going to be one element you want to delete. (As it is you're skipping the element after the one you delete anyway.)

    Setting i to 0 is bad because then you have to start over.

    What I had in mind is that you would use a real condition in your for loop, to wit "<" instead of "!=".

  5. #5
    Hail to the king, baby. Akkernight's Avatar
    Join Date
    Oct 2008
    Location
    Faroe Islands
    Posts
    717
    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
    Currently research OpenGL

  6. #6
    Registered User hk_mp5kpdw's Avatar
    Join Date
    Jan 2002
    Location
    Northern Virginia/Washington DC Metropolitan Area
    Posts
    3,802
    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?
    "Owners of dogs will have noticed that, if you provide them with food and water and shelter and affection, they will think you are god. Whereas owners of cats are compelled to realize that, if you provide them with food and water and shelter and affection, they draw the conclusion that they are gods."
    -Christopher Hitchens

  7. #7
    Hail to the king, baby. Akkernight's Avatar
    Join Date
    Oct 2008
    Location
    Faroe Islands
    Posts
    717
    Yeah, I noticed that :P But it's changed now
    Currently research OpenGL

  8. #8
    The larch
    Join Date
    May 2006
    Posts
    3,573
    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()
        );
    }
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  9. #9
    Hail to the king, baby. Akkernight's Avatar
    Join Date
    Oct 2008
    Location
    Faroe Islands
    Posts
    717
    Why is break; not good enough o.O? It's also way clearer, easier to understand, or atleast I think so xP
    Currently research OpenGL

  10. #10
    The larch
    Join Date
    May 2006
    Posts
    3,573
    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)));
    }
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  11. #11
    Hail to the king, baby. Akkernight's Avatar
    Join Date
    Oct 2008
    Location
    Faroe Islands
    Posts
    717
    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
    Currently research OpenGL

  12. #12
    The larch
    Join Date
    May 2006
    Posts
    3,573
    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.
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

  13. #13
    Registered User hk_mp5kpdw's Avatar
    Join Date
    Jan 2002
    Location
    Northern Virginia/Washington DC Metropolitan Area
    Posts
    3,802
    Quote Originally Posted by anon View Post
    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.
    "Owners of dogs will have noticed that, if you provide them with food and water and shelter and affection, they will think you are god. Whereas owners of cats are compelled to realize that, if you provide them with food and water and shelter and affection, they draw the conclusion that they are gods."
    -Christopher Hitchens

Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Mutiliplcation Program that uses a (do, while, and for loop)?
    By Debbie Bremer in forum C++ Programming
    Replies: 4
    Last Post: 10-11-2008, 06:04 PM
  2. BOOKKEEPING PROGRAM, need help!
    By yabud in forum C Programming
    Replies: 3
    Last Post: 11-16-2006, 10:17 PM
  3. Replies: 4
    Last Post: 06-23-2006, 07:03 PM
  4. DEV-C++ made program crashes when run from outside the ide
    By rainmanddw in forum C++ Programming
    Replies: 3
    Last Post: 01-20-2006, 09:27 PM
  5. Program crashes and I can't figure out why
    By Unregistered in forum C Programming
    Replies: 6
    Last Post: 09-19-2001, 05:33 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21