Video Store Rental Application

I'm in the middle of developing a video store rental application for a school project but am starting to question whether I'm going about things in the correct manner. Although the application is producing the desired results, I have a suspicion there's a more efficient way of coding/designing it. Any suggestions on how I can improve the design pattern of my code would be greatly appreciated.

Here's my main class:

```
public class PurpleBox implements AdminADT, UserADT {

// class variables
private Double volumeDiscount;
private Double priceDVD;
private Double priceBluray;
private String promoCode;
private String adminPassword;
private String adminName;
private String userName;
private String userEmail;
private boolean unitDisabled;
private final ArrayList shoppingCart;
private final Map movieList = new HashMap() {{
put("Sleepy Hollow", new Movie("Sleepy Hollow", "Sci-Fi", 2014, 1, false));
put("Dumb and Dumber", new Movie("Dumb and Dumber", "Comedy", 2002, 2, false));
}};

public PurpleBox() {
shoppingCart = new ArrayList<>();
priceDVD = 1.99;
priceBluray = 2.99;
volumeDiscount = .05;
promoCode = "10OFFMOV";
unitDisabled = false;
}

@Override
public void addMovieToShoppingCart(String name) {
if(movieList.containsKey(name)) {
Movie movie = movieList.remove(name);
shoppingCart.add(movie);
} // end if
else {
System.out.print("Error: There is no movie by that name.");
}
} // end addMovieToCart

@Override
public Movie removeMovieFromShoppingCart(int index) {
return shoppingCart.remove(index);
} // end removeMovieFromCart

@Override
public ArrayList viewShoppingCart() {
return shoppingCart;
}

@Override
public void removeAllMoviesFromShoppingCart() {
shoppingCart.clear();
} // end removeAllMoviesFromCart

@Override
public void seeAvailableMovies() {
for(Map.Entry entry : movieList.entrySet()) {
if(!(entry.getValue().isEmpty())) {
System.out.prin

Floats and Doubles

You should be very careful using doubles like so:

priceDVD = 1.99;
priceBluray = 2.99;
volumeDiscount = .05;

I will point you to this article about floating point. Although it is very in depth and potentially difficult to understand, it is worth reading. The bottom line is that if you don't understand the consequences of multiplying, dividing, adding and subtracting floating point numbers in your code, then by using them you will inevitably make mistakes and lose precision.

Specifically when it comes to money, floating point is not recommended. Here is another link for you about money and rounding errors. There are libraries available for dealing with money if you are able to use them. Another option is to simply use integers, where 1 represents 1 cent. This is much more reliable and more compatible with the base 10 nature of money.

Variable Names

public PurpleBox() {

PurpleBox() is not a very good name for what you have here. It is not descriptive enough and really doesn't tell me anything about what the object actually represents.

private boolean unitDisabled;

It is not clear what a unit is, so this variable name should be changed to something that makes the code more self documenting.

public void seeAvailableMovies() {

Since there is no view here and nothing is being displayed on screen, a better name for this might be logAvailableMovies().

In general, strive to have variable and method names that explain what they represent in the simplest and clearest way possible.

Unnecessary Comments

I see things like this in a few places in the code:

} // end if
    } // end for
} // end seeAvailableMovies

} // end removeAllMoviesFromCart

} // end addMovieToCart

In general, comments should never explain what the code is doing, but rather why it is doing what its doing. I think it is okay to leave yourself comments that are notes to yourself when you are trying to learn a language, but it is distracting to read them when reviewing code.

Design

Overall it seems like you are approaching the problem in a correct way. You have appropriately created a Movie object so there is a separation of concerns there, which is good.

I see a slight problem here:

private final Map movieList = new HashMap() {{
    put("Sleepy Hollow", new Movie("Sleepy Hollow", "Sci-Fi", 2014, 1, false));
    put("Dumb and Dumber", new Movie("Dumb and Dumber", "Comedy", 2002, 2, false));    
}};

I would definitely try to find a way to avoid hard coding these values at the same time that you are declaring the instance variables of the class. I think that the shopping cart should have methods to add and remove Movie objects, and then you could have a method that adds the initial movies to the movieList. I think it is cleaner this way, and allows you to add more movies to the code in an easy to find location, as well as allows you to add and remove movies on the fly.

I also think that you may want to avoid using a raw String as the key for the Movies in the Map. I don't think there is anything wrong with using Strings in this way, but since you are having to type the name twice in this line:

put("Sleepy Hollow", new Movie("Sleepy Hollow", "Sci-Fi", 2014, 1, false));

It is more prone to errors. You could make a string constant for each of the movie names that you intend to be dealing with, and then you would at least only have to type the string once.

StackExchange Code Review Q#81953, answer score: 5