The first thing I noticed about your code, is that you don't clearly have a movie category. I did find these methods on Movie after a while
public int getPriceCode() {
return priceCode;
}
public void setPriceCode(int arg) {
priceCode = arg;
}
I believe the getter would better be renamed to getCategory(). I would also completely remove the setter. It's really unlikely for a movie to change categories, so it should only be set during construction.
It might also be worth writing a specific class Category to list all possible categories. An enum would be the most natural here, but this might be awkward with the requirement:
with the ability to add new categories of movies.
If it's no problem to recompile the code with a new movie (as I expect is the case in this example program) then the enum is still the better choice here.
public enum Category {
CHILDREN, REGULAR, NEW_RELEASE;
}
This also changes the Movie class to this:
public class Movie {
private final String title;
private final Category category;
public Movie(String title, Category category) {
this.title = title;
this.category = category;
}
public Category getCategory() {
return category;
}
public String getTitle() {
return title;
}
}
Notice that I also added final to the fields to make it clear that a movie instance is immutable. Immutability is nice to have.
The other thing I didn't expect in your code is that the Customer class is responsible for calculating the "statement".
If I wanted to add a new category to the code, I wouldn't expect that I need to change the Customer class as well.
There are 2 major ways to solve this smell. Either put the "statement" calculations into a specialised class. Or add category specific calculations to that category.
Let's take a look into the second option since this might not be trivial if you're not used to working with enums in java.
I would implement it like this:
public enum Category {
CHILDREN {
@Override
public int calculateRent(int daysRented) {
int result = 75;
if (daysRented > 3)
result += (daysRented - 3) * 75;
return result;
}
}, REGULAR {
@Override
public int calculateRent(int daysRented) {
int result = 100;
if (daysRented > 2)
result += (daysRented - 2) * 75;
return result;
}
}, NEW_RELEASE {
@Override
public int calculateRent(int daysRented) {
return daysRented*150;
}
};
public abstract int calculateRent(int daysRented);
}
This way, we can simplify the statement() in Customer to:
public String statement() {
double totalAmount = 0;
int frequentRenterPoints = 0;
String returnVal = "Rental record for : " + this.getName() + ".....\n";
for(Rental rental : rentals){
double thisAmount = rental.getMovie().getCategory().calculateRent(rental.getDaysRented());
frequentRenterPoints ++;
if ((rental.getMovie().getCategory() == Category.NEW_RELEASE) && rental.getDaysRented() > 1) {
frequentRenterPoints ++;
}
returnVal += "\t" + rental.getMovie().getTitle() + ": " + thisAmount + "\n";
totalAmount += thisAmount;
}
returnVal += "Amount owed is: " + totalAmount + "\n";
returnVal += "You have earned " + frequentRenterPoints + " frequent renter points";
return returnVal;
}
With this change, adding a new Category only changes a single class (the enum).
Some other things to point out that I improved in this method:
foreach
Since java 5 I believe, there's a specialised way to loop over all itmes in a collection. The syntax here is for(Rental rental : rentals) which reads as "for each rental in rentals".
meaningful names
I renamed your each variable to rental. If you read this later on in the method it's more clear that it's the rental you're talking about.
comments are clutter
Comments are rarely useful. A comment should never be added to say what some line of code does. The only exception being a javadoc for methods (because you usually look at the javadoc from outside the class if you're interested in what that method does that it's name doesn't already tell you).
If you really need a comment to be able to understand some line(s) of code, you should either rename variables/methods in that line or extract a method with a name that clearly says what happens.
Always put {}
You should put curly brackets after each for/if/else/...
This is to prevent really difficult to debug mistakes like this:
if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE) && each.getDaysRented() > 1)
System.out.println("Added bonus frequenterpoints");
frequentRenterPoints ++;
All I did was add a print to see if anyone had bonus points added. But this innocent looking change cause EVERYONE to get a bonus point independent of the if check.
Most IDE's can be configured to automatically add the braces for you.
edit: making rentalpoints dependant on category as well.
Since the ammount of rental points depends on the category too, let's add another method in that enum. Since it only changes for a new release, I would add a default implementation that returns 1 and override this in the NEW_RELEASE. The new Category enum now looks like this:
public enum Category {
CHILDREN {
@Override
public int calculateRent(int daysRented) {
int result = 75;
if (daysRented > 3)
result += (daysRented - 3) * 75;
return result;
}
}, REGULAR {
@Override
public int calculateRent(int daysRented) {
int result = 100;
if (daysRented > 2)
result += (daysRented - 2) * 75;
return result;
}
}, NEW_RELEASE {
@Override
public int calculateRent(int daysRented) {
return daysRented*150;
}
@Override
public int calculateFrequentRenterPoints(int daysRented) {
if(daysRented > 1){
return 2;
} else {
return 1;
}
}
};
public abstract int calculateRent(int daysRented);
public int calculateFrequentRenterPoints(int daysRented){
return 1;
}
}
Now there's one more principle that I too have to get used to. It's the tell, don't ask. This is violated inside the statement method.
What we're doing is asking a rental for it's movie, then asking the movie for it's category and then once more asking the rental for the number of days rented. And then do the same asking for the frequent renter points.
This is too much asking. It would make more sense to just tell rental to calculate the price and then return the result. This way we only ask for one thing.
If we move those 2 methods to the Rental class istead, then our statement method will turn to this:
public String statement() {
double totalAmount = 0;
int frequentRenterPoints = 0;
String returnVal = "Rental record for : " + this.getName() + ".....\n";
for(Rental rental : rentals){
frequentRenterPoints += rental.calculateFrequentMoviePoints();
double thisAmount = rental.calculateRent();
returnVal += "\t" + rental.getMovie().getTitle() + ": " + thisAmount + "\n";
totalAmount += thisAmount;
}
returnVal += "Amount owed is: " + totalAmount + "\n";
returnVal += "You have earned " + frequentRenterPoints + " frequent renter points";
return returnVal;
}
For completeness sake here's the new Rental class as well:
public class Rental {
private Movie movie;
private int daysRented;
public Rental(Movie movie, int daysRented) {
this.movie = movie;
this.daysRented = daysRented;
}
public int getDaysRented() {
return daysRented;
}
public Movie getMovie() {
return movie;
}
public int calculateRent() {
return movie.getCategory().calculateRent(daysRented);
}
public int calculateFrequentMoviePoints() {
return movie.getCategory().calculateFrequentRenterPoints(daysRented);
}
}
This actually makes a lot more sense. A Customer only knows that it can ask a rental for the price and points, it doesn't need to know how those are calculated.
A Rental needs to know who to ask for the actual price or points, and has the information needed to calculate those (in this case, the number of days rented).
A Category contains category - specific calculations. Adding a new category means only implementing a new field in the enum. No other classes need to be changed.
Your idea of putting it into subclasses of movie (eliminating the Category class altogether) would work too. It's even better for the Rental class since it doesn't need to know that there exist such a thing as different categories of movies.
The only reason I would not go for this solution is the reason to change a Movie class. If your name is just an identifier for the movie, and no other info about the movie is stored in that class than I would completely agree with this being a good solution. Except that I would change the name to something else. (If only I could find a decent name for that specific concept ... naming things is hard). Let's give it the not so awesome name RentableMovie for now.
That would mean a Customer has a bunch of Rentals.
Each of those rentals has a number of days and a RentableMovie.
That RentableMovie contains specific ways of calculating the price and points. It only contains 1 identifier for the movie. Or we could introduce a new MovieInformation class so that the RentableMovie can point to all information about the movie, without knowing what exactly is in it.
This would work as well :)
So you can choose what feels best to you.
