0

So I have a custom class 'Book' that has a bunch of member variables, amongst which are a vector of another custom class called 'Review' and a pointer to point at that vector as I need to pass that around through function calls in a driver program. The driver program reads the details of the each book (such as title, author, publish date etc.) from a text file and inserts into a temporary 'Book' object which it then adds onto a vector of Books maintained by the driver program. Here is the code for reading from the file:

ifstream file("books.txt");
string line;
if(file.is_open())
{
    while(!file.eof())
    {
        Book buffBook;
        getline(file, line);
        buffBook.setTitle(line);
        getline(file, line);
        buffBook.setAuthor(line);
        getline(file, line);
        buffBook.setPubDate(line);
        getline(file, line);
        buffBook.setIsbn(line);
        getline(file, line);
        buffBook.setCategory(line);
        getline(file, line);
        buffBook.setFormat(line);
        getline(file, line);
        buffBook.setSynopsis(line);
        vectBooks.push_back(buffBook);
    }
}

else
    cout<<"File not found(1)!"<<endl;

file.close();

This is running inside a int main() function.
One of the functions of the driver program is to add a review, which takes in data from the user and inserts it into a temporary 'Review' object. That object is then passed onto to be inserted in to the vector of reviews for the corresponding book. Here is the code for the addReview() function:

void addReview()
{
    string name = "";
    string title;
    Book rTemp;
    cin.ignore();
    cout<<"Which book would you like to rate (Title)?: ";
    getline(cin, name);
    name = toLow(name);
    Review r;
    string re, user;
    int ra;
    cout<<"Username (Full Name): ";
    getline(cin, user);
    string fname = user.substr(0, user.find_first_of(' '));
    string lname = user.substr( user.find_first_of(' ') + 1, user.size());
    r.setUsrFName(fname);
    r.setUsrLName(lname);
    cout<<"Enter rating (1-5):";
    cin>>ra;
    r.setRating(ra);
    cout<<"Enter a short textual review: ";
    cin.ignore();
    getline(cin, re);
    r.setReview(re);
    for(unsigned int i = 0; i < vectBooks.size(); i++)
    {
         title = toLow(vectBooks[i].getTitle());
         if(title.find(name) != string::npos)
         {
             vectBooks[i].getReviews()->push_back(r);
         }
    }
}

Now the problem is if i add a review, it adds it for all the books. In other words, when I fetch the book info for any book, the review shows on all the books. I assume this is a problem with the pointer as it seems like all the reviews are getting stored in the same vector. I am not sure where I am messing up but I have a feeling it's with the pointer some where. Any help is appreciated.

Thank You

UPDATE

The point to the title of this problem is that I am doing the assignment of the pointer to the vector of Reviews in the constructor of the Book class, of which those 2 are member variables. Code as follows for constructor:

Book::Book()
{
    pointRev = &vectReviews;
}

UPDATE 2

Here is the code for the Book Class and supporting classes:

book.h

#ifndef BOOK_H_
#define BOOK_H_

#include <string>
#include <iostream>
#include <vector>
#include "review.h"

using namespace std;

class Book
{
private:
    string title;
    string author;
    string pubDate;
    string isbn;
    string category;
    string format;
    string synopsis;
    vector<Review> vectReviews;
    vector<Review>* pointRev;
public:
    Book::Book() : pointRev(&vectReviews) {};
    string getAuthor() const;
    string getCategory() const;
    string getFormat() const;
    string getIsbn() const;
    string getPubDate() const;
    string getSynopsis() const;
    string getTitle() const;
    vector<Review>* getReviews();
    void setAuthor(string author);
    void setCategory(string category);
    void setFormat(string format);
    void setIsbn(string isbn);
    void setPubDate(string pubDate);
    void setSynopsis(string synopsis);
    void setTitle(string title);

    friend ostream& operator <<(ostream& out, Book& book);
    vector<Review> *getPointRev() const;
    vector<Review> getVectReviews() const;
    void setPointRev(vector<Review> *pointRev);
    void setVectReviews(vector<Review> vectReviews);


};

#endif /* BOOK_H_ */

book. cpp

#include "book.h"
string Book::getAuthor() const
{
    return author;
}

string Book::getCategory() const
{
    return category;
}

string Book::getFormat() const
{
    return format;
}

string Book::getIsbn() const
{
    return isbn;
}

string Book::getPubDate() const
{
    return pubDate;
}

string Book::getSynopsis() const
{
    return synopsis;
}

string Book::getTitle() const
{
    return title;
}

void Book::setAuthor(string author)
{
    this->author = author;
}

void Book::setCategory(string category)
{
    this->category = category;
}

