Hope you're all doing well!

I'm reading and doing the exercises in Stanley Lippman's C Primer Ed.5.

I'm having the following issue with my code, when I update an existing Sales_data object in the std::vector<Sales_data> vec; the program crashes.

In order to over come this I erase the existing Sales_data object and replace it with a new updated object.

Is there a more efficient way of doing this without erasing the Sales_data object and then replacing it?

My CODE:

#include <iostream>
#include <vector>

/*
struct: Sales_data
attributes: total revenue, number of copies sold, average sales price for a book

*/

struct Sales_data
{
    //Exercise 7.2
    std::string isbn() const{return this->bookNo;}
    Sales_data& combine(const Sales_data &rhs)
    {
        this->units_sold  = rhs.units_sold;
        this->revenue  = rhs.revenue*rhs.units_sold;
        return *this;
    }

    Sales_data add(const Sales_data &lhs, const Sales_data &rhs)
    {
        Sales_data sum =lhs;
        sum.combine(rhs);
        return sum;
    }

    std::string bookNo;
    unsigned units_sold =0;
    double revenue =0.0;
};

int main()
{
    Sales_data book;
    std::vector<Sales_data> vec;
    bool isDuplicate = false;

        while(std::cin>>book.bookNo>>book.units_sold>>book.revenue)
        {
            for(auto it =vec.begin(); !vec.empty()&&it!=vec.end();   it)
            {
                if(book.bookNo == it->isbn()) //can also be written to dereference the it pointer (*it).bookNo
                {
                    Sales_data add_book =it->add(*it, book);

                    vec.erase(it); //must erase to prevent a crash
                    vec.push_back(add_book);//push add_book obj in vec position
                    isDuplicate = true;
                }
            }

          if(!isDuplicate)
            {
                vec.push_back(book);
            }

            for(size_t i=0; i!=vec.size();   i)
            {
                std::cout<<vec[i].isbn()<<"  "<<vec[i].units_sold<<"  "<<vec[i].revenue<<std::endl;
            }

          isDuplicate = false;
        }

    return 0;
}

//Thank you.

CodePudding user response：

You're not allowed to modify a collection as you're iterating over it. This is exemplified by push_back: occasionally it can decide to resize the backing vector, which makes it point to now-uninitialized memory and the resulting symptom is a crash. Calling erase first prevents the array from growing, but also invalidates it which has annoying side effects.

In this case, the correct solution is to overwrite the contents of the iterator in place:

*it = it->add(*it, book);

Or you can call combine directly:

it->combine(book);

Or you could choose to override operator = and get:

*it  = book;

CodePudding user response：

vec.erase(it); invalidates it, so when the loop continues, you have undefined behaviour.

Is there a more efficient way of doing this without erasing the Sales_data object and then replacing it?

Yes, use combine instead of add.

Aside: can you use standard algorithms, i.e. std::find_if?

while(std::cin >> book.bookNo >> book.units_sold >> book.revenue) {
    auto it = std::find_if(vec.begin(), vec.end(), [&](auto & other){ return book.isbn() == other.isbn(); });
    if (it != vec.end()) {
        it->combine(book);
    } else {
        vec.push_back(book);
    }
}

I'd also suggest moving your inputing into std::istream& operator>>(std::istream & is, Sales_data & data).