1

I've recently begun learning C++ and I'm having some trouble updating a pointer in my Movie class. I've got a feeling I have an issue somewhere in my Move/Copy constructors but have been trying to solve this issue for hours, swapping pointers to references to values and finally coming here for help.

I have a class Movie, which contains a string name, string rating and int watched member variables. Another class Movies, holds a vector of the movies. On movie, my method increment_watched is supposed to increment the int watched by one, the value does get incremented but it seems like the value is not saved. The display_all_movies method in Movies, which simply displays the movies that it stores holds the old value for watched. I know the issue is probably something really small but I haven't been able to work out why the value isn't being saved.

If someone could explain why the pointer value seems to be getting overridden I'd appreciate it greatly. Thanks in advance!

Code is below, there is some debug couts in there.

Movie.h

#ifndef _MOVIE_H_
#define _MOVIE_H_
#include <string>
#include <iostream>

class Movie {
private:
    std::string *name;
    std::string *rating;
    int *watched;
public:
    std::string get_name();
    std::string get_rating();
    int get_watched();
    void increment_watched();
    Movie(std::string name, std::string rating, int watched_val); // normal constructor
    Movie(const Movie &source); // copy constructor
    Movie(Movie &&source); // move constructor
    ~Movie();

};

#endif // _MOVIE_H_

Movie.cpp

#include "Movie.h"

std::string Movie::get_name() {
    return *name;
}

std::string Movie::get_rating() {
    return *rating;
}

int Movie::get_watched() {
    return *watched;
}

void Movie::increment_watched() {
    std::cout << *watched << std::endl;
    (*watched)++; // TODO FIX!
    std::cout << *watched << std::endl;
}

Movie::Movie(std::string name, std::string rating, int watched_val) 
    : name{nullptr}, rating{nullptr}, watched{nullptr}{   
    std::cout << "Creating movie " << watched_val << " with normal constructor" << std::endl; 
    this->name = new std::string{name};
    this->rating = new std::string{rating};
    this->watched = new int{watched_val};

}

Movie::Movie(const Movie &source) 
    : Movie(*source.name, *source.rating, *source.watched) {
        std::cout << "Creating movie " << *source.watched << " with copy constructor" << std::endl; 

}

Movie::Movie(Movie &&source)
    : Movie(*source.name, *source.rating, *source.watched) {
        std::cout << "Creating movie " << *source.watched << " with move constructor" << std::endl; 
        source.name = nullptr;
        source.rating = nullptr;
        source.watched = nullptr;
}

Movie::~Movie() {
        delete name;
        delete rating;
        delete watched;
}

Movies.h

#ifndef _MOVIES_H_
#define _MOVIES_H_
#include <vector>
#include <string>
#include <iostream>
#include "Movie.h"

class Movies {
private:
    static int count;
    std::vector<Movie> *movies;
public:
    void add_movie(std::string &&name, std::string &&rating, int &&watch);
    void increment_movie_count(std::string &&name);
    void display_all_movies();
    void count_movies();
    Movies();
    Movies(const Movie &source); // copy constructor
    Movies(Movie &&source); // move constructor
    ~Movies();

};

#endif // _MOVIES_H_

Movies.cpp

#include "Movies.h"

int Movies::count = 0;

void Movies::add_movie(std::string &&name, std::string &&rating, int &&watch) {
    bool contains {false};

    for(auto movie : *movies) {
        if(movie.get_name() == name) {
            contains = true;
        }
    }
    if(!contains) {
        movies->push_back(Movie{name, rating, watch});
        count++;
    }
}

void Movies::increment_movie_count(std::string &&name) {
    for(auto movie : *movies) {
        if(movie.get_name() == name) {
            movie.increment_watched();
        }
    }

}

void Movies::display_all_movies() {
    for(auto movie : *movies) {
        std::cout << "Title " << movie.get_name() << " Rating " << movie.get_rating() << " Watched " << movie.get_watched() << std::endl;
    }
}

void Movies::count_movies() {
    std::cout << "There are " << count << " movies " << std::endl; 
}

