I'm looking for a bit of help please, I have a merchant class that you can buy from and sell to. The merchant has his own vector of struct 'items' as does the player. To begin the transactions, a merchants menu function is called, passing in the address of a player (character class)

The below code works perfectly as expected, (e.g. if you haven't got enough gold, it states it correctly, or if either person doesn't have the item, etc) but as soon as a transaction occurs, like the player selling something, any attempts to sell anything else will show no message and just show the ".1. Back" option, or trying to buy something will say twice the merchant doesn't have it, or if he does, it will say he doesn't have it, but then say you bought it... it's probably something really simple and bad design on my part, but I can't figure it out

void merchant::buyfrom(character *a)
{
    print("\nItem to buy: "); 
    string item1; 
    fflush(stdin); 
    getline(cin, item1);
    vector<item>::iterator it;
    for (it = this->items.begin(); it != this->items.end(); ++it)
    {
        if (it->name == item1)
        {
            if (it->price > a->gold)
            {
                print("\n\nYou don't have enough gold to buy the "); cout << it->name; break;
            }
            print("\n\nYou bought "); std::cout << item1; print(" for "); std::cout << it->price; print(" gold");
            this->items.erase(it);
            a->inventory.push_back(*it);
            a->gold -= it->price;
            this->gold += it->price;
            break;
        }
    else
    {
            print("\n\nMerchant does not have a "); std::cout << item1; print(" to sell");
    }
    }
    print("\n\n\n1. Back\n\n\n> "); 
   char c; 
   std::cin >> c;
}

void merchant::sellto(character *a)
{
    print("\nItem to sell: ");
   string item1; 
   fflush(stdin); 
   getline(cin, item1);
    vector<item>::iterator it;
    for (it = a->inventory.begin(); it != a->inventory.end(); ++it)
    {
        if (it->name == item1)
        {
            print("\n\nYou sold "); std::cout << item1; print(" for "); std::cout << it->price; print(" gold");
            a->inventory.erase(it);
            this->items.push_back(*it);
            a->gold += it->price;
            this->gold -= it->price;
            break;
        }
        else
        {
            print("\nYou do not have "); std::cout << item1; print(" to sell");
        }
    }
    print("\n\n\n1. Back\n\n\n> "); 
  char c; 
  std::cin >> c;
}

Not entirely sure, but the moment you call erase on an vector iterator the iterator is invalid. It happens that you seem to be able to access it's content, but you really shouldn't.

Put the erase call as last code before the break.

2 hours ago, Endurion said:

Not entirely sure, but the moment you call erase on an vector iterator the iterator is invalid. It happens that you seem to be able to access it's content, but you really shouldn't.

Put the erase call as last code before the break.

Or simply put it as "it = items.erase(it);"  Also, you don't need to use this->, I personally hate typing extra and making it harder to read the code but that is a personal thing.

Thanks, i've put the erase just before the break but the problem still occurs. I guess if there isn't anything obviously wrong the bug must be on the menu function which calls the above 2 functions? Or possibly the item struct?

Well, I'd assume the bug is that you're changing the loop index "it" inside the loop.  Can you really reason correctly about the loop invariant if the loop invariant changes in the middle of the loop?

You can see the effects of calling std::vector::erase here.  Pay special attention to the bit that says Invalidates iterators and references at or after the point of the erase, including the end() iterator.

Thanks for all the replies. I've fixed the iterator issue, and the actual cause of the problem was something to do with the messages not being properly conditioned, adding in a few ifs below fixed the issue

void merchant::buyfrom(character *a)
{
    print("\nItem to buy: "); string item1; getline(cin, item1);
    vector<item>::iterator it;
    unsigned vector_size = this->items.size();
    int check = 0; // to handle merchant does not have msg with not enough gold msg
    for (it = this->items.begin(); it != this->items.end();)
    {
        if (it->name == item1)
        {
            if (it->price > a->gold)
            {
                print("\n\nYou don't have enough gold to buy the "); cout << it->name; check = 1; break;
            }
            print("\n\nYou bought "); std::cout << item1; print(" for "); std::cout << it->price; print(" gold");
            a->inventory.push_back(*it);
            a->gold -= it->price;
            this->gold += it->price;
            this->items.erase(it);
            break;
        }
    else
    {
            ++it;
    }
    }
    if (this->items.size() == vector_size && check == 0)
    {
    print("\n\nMerchant does not have a "); std::cout << item1; print(" to sell");
    }
    print("\n\n\n1. Back\n\n\n> "); char c; std::cin >> c;
}

void merchant::sellto(character *a)
{
    print("\nItem to sell: "); string item1; getline(cin, item1);
    vector<item>::iterator it;
    unsigned vector_size = a->inventory.size();
    for (it = a->inventory.begin(); it != a->inventory.end();)
    {
        if (it->name == item1)
        {
            print("\n\nYou sold "); std::cout << item1; print(" for "); std::cout << it->price; print(" gold");
            this->items.push_back(*it);
            a->gold += it->price;
            this->gold -= it->price;
            a->inventory.erase(it);
            break;
        }
        else
        {
            ++it;
        }
    }
        if (a->inventory.size() == vector_size)
        {
            print("\nYou do not have "); std::cout << item1; print(" to sell");
        }
    /* int increase = (it->price * 1.2+1);
    vector<item>::iterator it2;
    for (it2 = this->items.begin(); it2 != this->items.end(); ++it2)
        {
            if (it2->name == it->name)
            {
                it2->price = increase;
            }
            } */
    print("\n\n\n1. Back\n\n\n> "); char c; std::cin >> c;
}

On 1/13/2018 at 9:53 PM, Bregma said:

Well, I'd assume the bug is that you're changing the loop index "it" inside the loop.  Can you really reason correctly about the loop invariant if the loop invariant changes in the middle of the loop?

You can see the effects of calling std::vector::erase here.  Pay special attention to the bit that says Invalidates iterators and references at or after the point of the erase, including the end() iterator.

That isn't relevant here, as every time he erases an iterator from within the loop, he breaks out of the loop.

I think the issue was the erase() being done too early, while he was still trying to use the iterator after the erase() call