C++ Function causing app to crash and not working properly

Question

A problem has come up in my application where my PrintAll function will not work correctly and only ultimately crash my application. My app is supposed to read strings from a file and insert them into an array. The problem is it is reading incorrectly and will ultimately crash my app. Here is where I think the problem lies:

int main()
{
    LoadMovies();

    MovieList *movies = LoadMovies();
    //movies->MovieList::PrintAll();

    //    // test methods for the Movie and MovieList classes
        //PrintAllMoviesMadeInYear(movies, 1984);
        //PrintAllMoviesWithStartLetter(movies, 'B');
        //PrintAllTopNMovies(movies, 5);

    //delete movies;
    return 0;
}

MovieList* LoadMovies()
{
    vector<string> movies;
    ReadMovieFile(movies);
    MovieList ml = MovieList(movies.size());

    string name;
    int year;
    double rating;
    int votes;

    for (int i = 0; i < movies.size(); i++)
    {
        istringstream input_string(movies[i]);
        getline(input_string, name, '\t');
        input_string >> year >> rating >> votes;
        Movie movie (name, year, votes, rating);
        ml.Add(movie);
    }
    ml.PrintAll();
}

Complete Example:

/*
 * File: MovieStatsProgram.cpp
 * Author:
 * Date:
 * ===============================================================
 * This is a console app to test the Movie and MovieList classes.
 *
 * TODO:
 *
 * You need to finish the implementation of the loadMovies method
 * to create and initialize the MovieList object.
 *
 * You also need to create three static methods:
 *
 * PrintAllMoviesMadeInYear - it will print all the movies made in a
 * given year once sort in alphabetical order and once sorted by the number
 * of votes with the movie with the most number of votes printed first.
 *
 * PrintAllMoviesWithStartLetter - it will print all the movies started with
 * a given letter sorted in alphabetical order
 *
 * PrintAllTopNMovies - it will display the top N movies based on the number of
 * votes
 */

#include <iostream>
#include <sstream>
#include <vector>
#include <string>
#include <iomanip>
#include <fstream>

using namespace std;

class Movie {
public:
    Movie();
    Movie(string n, int y, int v, double r);
    string get_name();
    void set_name(string n);
    int get_year();
    void set_year(int y);
    int get_votes();
    void set_votes(int v);
    double get_rating();
    void set_rating(double r);
    string PrintMovie();

private:
    string name;
    int year_made;
    int votes;
    double rating;

};

Movie::Movie() {
    name = "null";
    year_made = 0;
    votes = 0;
    rating = 0.0;
}

Movie::Movie(string n, int y, int v, double r) {
    name = n;
    year_made = y;
    votes = v;
    rating = r;
}

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

void Movie::set_name(string n) {
    name = n;
}

int Movie::get_year() {
    return year_made;
}

void Movie::set_year(int y) {
    year_made = y;
}

int Movie::get_votes() {
    return votes;
}

void Movie::set_votes(int v) {
    votes = v;
}

double Movie::get_rating() {
    return rating;
}

void Movie::set_rating(double r) {
    rating = r;
}

string Movie::PrintMovie() {
    cout << fixed << setprecision(1) << rating << "\t\t" << votes << "\t\t" << "(" <<
            year_made << ")" << "\t" << name << endl;
}

class MovieList {
public:
    MovieList(int size);
    ~MovieList();
    int Length();
    bool IsFull();
    void Add(Movie const& m);
    string PrintAll();

private:
    Movie* movies;
    int last_movie_index;
    int movies_size;
    int movie_count = 0;

};

MovieList::MovieList(int size) {
    movies_size = size;
    movies = new Movie[movies_size];
    last_movie_index = -1;
}

MovieList::~MovieList() {
    delete [] movies;
}

int MovieList::Length() {
    return last_movie_index;
}

bool MovieList::IsFull() {
    return last_movie_index == movies_size;
}

void MovieList::Add(Movie const& m)
{
    if (IsFull()) {
        cout << "Cannot add movie, list is full" << endl;
        return;
    }

    ++last_movie_index;
    movies[last_movie_index] = m;
}

string MovieList::PrintAll() {
    for (int i = 0; i < last_movie_index; i++) {
        movies[last_movie_index].Movie::PrintMovie();
        //cout << movies[last_movie_index] << endl;
    }
}

void ReadMovieFile(vector<string> &movies);
MovieList* LoadMovies();

enum MovieSortOrder
{
    BY_YEAR = 0,
    BY_NAME = 1,
    BY_VOTES = 2
};

int main()
{
    LoadMovies();

    MovieList *movies = LoadMovies();
    //movies->MovieList::PrintAll();

    //    // test methods for the Movie and MovieList classes
        //PrintAllMoviesMadeInYear(movies, 1984);
        //PrintAllMoviesWithStartLetter(movies, 'B');
        //PrintAllTopNMovies(movies, 5);

    //delete movies;
    return 0;
}