Movies::Movies() {
    movies = new std::vector<Movie>{};
}

Movies::~Movies() {
    delete movies;
}

And finally main.cpp

#include <iostream>
#include "Movies.h"

using std::cout;
using std::endl;

int main() {

    Movies *my_movies;


    my_movies = new Movies();

    (*my_movies).count_movies();

    my_movies->add_movie("Big", "PG-13", 2);
    my_movies->increment_movie_count("Big");

    my_movies->display_all_movies();

    return 0;
}
  • 1
    why do you use pointers everywhere? In your code there is no obvious reason for using a single pointer – 463035818_is_not_a_number Nov 27 '19 at 20:45
  • If you've just begun learning C++, please learn "proper" C++ and do not use raw pointers (not at this point in time)! Also avoid rvalue references `&&` if you don't know what they do! I suggest you change your tutorial/teacher/learning resource as quick as possible! – andreee Nov 27 '19 at 20:45
  • 1
    Is there a reason why you are dynamically allocating everything? – ChrisMM Nov 27 '19 at 20:46
  • 2
    Recommended reading: [Why should C++ programmers minimize use of 'new'?](https://stackoverflow.com/questions/6500313/why-should-c-programmers-minimize-use-of-new) – user4581301 Nov 27 '19 at 20:46
  • FWIW, I really support this: https://www.youtube.com/watch?v=YnWhqhNdYyk – andreee Nov 27 '19 at 20:47
  • 2
    Use `&` in range-based for. – rafix07 Nov 27 '19 at 20:47
  • Also recommended: [What are the rules about using an underscore in a C++ identifier?](https://stackoverflow.com/questions/228783/what-are-the-rules-about-using-an-underscore-in-a-c-identifier). It's rare that an identifier like `_MOVIES_H_` will cause you problems, but when it does, the error messages and program behaviour are utterly bizarre. – user4581301 Nov 27 '19 at 20:49
  • 1
    Side note: your move constructor is wrong, if I'm not mistaken. It leaks memory. –  Nov 27 '19 at 21:06
  • Also, don't use pointers unless completely necessary. It will make your life a lot easier. –  Nov 27 '19 at 21:07
  • Your move constructor has to call an allocation function three times (because it invokes `Movie(*source.name, *source.rating, *source.watched)`). Nothing has actually been moved, everything has been copied, defeating the entire point of using pointers. – David Schwartz Nov 27 '19 at 21:08
  • @formerlyknownas_463035818 I was under the impression for non-primative types pointers should be used? as it is more efficient to pass the pointer than to copy the value? – DarkSideOfTheMoon Nov 27 '19 at 21:37
  • @andreee thanks for the advice! I'm actually currently doing a course, I know the lectures on smart pointers are coming up, but I guess they've decided to teach raw pointers first. I've only used rvalue references for my move constructor here, is my implementation incorrect? or is it that these shouldn't be used and am being taught outdated concepts? – DarkSideOfTheMoon Nov 27 '19 at 21:39
  • thats only partly correct, but to keep it simple: This is wrong. Especially for your members there is no point in using pointers. When you want to avoid copies you pass references around – 463035818_is_not_a_number Nov 27 '19 at 21:41
  • @user4581301 interesting thanks for the link. I've been taught that I should always define my header files _CLASS_H_ and that it's common practice – DarkSideOfTheMoon Nov 27 '19 at 21:43
  • `CLASS_H` is fine. `_CLASS_H` is not. Violates the no `_` prefix at any scope rule (and the `_` prefix at file scope rule, not that it matters). – user4581301 Nov 27 '19 at 22:22
  • I would say that the resources are outdates. And in any case, I wouldn't consider rvalue references a topic for a beginner's C++ introduction... – andreee Nov 28 '19 at 05:38

1 Answers1

3

Changefor(auto movie : *movies) to for(auto& movie : *movies) to update the original movie objects. Otherwise, you're only updating copies of the movie objects.

Saisai3396
  • 191
  • 1
  • 8