3
\$\begingroup\$

The condition for refactoring is:

Define 2 new rules with respect to rental charges that are applicable and with the ability to add new categories of movies.

Ex: Rental amount for children's movie for more than 2 days should be 100 from 3rd day onwards.

I have 3 classes (Children, Movie and Rental). I need suggestions on code smell and refactoring it.

Customer.java

import java.util.ArrayList;
import java.util.Iterator;

public class Customer {

    private String name;
    private ArrayList rentals = new ArrayList();

    public Customer(String name) {
        this.name = name;
    }

    public void addRental(Rental arg) {
        rentals.add(arg);
    }

    public String getName() {
        return name;
    }

     public String statement() { 

        double totalAmount = 0;
        int frequentRenterPoints = 0;

        String returnVal = "Rental record for : " + this.getName() + ".....\n";

        Iterator iter = rentals.iterator();

        while(iter.hasNext()) {
            double thisAmount = 0;
            Rental each = (Rental) iter.next();
            //determine amounts for each line
            switch (each.getMovie().getPriceCode()) {
                case Movie.REGULAR: thisAmount += 100;
                if (each.getDaysRented() > 2)
                    thisAmount += (each.getDaysRented() - 2) * 75; 
                break;

                case Movie.NEW_RELEASE:
                thisAmount += each.getDaysRented() * 150; 
                break;

                case Movie.CHILDREN:
                thisAmount += 75;
                if (each.getDaysRented() > 3)
                thisAmount += (each.getDaysRented() - 3) * 75; 
                break;
            }


            // add frequent renter points
            frequentRenterPoints ++;
            // add bonus for a two day new release rental
            if ((each.getMovie().getPriceCode() == Movie.NEW_RELEASE) && each.getDaysRented() > 1) 
                frequentRenterPoints ++;

            //show figures for this rental
            returnVal += "\t" + each.getMovie().getTitle() + ": " + thisAmount + "\n";
            totalAmount += thisAmount;

        }

        //print footer
        returnVal += "Amount owed is: " + totalAmount + "\n";
        returnVal += "You have earned " + frequentRenterPoints + " frequent renter points";

        return returnVal;
    }
}

Movie.java

public class Movie {

    public static final int CHILDREN = 2;
    public static final int REGULAR = 0;
    public static final int NEW_RELEASE = 1;

    private String title;
    private int priceCode;

    public Movie(String title, int priceCode) {
        this.title = title;
        this.priceCode = priceCode;
    }

    public int getPriceCode() {
        return priceCode;
    }

    public void setPriceCode(int arg) {
        priceCode = arg;
    }

    public String getTitle() {
        return title;
    }
}

Rental.java

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;
    }
}
\$\endgroup\$
  • \$\begingroup\$ I have tried making a class movie as an Abstract class and added 2 abstract methods to calculate fare and points and then created concrete classes of movies (children, new) by extending the movie class. can this remove all the code smells ? \$\endgroup\$ – Rishabh Shukla May 7 '17 at 5:37
1
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Thanks for this nice solution. But I would like to put attention that the frequentRenterPoints are still in customer class and is tightly coupled with it. If we need to introduce renter point for each new movie , it will be difficult to maintain and change the renter calculation logic.** I am not clear that the if the customer class is doing calculation for renterPoints and rent, is it following SIngleResponsibilityPrinciple ?.** I will post my solution also which is somewhat similar to yours. Please suggest changes in that or some best approach. \$\endgroup\$ – Rishabh Shukla May 7 '17 at 14:03
  • 1
    \$\begingroup\$ @Imus: Enums shouldn't provide business logic. It's an enumerated type, it represents a state or a constant value. Adding logic to it does violate the single responsibility pattern imo. It will backfire, when the calculation needs other parameters, for instance "regular customer" or "special offer 10% off rent" or "special offer rent two, rent one for free". I would have used a dedicated "RentCalculationService" or something similar. \$\endgroup\$ – slowy May 8 '17 at 12:08
  • \$\begingroup\$ @Imus: so going with the solution of the subclasses that I have posted and incorporating your suggestion also at the same time, I didn't understand RentableMovie(Movie renmed) 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. Can you please make it more clear. And after this will there be any code smell left in Customer class if I go with the Subclass solution. \$\endgroup\$ – Rishabh Shukla May 8 '17 at 15:32
  • \$\begingroup\$ The subclass itself will work. But think about the major responsibility of those classes. Are they responsible for calculating the rental price and frequent renter points? Or are they responsible for providing detailed information about the movie? If your answer is both, then that's a code smell. In that subclass solution it should be responsible only for the calculations. And at best have a "pointer" to another class that can tell everything about the movie. That way it's not responsible for the details, but it can point you to someone else who does have those details. \$\endgroup\$ – Imus May 10 '17 at 18:41
1
\$\begingroup\$

private ArrayList rentals = new ArrayList();

The list contains Rental objects so you should make it generic. No more cast needed!

private List<Rental> rentals = new ArrayList<>();

ps: I changed the type from ArrayList (implementation) to List (interface) for maintainability purpose, see Interface-based programming.

\$\endgroup\$

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.