8
\$\begingroup\$

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<Movie> shoppingCart;
private final Map<String, Movie> movieList = new HashMap<String, Movie>() {{
    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<Movie> viewShoppingCart() {
    return shoppingCart;
}

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

@Override
public void seeAvailableMovies() {       
    for(Map.Entry<String, Movie> entry : movieList.entrySet()) {
        if(!(entry.getValue().isEmpty())) {
            System.out.print(entry.getValue().getTitle());
        } // end if
    } // end for
} // end seeAvailableMovies

@Override
public HashMap<String, Movie> search(String movieTitle) {   

return null;
}

@Override
public void checkout() {

}

@Override
public void returnMovie(Movie movie) {
movieList.put(movie.getTitle(), movie);
}

@Override
public void addMovieToInventory(Movie aMovie) {        
    // make sure movie doesn't already exist
if(!movieList.containsKey(aMovie.getTitle())) { 
    movieList.put(aMovie.getTitle(), aMovie);       
} // end if
} // end addMovieToInventory

@Override
public void addMovieToInventory(String title, String genre, int year, int quantity, boolean isbluray) {
    addMovieToInventory(new Movie(title, genre, year, quantity, isbluray));
} // end addMovieToInventory

@Override
public void removeMovieFromInventory(Movie aMovie) {
//remove from store
movieList.remove(aMovie.getTitle());    
} // end removeMovieFromInventory

@Override
public void removeMovieFromInventory(String movieTitle) {
if (movieList.get(movieTitle) == null) 
    System.out.println("No movie found with that title.");
else 
    removeMovieFromInventory(movieList.get(movieTitle));
} // end removeMovieFromInventory

public void listMoviesByType(String aType) {

} // end listMoviesByType

public String getPromoCode() {
return promoCode;   
}

@Override
public void setPromoCode(String promocode) {
this.promoCode = promocode; 
}

public Double getVolumeDiscount() {
return volumeDiscount;  
}

@Override
public void setVolumeDiscount(double percentage) {
this.volumeDiscount = percentage;   
}

public boolean getUnitState() {
    return unitDisabled;
}

@Override
public void setUnitState(boolean unitState) {
unitDisabled = unitState;
}    

@Override
public void setBlurayPrice(Double price) {
    this.priceBluray = price;
}

public double getBluerayPrice() {
    return priceBluray;
}    

@Override
public void setDVDPrice(Double price) {
    this.priceDVD = price;
}

public double getDVDPrice() {
    return priceDVD;
}

@Override
public void setAdminPassword(String password) {
    this.adminPassword = password;
}

@Override
public String getAdminPassword() {
    return adminPassword;
}   

} // end PurpleBox

The User interface:

public interface UserADT {

/**
 * 
 * @param userName user name
 */
public void setUserName(String userName);

/**
 * 
 * @return user name
 */
public String getUserName();  

/**
 * 
 * @param emailAddress email address
 */
public void setEmail(String emailAddress);

/**
 * 
 * @return user's email address
 */
public String getEmail();

/**
 * @param movieTitle search by movie title
 * @return returns movie searched
 */
public HashMap<String, Movie> search(String movieTitle);

/**
 * @param movieName movie to add to shopping cart
 */
public void addMovieToShoppingCart(String movieName);

/**
 * @param index remove movie by index
 * @return returns the movie that was removed from cart
 */
public Movie removeMovieFromShoppingCart(int index);

/**
 * @return current shopping cart
 */
public ArrayList<Movie> viewShoppingCart();

/**
 * removes all movies from shopping cart
 */
public void removeAllMoviesFromShoppingCart();

/**
 * returns available movies
 */
public void seeAvailableMovies();

/**
 * checkout movies in shopping cart
 */
public void checkout();

/**
 * @param movie movie to return to inventory
 */
public void returnMovie(Movie movie);

} // end UserADT

The Admin interface:

public interface AdminADT {

/**
 * @param adminName admin's name
 */

public void setAdminName(String adminName);

/**
 * @return admin's name
 */
public String getAdminName();

/**
 * @param password
 */
public void setPassword(String password);

/**
 * @return return admin's password
 */
public String getPassword();   

/**
 * @param movie movie to add to inventory
 */
public void addMovieToInventory(Movie movie);

/**
 * @param title 
 * @param genre 
 * @param year 
 * @param quantity 
 * @param isbluray 
 */
public void addMovieToInventory(String title, String genre, int year, int quantity, boolean isbluray);

/**
 * @param movie
 */
public void removeMovieFromInventory(Movie movie); 

/**
 * @param movieTitle
 */
public void removeMovieFromInventory(String movieTitle);   

/**
 * @param price DVD price
 */
public void setDVDPrice(Double price);

 /**
 * @param price Blu-Ray price
 */
public void setBlurayPrice(Double price);

/**
 * @param promoCode the promo code
 */
public void setPromoCode(String promoCode);

 /**
 * @param volume volume discount
 */
public void setVolumeDiscount(double volume);

/**
 * @return unit's state, disabled or enabled
 */
public boolean getUnitState();

/**
 * @param unitState if true, disable unit
 */
public void setUnitState(boolean unitState);   

} // end AdminADT
\$\endgroup\$
4
\$\begingroup\$

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<String, Movie> movieList = new HashMap<String, Movie>() {{
    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.

\$\endgroup\$
  • \$\begingroup\$ Thank you for taking the time to write a well detailed response! I've taken your tips (changed doubles to ints, removed unnecessary comments, removed hard coding the Map) and have integrated them into the current version of my project. \$\endgroup\$ – Shapada Feb 20 '15 at 14:34
  • \$\begingroup\$ You are welcome, and welcome to Code Review! \$\endgroup\$ – bazola Feb 20 '15 at 15:04
3
\$\begingroup\$

UserADT

  1. *ADT suffix doesn't give any value to the name
  2. The following methods should be removed because they are too low level for the purpose of the interface

    • public void setUserName(String userName);
    • public String getUserName();
    • public void setEmail(String emailAddress);
    • public String getEmail();

    If you make the fields of your domain object public final (username, email, etc) you can avoid using of getters, also in the most situations is good to avoid mutability of your data, so I think you can remove this setters.

  3. This interface is too complex and should be split into two interfaces with different implementations

    • search/get/add movie which will be more appropriate to be some kind of InMemoryMovieRepository
    • add/remove/etc to shopping cart which will have its own class e.g. InMemoryShoppingCart

AdminADT

  1. Same for *ADT suffix can be removed
  2. Same for setters/getters (90% of the current method definitions)

    If you have something like the movie repository I think that this is redundant and can be deleted

PurpleBox

  1. The name doesn't tell its purpose, maybe something like MovieStore would be more appropriate
  2. You have a lot of fields that are part of one abstraction e.g. userName, userEmail could be in class named User which will have fields just name and email

    This suggestions came on first thought, I hope that it will help you to finish your school project

\$\endgroup\$
  • \$\begingroup\$ If an admin interface isn't needed, then how would I go about creating an application that separates the user and admin privileges. Is the use of getters and setters not the correct way of controlling who can access what data members? Also, when you say create something like a movie repository, could you please elaborate? I really want to learn the correct way of doing things. Thanks for taking the time to help. It's much appreciated. \$\endgroup\$ – Shapada Feb 20 '15 at 14:11
  • \$\begingroup\$ Here is simple example for repository that I found on GitHub. For your purpose I think that you could use regular User for the admin account ( name:admin and password:admin) \$\endgroup\$ – kinetix Feb 20 '15 at 17:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.