June 10, 2014

Java to Clojure: "Refactoring" refactored functionally

I was searching for a good example to illustrate some properties of refactoring object-oriented code into a more functional style, and I came across this example from Martin Fowler’s Refactoring. It describes the process of refactoring code for a video store. It’s a nice little example with a useful narrative to go along with it.

I don’t own the book, and don’t make “buy a book to make a post” revenue from this blog, so, I snatched Uncle Bob’s version of the code and got to work rewriting it in Clojure. (Ed. 2015: Martin Fowler pointed out to me that the example in his book differs significantly from this one.)

I’ve provided the finished versions in both Java and Clojure on github. All Java examples are republished without permission, by the way, just so we’re clear.

The Java code defines several variants of a base class Movie: RegularMovie, NewReleaseMovie, and ChildrensMovie. Each of these has a slightly different fee structure and rewards a different amount of frequent rental points. Here’s Movie in Java:


abstract class Movie
{
    private String title;

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

    public String getTitle () {
        return title;
    }

    public abstract double determineAmount(int daysRented);

    public abstract int determineFrequentRenterPoints(int daysRented);
}

I’ve chosen in the rewrite to represent Movie as a protocol. Protocols are a whole lot like interfaces, except that implementations of protocols can be specified at runtime. You can even use protocols to bolt new methods onto existing java classes! But, we don’t need to do that here. We just need to specify this protocol so that our different movie types can implement it.

Here’s the Movie protocol:

(defprotocol Movie
  (determine-amount [this days-rented])
  (determine-frequent-renter-points [this days-rented]))

This is pretty similar, with the exception that the Movie protocol doesn’t care about the title getter (for reasons that will be explained shortly). Here are the concrete implementations of the movie types in Java:

class ChildrensMovie extends Movie {
    public ChildrensMovie(String title) {
        super(title);
    }

    public double determineAmount(int daysRented) {
        double thisAmount = 1.5;
        if (daysRented > 3)
            thisAmount += (daysRented - 3) * 1.5;

        return thisAmount;
    }

    public int determineFrequentRenterPoints(int daysRented) {
        return 1;
    }
}


class RegularMovie extends Movie {
    public RegularMovie(String title) {
        super(title);
    }

    public double determineAmount(int daysRented) {
        double thisAmount = 2;
        if (daysRented > 2)
            thisAmount += (daysRented - 2) * 1.5;

        return thisAmount;
    }

    public int determineFrequentRenterPoints(int daysRented) {
        return 1;
    }
}

class NewReleaseMovie extends Movie {
    public NewReleaseMovie(String title) {
        super(title);
    }

    public double determineAmount(int daysRented) {
        return daysRented * 3.0;
    }

    public int determineFrequentRenterPoints(int daysRented) {
        return (daysRented > 1) ? 2 : 1;
    }
}

These are very well-behaved classes. Note that their methods don’t depend on their fields; each class defines the different computations for amount and renter points, which solves the problem of dispatch based on rental type. So I don’t feel the need to change this too much.

For the three concrete types, I’m using records. Records are objects that can be manipulated like hash-maps, but can implement methods from interfaces and protocols too. Here are my implementations:


(defrecord ChildrensMovie [title]
  Movie
  (determine-amount [this days-rented]
    (+ 1.5
       (if (> days-rented 3)
         (* (- days-rented 3) 1.5)
         0.0)))
  (determine-frequent-renter-points [this _] 1))

(defrecord RegularMovie [title]
  Movie
  (determine-amount [this days-rented]
    (+ 2
       (if (> days-rented 2)
         (* (- days-rented 2) 1.5)
         0.0)))
  (determine-frequent-renter-points [this _] 1))

(defrecord NewReleaseMovie [title]
  Movie
  (determine-amount [this days-rented]
    (* days-rented 3.0))
  (determine-frequent-renter-points [this days-rented]
    (if (> days-rented 1) 2 1)))

I declare each as a record with one field, title, which can be accessed in just the same way as if I created a map: (:title movie). Since records are immutable, just like other Clojure data structures, there’s no benefit to creating a getter.

Each record contains implementations for the Movie protocol. These “methods” act just like functions, since again, we can’t mutate our records (and we wouldn’t want to). Later, when we call them, you’ll see they look just like regular function calls.

The Java code has a container class called Rental, which combines a movie and a count of days. It’s a very thin wrapper around a Movie object:

class Rental
{
    private Movie movie;
    private int daysRented;

    public Rental (Movie movie, int daysRented) {
        this.movie      = movie;
        this.daysRented = daysRented;
    }

    public String getTitle() {
        return movie.getTitle();
    }

    public double determineAmount() {
        return movie.determineAmount(daysRented);
    }

    public int determineFrequentRenterPoints() {
        return movie.determineFrequentRenterPoints(daysRented);
    }
}

Since there’s only the one type of rental, we don’t have a dispatch problem, so I didn’t feel the need to create a protocol. However, I did create a record, just to make the situation explicit:

(defrecord Rental [movie days-rented])

Finally, the Java code defines a RentalStatement object, where all the logic of producing a printable invoice for a series of rentals lives. Here’s where things are a little stateful; the RentalStatement object contains a mutable list of Rentals and some counters used when adding up the amount and points. I find this design mixes data and functionality a bit too much:


class RentalStatement {
    private String name;
    private List<Rental> rentals = new ArrayList<Rental>();
    private double totalAmount;
    private int frequentRenterPoints;

    public RentalStatement(String customerName) {
        this.name = customerName;
    }

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

    public String makeRentalStatement() {
        clearTotals();
        return makeHeader() + makeRentalLines() + makeSummary();
    }

    private void clearTotals() {
        totalAmount = 0;
        frequentRenterPoints = 0;
    }

    private String makeHeader() {
        return "Rental Record for " + getName() + "\n";
    }

    private String makeRentalLines() {
        String rentalLines = "";

        for (Rental rental : rentals)
            rentalLines += makeRentalLine(rental);

        return rentalLines;
    }

    private String makeRentalLine(Rental rental) {
        double thisAmount = rental.determineAmount();
        frequentRenterPoints += rental.determineFrequentRenterPoints();
        totalAmount += thisAmount;

        return formatRentalLine(rental, thisAmount);
    }

    private String formatRentalLine(Rental rental, double thisAmount) {
        return "\t" + rental.getTitle() + "\t" + thisAmount + "\n";
    }

    private String makeSummary() {
        return "You owed " + totalAmount + "\n" +
            "You earned " + frequentRenterPoints +
            " frequent renter points\n";
    }

    public String getName() {
        return name;
    }

    public double getAmountOwed() {
        return totalAmount;
    }

    public int getFrequentRenterPoints() {
        return frequentRenterPoints;
    }
}

This also requires no multiple dispatch, so we don’t need to make a protocol.

To rewrite this, I decided to work backwards from the desired result. I first decided that a statement could just be data, with a function for printing it:

(defrecord Statement [lines total points])
(defrecord StatementLine [title amount])

(defn format-statement [customer-name statement]
  (str "Rental Record for " customer-name "\n"
       (apply str (map #(str "\t" (:title %) "\t" (:amount %) "\n") (:lines statement)))
       "You owed " (:total statement) "\n" 
       "You earned " (:points statement) " frequent renter points\n"))

Then, it’s simply necessary to build a Statement from a sequence of Rentals. Remember, whenever you need to combine a bunch of data into one thing, you probably want to reduce.


(defn add-to-statement [statement {movie :movie days-rented :days-rented}]
    (assoc statement
           :lines (conj (:lines statement)
                        (StatementLine. (:title movie)
                                            (determine-amount movie days-rented)))
           :total (+ (:total statement)
                     (determine-amount movie days-rented))

           :points (+ (:points statement)
                      (determine-frequent-renter-points movie days-rented))))

(defn make-statement [rentals]
  (reduce add-to-statement (Statement. [] 0.0 0) rentals))

The add-to-statement function accepts a Statement and a Rental and returns another Statement with the rental added on, and make-statement just reduces all the rentals over an empty Statement and returns the result.

Finally, there’s the problem of using all this code. To create a statement and print it in Java looks like this:

public class videostore {
    public static void main(String[] args){
        RentalStatement rs = new RentalStatement("Tom");
        rs.addRental(new Rental(new RegularMovie("Reg1"), 1));
        rs.addRental(new Rental(new NewReleaseMovie("NR2"), 2));
        rs.addRental(new Rental(new ChildrensMovie("Childrens3"), 3));
        System.out.println(rs.makeRentalStatement());
    }
}

Stateful, but straightforward, in true Java style. Our Clojure version ends up thusly:

(defn -main []
  (let [rentals [(Rental. (RegularMovie. "Reg1") 1)
                 (Rental. (NewReleaseMovie. "NR2") 2)
                 (Rental. (ChildrensMovie. "Childrens3") 3)]]
    (->> rentals
         (make-statement)
         (format-statement "Tom")
         (print))))

Take a list of rentals, make a statement, format the statement for “Tom”, and print it. I used ->> so that these things can be represented in this logical order, instead of (print (format-statement "Tom" (make-statement rentals)))

All told, the Java version occupies 174 lines, while Clojure version takes up 61. The reduction comes from a few places:

Clojure’s immutable records saved us from writing getters and setters.
Clojure style is more compact, and often does more on a given line than java.
Clojure’s collection functions saved us from some loops.

As for which is more readable, there’s an argument either way. I personally find the statement construction in both versions a bit intertwined. I’m tempted to give it to Clojure just because there’s less jumping around between methods to follow, but I also have more experience reading Clojure than the average developer. What do you think?

Adam Bard

Makes Websites

Java to Clojure: "Refactoring" refactored functionally