MovieList* LoadMovies()
{
    vector<string> movies;
    ReadMovieFile(movies);
    MovieList ml = MovieList(movies.size());

    string name;
    int year;
    double rating;
    int votes;

    for (int i = 0; i < movies.size(); i++)
    {
        istringstream input_string(movies[i]);
        getline(input_string, name, '\t');
        input_string >> year >> rating >> votes;
        Movie movie (name, year, votes, rating);
        ml.Add(movie);
    }
    ml.PrintAll();
}

void ReadMovieFile(vector<string> &movies)
{
    ifstream instream;
    instream.open("imdbtop250.txt");
    if (instream.fail())
    {
        cout << "Error opening imdbtop250.txt" << endl;
        exit(1);
    }


    while (!instream.eof())
    {
        string movie;
        getline(instream, movie);
        movies.push_back(movie);
    }

    instream.close();
}

When I use MovieList::PrintAll in the main function, my function just crashes, and when I put it in the LoadMovies function, it will read and add data incorrectly before crashing. The size of the list is 251 and the application will just read the same data 251 times.

Answer 1

You have a two part problem:

1: As Brad S stated, your function returns nothing. This is a no-no.

MovieList* LoadMovies()
{
    MovieList ml = MovieList(movies.size());
    // Your function returns a pointer to a MovieList, so...
    return &ml;
}

So, problem #2 is that you're going to return a pointer to something you created on the stack in your function. When you try to access it outside of your function, you'll run into undefined behavior.

Option 1:

 MovieList* ml = new MovieList( movies.size() );
 return ml;

You now need to delete ml when you're done w/ it.

Option 2: Change your function to return a non-pointer... then you don't have the hassle of managing the memory.

EDIT: Try this

int main()
{
    // Don't need this
    // LoadMovies();

    MovieList *movies = LoadMovies();

    // Uncommented this
    delete movies;
return 0;
}

MovieList* LoadMovies()
{
    vector<string> movies;
    ReadMovieFile(movies);
    // CHANGE
    MovieList* ml = new MovieList(movies.size());
    // CHANGE

    string name;
    int year;
    double rating;
    int votes;

    for (int i = 0; i < movies.size(); i++)    
    {
        istringstream input_string(movies[i]);
        getline(input_string, name, '\t');    
        input_string >> year >> rating >> votes;
        Movie movie (name, year, votes, rating);
        ml.Add(movie);
    }
    ml.PrintAll();
    // CHANGE
    return ml;
}

Answer 2

Your MovieList class has a fundamental problem. This comes to light on this line:

MovieList ml = MovieList(movies.size());

Your MovieList class has a member that is a pointer to dynamically allocated memory. Once you have this, you have to manage copying and assignment by creating a user-defined copy constructor and assignment operator.

The easiest fix for this is to use std::vector<Movie> instead of Movie * as a member variable of MovieList. Then copy-assignment comes for free and you don't need to implement further functions.

However, if you can't use std::vector for some reason, the following functions can be added:

class MovieList {
public:
    //...
    MovieList(const MovieList& m);
    MovieList& operator=(MovieList m);
    //...
};

#include <algorithm>
//...    
// copy constructor
MovieList::MovieList(const MoveList& m) {
    movies_size = m.size;
    movie_count = m.movie.count;
    last_movie_index = m.last_movie_index;
    movies = new Movie[movies_size];
    for (int i = 0; i < movies_size; ++i)
       movies[i] = m.movies[i];
}
//...
// assignment operator
MovieList& MovieList::operator=(MoveList m) {
    std::swap(m.movie_size, movie_size);
    std::swap(m.last_movie_index, last_movie_index);
    std::swap(m.movies, movies);
    std::swap(m.movie_count, moviE_count);
    return *this;
}

The easiest way to describe this to you is not to describe what these do. The best thing for you is to use your debugger and put a breakpoint in any of these functions and step through the code. When you hit the line I mentioned above, you will more than likely see that the copy constructor function is called -- then you can see it in action as to what it is doing.

The assignment operator is the function that's called when you assign an existing MovieList to another MovieList. It's implemented via the copy/swap idiom. This relies on a working copy constructor (provided above), and a destructor (which you already provided in your code). It works by creating a temporary MovieList, and swapping out the internals of the current MovieList with the temporary MovieList. There are many threads on SO as to how this works.

As to the reason why you need these functions above is that without the above functions, the line:

MovieList ml = MovieList(movies.size());

will create two MovieList objects, one temporary and one non-temporary, however the movies pointer for both will be pointing to the same memory. When the temporary is destroyed, the destructor is called, thus deleting the memory pointed to by movies. Now you have m1 pointing to memory that has gone up in smoke. Bad news when you try to use m1.

The user-defined copy and assignment functions above properly copy the object so that you get two distinct memory allocations for movies, so that when the destructor is called, the memory deleted will be unique to that object.

Again, all of this would be alleviated if you used std::vector and forego having to write copy ctor/assignment operators.