void Book::setFormat(string format)
{
    this->format = format;
}

void Book::setIsbn(string isbn)
{
    this->isbn = isbn;
}

void Book::setPubDate(string pubDate)
{
    this->pubDate = pubDate;
}

void Book::setSynopsis(string synopsis)
{
    this->synopsis = synopsis;
}

void Book::setTitle(string title)
{
    this->title = title;
}

vector<Review> *Book::getPointRev() const
{
    return pointRev;
}

vector<Review> Book::getVectReviews() const
{
    return vectReviews;
}

void Book::setPointRev(vector<Review> *pointRev)
{
    this->pointRev = pointRev;
}

void Book::setVectReviews(vector<Review> vectReviews)
{
    this->vectReviews = vectReviews;
}

vector<Review>* Book::getReviews()
{
    return pointRev;
}


ostream& operator <<(ostream& out, Book& book)
{
    out<<"\nTitle: "<<book.getTitle()<<endl;
    out<<"Author: "<<book.getAuthor()<<endl;
    out<<"Publish Date: "<<book.getPubDate()<<endl;
    out<<"ISBN: "<<book.getIsbn()<<endl;
    out<<"Category: "<<book.getCategory()<<endl;
    out<<"Format: "<<book.getFormat()<<endl;
    out<<"Synopsis: "<<book.getSynopsis()<<endl;
    cout<<"\n--- Reviews ---"<<endl;
//  vector<Review>* revs = book.getReviews();
    for(unsigned int h = 0; h < book.getReviews()->size(); h++)
    {
        cout<<"Review by: "<<book.getReviews()->at(h).getUsrFName()<<" "<<book.getReviews()->at(h).getUsrLName()<<endl;
        cout<<"Rating: "<<book.getReviews()->at(h).getRating()<<endl;
        cout<<"Review: "<<book.getReviews()->at(h).getReview()<<endl;
    }

    return out;
}

review.h

#ifndef REVIEW_H_
#define REVIEW_H_

#include <string>

using namespace std;

class Review
{
private:
    int rating;
    string review;
    string usrFName;
    string usrLName;
public:
    int getRating() const;
    string getReview() const;
    void setRating(int rating);
    void setReview(string review);
    string getUsrFName() const;
    string getUsrLName() const;
    void setUsrFName(string usrFName);
    void setUsrLName(string usrLName);
};

#endif /* REVIEW_H_ */

review.cpp

#include "review.h"



int Review::getRating() const
{
    return rating;
}

string Review::getReview() const
{
    return review;
}


void Review::setRating(int rating)
{
    this->rating = rating;
}

string Review::getUsrFName() const
{
    return usrFName;
}

string Review::getUsrLName() const
{
    return usrLName;
}

void Review::setUsrFName(string usrFName)
{
    this->usrFName = usrFName;
}

void Review::setUsrLName(string usrLName)
{
    this->usrLName = usrLName;
}

void Review::setReview(string review)
{
    this->review = review;
}
Vikas Rana
  • 1,961
  • 2
  • 32
  • 49
  • 3
    `while (!file.eof())` is not the correct way to write an input loop. For the correct way, see GMan's answer to [one of the Stack Overflow C++ FAQ questions](http://stackoverflow.com/questions/4258887/semantics-of-flags-on-basic-ios/4259111#4259111). – James McNellis Mar 02 '11 at 05:44
  • In a ctor one should prefer using the initializer list instead of an assignment in the ctor body. – 0xC0000022L Mar 02 '11 at 05:56
  • @STATUS_ACCESS_DENIED : Could you please give me an example? I'm not sure what you mean. – blazethesinner Mar 02 '11 at 06:01
  • Hard to to in a comment, but let me try: `Book::Book() : pointRev(&vectReviews) {}` ... – 0xC0000022L Mar 02 '11 at 06:05
  • @STATUS_ACCESS_DENIED: Alright. Thanks for that input. I tried that but it didn't work. Any other suggestions? Maybe I'm doing something wrong else where? – blazethesinner Mar 02 '11 at 06:10
  • @blaze: Show the copy-constructor and destructor also. In fact, showing the entire definition of `Book` would be good, it isn't clear what the lifetime of `vectReviews` is. – Ben Voigt Mar 02 '11 at 06:15
  • @STATUS : I dont have any destructors yet, and also not sure what a copy constructor is. I know this might show me as stupid but trust me I'm doing the best I can. I'll post the header and cpp file for book. – blazethesinner Mar 02 '11 at 06:25
  • @blaze: If you're trying to keep the question from getting really long with so much code, one thing you can do is use links to a separate page (like ideone.com or codepad.com). Those sites format code nicely and also show line numbers. – Ben Voigt Mar 02 '11 at 06:29
  • @Ben : Oh ok. I did not know that. Thanks for pointing that out. Will keep that in mind for future posts. – blazethesinner Mar 02 '11 at 06:33

