views:

130

answers:

6

Run time error on main method in MovieList.java.

I'm not sure my program design is fundamentally very good, but I'd like to know why it crashes. Thanks in advance.

package javaPractical.week3;

import javax.swing.*;

public class Movie {
    //private attributes
    private String title;
    private String movieURL;
    private String year;
    private String genre;
    private String actor;

    // constructor
    Movie(String t, String u, String y, String g, String a) {
        this.title = t;
        this.movieURL = u;
        this.year = y;
        this.genre = g;
        this.actor = a;

    }
    //getters and setters
    public void setTitle(String t) {
        this.title = t;
    }

    public String getTitle() {
        return this.title;
    }

    public void set_url(String a) {
        this.movieURL = a;
    }

    public String get_url() {
        return this.movieURL;
    }

    public void setYear(String y) {
        this.year = y;
    }

    public String getYear() {
        return this.year;
    }

    public void setGenre(String g) {
        this.genre = g;
    }

    public String getGenre() {
        return this.genre;
    }

    public void setActor(String a) {
        this.actor = a;
    }

    public String getActor() {
        return this.actor;
    }


    //output movie details
    public String toString() {
        return ("Title: " + this.title + "\nURL: " + this.movieURL + "\nYear: "
            + this.year + "\nGenre: " + this.genre + "\nActor: "
            + this.actor);
    }

    public static void main(String[] args) {
        //testing Movie class
        Movie Movie1 = new Movie("Spiderman", "www.", "2002", "Action",
            "Tobey M");

        JOptionPane.showMessageDialog(null, Movie1.toString());
        //testing MovieList class
    }
}

package javaPractical.week3;

import javax.swing.*;

import java.util.ArrayList;

public class MovieList1 {

    private static ArrayList myFavouriteMovies = new ArrayList();
    private static int NUM_OF_MOVIES = 10;
    private int numberOfMovies = 0;
    private int index = 0;

    public MovieList1() {
        this.myFavouriteMovies = null;
        this.numberOfMovies = 0;
        this.index = 0;
    }

    public int getNumberOfMovies() {
        return this.myFavouriteMovies.size();
    }

    public boolean isEmpty() {
        if (this.myFavouriteMovies.isEmpty()) {
            return true;

        } else
        return false;

    }

    public static void main(String[] args) {
        MovieList1 List = new MovieList1();
        String titleADD;
        String movieURLADD;
        String yearADD;
        String genreADD;
        String actorADD;

        titleADD = JOptionPane.showInputDialog(null, "Enter title:");
        movieURLADD = JOptionPane.showInputDialog(null, "Enter URL:");
        yearADD = JOptionPane.showInputDialog(null, "Enter year:");
        genreADD = JOptionPane.showInputDialog(null, "Enter genre:");
        actorADD = JOptionPane.showInputDialog(null, "Enter actor:");

        Movie TempMovie = new Movie(titleADD, movieURLADD, yearADD, genreADD,
            actorADD);

        myFavouriteMovies.add(TempMovie);   
    }
}
+1  A: 

When you call the constructor for MovieList1, you set the ArrayList MyFavouriteMovies to null. If you call methods on MyFavouriteMovies, there is a null pointer exception (at myFavouriteMovies.add(TempMovie);).

this.myFavouriteMovies = null; should be this.myFavouriteMovies = new ArrayList(); and private static ArrayList myFavouriteMovies = new ArrayList(); should be private ArrayList myFavouriteMovies;

By the way, I wouldn't make myFavouriteMovies static, since it differs for every instance of MovieList1. You'd then have a method addMovie() in MovieList1. Also, if NUM_OF_MOVIES is constant, as the uppercase name suggests, you should declare it final.

irrelephant
+6  A: 

The program crashes when it tries to add the new Movie to myFavouriteMovies, because myFavouriteMovies is null.

Although myFavouriteMovies is initialised to a new, empty ArrayList, it's then set to null in the MovieList1 constructor.

At the moment, myFavouriteMovies is static, so there's only one copy of this variable shared between every MovieList1 instance. You probably want to remove the static modifier from the myFavouriteMovies declaration. Then each MovieList1 object will have its own myFavouriteMovies field. However you'll then to add a new method to the MovieList1 class to allow your main method to add the movie to the movie list, perhaps like this:

List.add(TempMovie);

Also you'll need to remove

this.myFavouriteMovies = null;

from the constructor, because having initialised it to an empty ArrayList, you don't want to set it back to null.

Richard Fearn
+2  A: 

Within your constructor you are setting

 public MovieList1() {
   this.myFavouriteMovies = null;
   this.numberOfMovies = 0;
   this.index = 0;
 }

after you already declared myFavouriteMovies above. This could result in a NullPointer

Keyboardsurfer
Yes. @James Is myFavouriteMovies static, or a an instance variable? Choose one and instantiate it only once.
Synesso
+1  A: 

All the above answers are spot on, but I question whether or not you even need the MovieList1 class at all. Essentially, you're just providing a wrapper around List. I'm not sure if you have plans to expand the behaviors of the movie list, but as is, you could simply do:

List<Movie> movies = new ArrayList<Movie>();
String titleADD = JOptionPane.showInputDialog(null, "Enter title:");
String movieURLADD = JOptionPane.showInputDialog(null, "Enter URL:");
String yearADD = JOptionPane.showInputDialog(null, "Enter year:");
String genreADD = JOptionPane.showInputDialog(null, "Enter genre:");
String actorADD = JOptionPane.showInputDialog(null, "Enter actor:");

Movie TempMovie = new Movie(titleADD, movieURLADD, yearADD, genreADD, actorADD);
movies.add(TempMovie);  

A couple of other notes...

You should probably have an addMovie(Movie movie) method or something similar in the movie list class instead of accessing the list directly in your main method.

You should program to the interface instead of declaring an ArrayList as the type of myFavoriteMovies.

There's no need to overwrite the values in the constructor because you've already instantiated or initialized them when you declare them.

You probably get a warning by calling this.myFavoriteMovies because it's static. Static members and methods should be accessed by ClassName.staticMethodOrVariable. I only mention this because there is a big difference between what this implies and what static is. This refers to the current reference of the type, while static is meant to be persistent. Also, when declaring unmutable static members, use the final modifier, but in this case, I don't think it needs to be static, but could definitely be final.

hisdrewness
A: 

Richard has the right answer to what the problem is. There is a better solution though.

When you declare a variable you should think about if you ever want it to change. If you do not mark it as final.

So:

private final String title;
private final String movieURL;
private final String year;
private final String genre;
private final String actor;

private static final int NUM_OF_MOVIES = 10;
private final List myFavouriteMovies = new ArrayList();
private int numberOfMovies;
private int index;

This will mean you need to get rid of most of the setXXX methods - if you think about it you never want to change the values after you create an instance anyways. The numberOfMovies and index presumably need to change, so they are not final. Finally (no pun intended!) you do not need to set instance variables to 0, null, or false, they are set to that by default.

TofuBeer
A: 

I am observing that you do not have a good grip of programming logic. You also need to further understand the flow of Java runtime and the behaviour of the language. Still, logic flow is a skill you need to acquire, regardless if you are communicating in Java, C, C++ or the English language. (BTW, the English language, despite having inconsistent synthetic elements, is an analytically logical language.)

First, you declare a static myFavouriteMovies and instantiate it to an ArrayList. After that, you equate it to null. After equating the variable to null, you attempt to use it as

myFavouriteMovies.add(TempMovie);

Of course, you will get a null pointer.

You need to acquaint yourself with the compiler and runtime error dumps. The Java runtime error would certainly have pointed out the statement number where it encountered your attempt to withdraw money from an empty non-overdraft-protected bank account.

C# programs written by "VB experts" always frustrate me. I realise this is not a kind response, but I am projecting a very presumptuous attitude in saying that you are highly probable a proficient Visual Basic programmer. Therefore, I am taking further opportunity towards all proficient Visual Basic programmers out there by throwing salt on an open wound (salt being a disinfectant), that you should NEVER transfer your programming expertise in an object-referencing language to object-oriented languages. Just as it is difficult for an expert speaker of a synthetically logical language like Greek or Hebrew to adjust to an analytically logical language like English, vice versa.

Secondly, what is the reason behind your declaring myFavouriteMovies as static? Do you realise the implications of a static variable?

Thirdly, you should not refer to a static variable thro the class instance reference "this". myFavouriteMovies is a static variable within the class Movie. Therefore, you should refer to it as

Movie.myFavouriteMovies

and not as

this.myFavouriteMovies

Java allows you to perform such aberrational reference but C# would not. "this" should be reserved for "instance variables".

Finally, there is high probability, compared to other "VB experts", that you need further understanding of what instance variables are vs static variables. A static variable exists without needing a class to be instantiated. Therefore, it is "shared" by all instances of the class; and if it is public or protected, it is shared with any process elements to which that static variable is exposed.

Normally, when you have a static list, your get method should not return the reference of that static variable. You should return a subset of the list. For example, you could have a static list of all the Fans of every movie. When you need to return a list of the list of Fans of a particular movie, you do not return the whole static list but only a subset of that static list. You need to understand when static variables could be used and restrain yourself from deploying static references - meaning, DO NOT declare a variable static if making it a class instance will work.

You can use a static variable to monitor and restrict that a Movie class is not instantiated more than, say, five times or that there are not currently more than five instances, by incrementing a static count everytime it is instantiated and decrementing everytime it is destroyed. You can use a static variable to hold a pool of connections to the database, where each connection is an instance of a connection.

Little trivia: Do you know that you could perform static manipulation in Java using a static block? If there are more than one static block, they will be processed in the order they are declared. As usual, any variable declared inside a code block is not visible outside that block.

class Hello{
  static {
    // perform static manipulation here
  }

  public void setAnInstanceValue(int a){
    ...
  }
  static {
    // another static block of code here
  }
  .....
}

You should try to instantiate your static variables in a static code block. If you find that you could instantiate a static variable in an instance constructor, there is high probability that it should not be static.

Read this: Execution flow for static code block - http://forums.sun.com/thread.jspa?threadID=5418566.

Blessed Geek