1

I have a MovieController object with an array of Movie objects. I have a MovieView object that calls 'doAddMovie' on the controller.

When I call 'doAddMovie' in the MovieController's constructor, a movie gets added to the array. But when I call 'doAddMovie' from the MovieView there's a segmentation fault.

Can anyone tell me what I'm doing wrong?

MovieController.h

#ifndef MOVIECONTROLLER_H
#define MOVIECONTROLLER_H

#include "MovieView.h"
#include "Movie.h"

class MovieView;

class MovieController
{
  public:
    MovieController();
    void doAddMovie(string title, int year, string genre);

  private:
    MovieView* movieView;
    Movie** movies;
};

MovieController.cc:

const int MAX_MOVIES = 10;

MovieController::MovieController() {
  movies = new Movie*[MAX_MOVIES];
  doAddMovie("Star Wars", 1977, "SciFi"); // <<-- this works
}

/* when I call this method from my MovieView object, I get a segmentation fault, pretty
 * sure the fault happened on line:  movies[i] = new Movie(title, year, genre);
 */
void MovieController::doAddMovie(string title, int year, string genre) {
  cout << "doAddMovie; title=" << title << "; year=" << year << "; genre =" << genre;
  int i = 0;
  while (i < MAX_MOVIES) {
    if (movies[i] == NULL) {
      movies[i] = new Movie(title, year, genre); <<-- segmentation fault here
      break;
    }
    i = i+1;
  }
  return;
}
Improve this question
2
  • 3
    Looks correct. If you give us a minimal complete example, we can probably find the bug (if you don't discover it yourself in the process). Commented Jan 31, 2014 at 3:24
  • Why are you forward declaring MovieView? Wouldn't it be defined or at least already declared in MovieView.h? Commented Jan 31, 2014 at 3:31

2 Answers 2

Reset to default
2

doAddMovie() expects to find a NULL pointer on an available slot, but you are not initializing the array slots to NULL after you have allocated the array, so the contents of the array will be random garbage. The fact that doAddMovies() works when called in the constructor is a fluke.

Try this instead:

class MovieController
{
public:
    MovieController();
    ~MovieController();

    void doAddMovie(string title, int year, string genre);

private:
    MovieView* movieView;
    Movie** movies;
};


MovieController.cc:
const int MAX_MOVIES = 10;

MovieController::MovieController()
{
    movies = new Movie*[MAX_MOVIES];
    memset(movies, 0, sizeof(Movie*)*MAX_MOVIES); // <-- add this
    doAddMovie("Star Wars", 1977, "SciFi");
}

MovieController::~MovieController()
{
    for (int i = 0; i < MAX_MOVIES; ++i)
        delete movies[i];
    delete[] movies;
}

void MovieController::doAddMovie(string title, int year, string genre)
{
    for (int i = 0; i < MAX_MOVIES; ++i)
    {
        if (movies[i] == NULL)
        {
            movies[i] = new Movie(title, year, genre);
            cout << "Movie added: title=" << title << "; year=" << year << "; genre =" << genre;
            return;
        }
    }

    cout << "Movie ignored: title=" << title << "; year=" << year << "; genre =" << genre;
}

With that said, you should embrace C++ memory management and not use a raw array at all:

#include <vector>

class MovieController
{
public:
    MovieController();
    void doAddMovie(string title, int year, string genre);

private:
    MovieView* movieView;
    std::vector<Movie> movies;
};


MovieController.cc:

MovieController::MovieController()
{
    doAddMovie("Star Wars", 1977, "SciFi");
}

void MovieController::doAddMovie(string title, int year, string genre)
{
    movies.push_back(Movie(title, year, genre));
    cout << "Movie added: title=" << title << "; year=" << year << "; genre =" << genre;
}

Finally, if you are still getting the SEGFAULT after that, then make sure your MovieView object is calling doAddMovie() using a valid MovieController object pointer to begin with.

Improve this answer
Sign up to request clarification or add additional context in comments.

Comments

1

After taking a step back, I realized that I wasn't setting 'this' MovieController in the associated MovieView, so when MovieView was calling doAddMovie it was doing so on a different instance of MovieController that has an uninitialized array.

Feeling stupid. But thanks anyway!

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.