1 Answers1

1

From the behavior you describe, a copy-constructor is running and making two objects that point to the same vector. push_back does indeed use a copy-constructor.

But your first code snippet doesn't make a bunch of copies of the same Book, but a new Book is created on each loop iteration (and then copied into vectBooks.

If Book doesn't have a correct user-defined copy-constructor then you aren't managing pointRev correctly. From the observed behavior, I believe that you have a destructor which frees pointRev, and then the copy inside vectBooks is left with a dangling pointer. Everything after this falls into the category of undefined behavior according to the standard, meaning "anything can happen" Then the next Book happens to reuse the same memory area, so all the instances of Book end up with the same value in the wild pointer. And then updating any one changes the vector (which isn't even alive anymore) seen by all Book instances.

Why are you using a pointer to a std::vector anyway? Much better to just put the vector into the class as a direct data member, which lets the compiler construct, copy, and destruct it automatically with no additional help from you.


Of course, it's entirely possible that you've made vectReviews a global variable, and then every single book points to the same instance. That would a review show up in all books simultaneously, because they share the vector you're adding it to.

Ben Voigt
  • 277,958
  • 43
  • 419
  • 720
  • @Ben : I don't have any destructors setup yet. I didn't really finish coding this project and it slipped my mind for now. Also, I'm not sure what you meant by why i am using vectors. The driver program doesn't really have a class. Just a main() function that does all the reading from files and then a switch cased menu that present options for manipulation books and reviews. So I am storing these Book objects in a vector declared globally in the driver program and each Book object has a corresponding vector of reviews for storing reviews from multiple users. – blazethesinner Mar 02 '11 at 06:19
  • @blazethesinner: Where does `vectBooks` come from? You're taking its address, but that pointer is only good as long as the original object is alive. – Ben Voigt Mar 02 '11 at 06:21
  • But anyways, I do not have any destructors set up yet. And could you give an example of a copy constructor? Or what one might look like in this case? – blazethesinner Mar 02 '11 at 06:24
  • http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three And I'm not asking why you're using vectors, I'm asking why your class has a pointer to a vector instead of just having a vector. – Ben Voigt Mar 02 '11 at 06:26
  • @Ben : to answer your question, it's because it's easier to pass the pointer around than pass the entire vector which I do not know how to do since I would need to pass it by reference the vector is a private member of the class 'Book'. – blazethesinner Mar 02 '11 at 06:35
  • @blaze: Surely you could just write `vector* Book::getReviews() { return &vectReviews; }` and not need the `pointRev` variable. And then it couldn't possibly end up pointing somewhere weird. You've done the right thing by making `vectReviews` a member of the class. – Ben Voigt Mar 02 '11 at 06:41
  • @Ben: So that means the getReviews() function will return a vector? Or dies it return a pointer? Because the return type is of pointer but it's the return statement is returning a vector, I'm confused what's actually being returned. And if it returns a vector, will changes to this returned vector be applied to the one being sent on the other end of the function call? – blazethesinner Mar 02 '11 at 06:46
  • @Ben : I tried replacing that return statement with what you stated and it worked. But I still don't understand what happened and changed. Could you please explain? I rather know how it works. Thanks again for the help though! – blazethesinner Mar 02 '11 at 06:48
  • @blaze: I don't actually see the code that caused the problem, you have quite a large program apparently. But something must have called `setPointRev` and associated this book with another book's list of reviews. See, when you do `setReviews` and say `vectReviews = ...;` it calls vector's assignment operator which makes a copy of the source data. But when `setPointRev` does `pointRev = ...;` it just copies the pointer, you then have two pointers to the same vector. And when that vector changed, it affected both books. – Ben Voigt Mar 02 '11 at 06:55
  • @blaze: Also, the [C++-FAQ Question "Rule of Three"](http://stackoverflow.com/questions/4172722/what-is-the-rule-of-three) does a good job of explaining deep copy vs shallow copy and why you need a copy constructor and destructor (and how to write them) when your class has a pointer inside. – Ben Voigt Mar 02 '11 at 07:02
  • @Ben: I was reading the rule of 3 document. Kind of in a rush now but will surely finish reading it later. But I think I got the basic point. Also, I didn't notice those set functions. I used my compilers generator for getters and setters so that generated automatically. But I haven't used it once in my code. – blazethesinner Mar 02 '11 at 07:09