I got this refactor challenge for a simple ruby script that has 3 objects and produces a statement. In effect we have a Movie object, a Rental object and a Customer object. The purpose of the script is to print out a statement with the customer’s name, order, amount due and frequent renter points.
The code that I received for refactoring has syntax and other errors, which after a quick email exchange I found out where put there intentionally. So it’s more of a fix the code challenge rather than a refactor job, but I did both. Underneath you’ll see the snippets of the code I got:
class Movie
CHILDRENS_MOVIE = 2
REGULAR = 0
NEW_RELEASE = 1
attr_reader :title_for_movie
attr_accessor :price_code
def initialize(title_for_movie, price_code)
@title_for_movie, @price_code = title_for_movie, price_code
end
end
class Rental
attr_reader :movie, :days_rented
def initialize(movie, days_rented)
@movie, @days_rented = movie, days_rented
end
end
And the problem object
class CustomerClass
attr_reader :customer_name
def initialize(customer_name)
@customer_name = customer_name
@rentals ||= []
end
def add_rental_object_to_list(arg)
x = 7
@rentals << arg
end
def statement
total_amount, freqent_renter_points = 0, 0
result = "Rental Record for #{@customer_name}\n"
@rentals.each do |_element|
this_amount = 0
# determine amounts for each line
case _element.movie.price_code
when Movie::REGULAR
this_amount += 2; this_amount += (_element.days_rented - 2) * 1.5 if _element.days_rented > 2
when Movie::NEW_RELEASE
this_amount += _element.days_rented * 3
when Movie::CHILDRENS_MOVIE
this_amount += 1.5; this_amount += (_element.days_rented - 3) * 1.5 if _element.days_rented > 3
end
# add frequent renter points
freqent_renter_points += 1
# add bonus for a two day new release rental
freqent_renter_points += 1 if _element.movie.price_code == Movie.NEW_RELEASE && _element.days_rented > 1
# show figures for this rental
result += "\t" + _element.movie.title_for_movie + "\t" + this_amount.to_s + "\n"
total_amount += this_amount
end
# add footer lines
result += "Amount owed is #{total_amount.to_s}\n"
result += "You earned #{freqent_renter_points.to_s} frequent renter points"
result
end
end
At first glance we can see that we are dealing with a renting operation in which a customer chooses one or more movies to rent and gets billed accordingly.
We can see from a glance that the customer class is pretty much a mess. We have an unnecessary variable (x = 7) , we have bad variable and method names “add_rental_object_to_list(arg)” , an improper constant call “Movie.NEW_RELEASE” , a couple of typos and worst still:
# We have code comments # Now for those of you that are new to my blog # or are simply curious as to why I am so opposed to comments # the reason is twofold and boils down to this: # # The existence of code comments is proof of hard to understand code # and hard to understand code logic. # If you need to add comments to your code # to make it readable, chances are you need to refactor more. # # And secondly, I have yet to meet a developer that once tasked with # making changes to commented code will take the time to # improve and change the comments so that they now reflect the new # code reality. So what happens is that by the time the third developer # comes up to the code, the comments say one story because they are outdated # and the code tells a whole other story.
Now, let’s move on to how I made the code, and look under the hood a bit
class Movie
CHILDRENS_MOVIE = 2
REGULAR = 0
NEW_RELEASE = 1
attr_reader :title_for_movie
attr_accessor :price_code
def initialize(title_for_movie: title_for_movie, price_code: price_code)
@title_for_movie = title_for_movie
@price_code = price_code
end
end
class Rental
attr_accessor :movie, :days_rented
def initialize(movie: movie, days_rented: days_rented)
@movie = movie
@days_rented = days_rented
end
end
class Customer
attr_accessor :customer_name
def initialize(customer_name: customer_name)
@customer_name = customer_name
end
end
Now you must be wondering ‘what happened to all the code logic from the customer class?’
Well I extracted out all that business logic and placed it all inside an Interactor. I did this because all business logic should be extracted and separated from ruby/rails logic. I like to keep my public objects as plain and as dumb as humanly possible. I guess you have heard of the paradigm, ‘slim model fat controller’ or others put it as ‘slim controller fat model’. I for one prefer to keep all my main public objects as slim and as dumb as possible.
I know what you’re thinking, ‘model? controller? what does rails logic have to do with a ruby script?’ and you’d be right. But I am putting out an idea. and as I’ve said it before in a previous post the business logic of your app should be extracted and tested separately. This has many advantages including: a more modular design, easier to read code, easier to extend and maintain, easier to refactor etc.
So first I’ll put the tests
require 'spec_helper'
require_relative '../interactors/set_price_and_bonus_points'
require_relative '../bin/rental'
require_relative '../bin/movie'
require_relative '../bin/customer'
RSpec.describe Interactors::SetPriceAndBonusPoints do
let(:new_movie) {Movie.new(title_for_movie: "Jaws", price_code: Movie::NEW_RELEASE) }
let(:kids_movie) {Movie.new(title_for_movie: "Incredibles 2", price_code: Movie::CHILDRENS_MOVIE)}
let(:regular_movie) {Movie.new(title_for_movie: "Avengers", price_code: Movie::REGULAR)}
let(:new_rental) {Rental.new(movie: new_movie, days_rented: 2)}
let(:kids_rental) {Rental.new(movie: kids_movie, days_rented: 4)}
let(:regular_rental) {Rental.new(movie: regular_movie, days_rented: 2)}
let(:customer) {Customer.new(customer_name: "Robert")}
describe 'calculates and prints out a statement' do
it 'returns 0 amount and 0 bonus points if no movie is rented' do
expect(Interactors::SetPriceAndBonusPoints.call([], customer)).to eq "Rental record for Robert\nAmount owed is $0\nYou earned 0 frequent renter points"
end
it 'can calculate a price and bonus points awarded for a new release movie rented for 2 days' do
expect(Interactors::SetPriceAndBonusPoints.call([new_rental], customer)).to eq "Rental record for Robert\nAmount owed is $6\nYou earned 2 frequent renter points"
end
it 'can calculate a price and bonus points awarded for a kids movie rented for 4 days' do
expect(Interactors::SetPriceAndBonusPoints.call([kids_rental], customer)).to eq "Rental record for Robert\nAmount owed is $1.5\nYou earned 1 frequent renter points"
end
it 'can calculate a price and bonus points awarded for a kids movie rented for 2 days, and it should cost just as much as for 4 days' do
new_kids_rental = Rental.new(movie: kids_movie, days_rented: 1)
expect(Interactors::SetPriceAndBonusPoints.call([new_kids_rental], customer)).to eq "Rental record for Robert\nAmount owed is $1.5\nYou earned 1 frequent renter points"
end
it 'can calculate a price and bonus points awarded for a regular movie rented for 2 days' do
expect(Interactors::SetPriceAndBonusPoints.call([regular_rental], customer)).to eq "Rental record for Robert\nAmount owed is $2\nYou earned 1 frequent renter points"
end
it 'can calculate multiple rental bonus points and a prices added up' do
expect(Interactors::SetPriceAndBonusPoints.call([new_rental, kids_rental, regular_rental], customer)).to eq "Rental record for Robert\nAmount owed is $9.5\nYou earned 4 frequent renter points"
end
end
end
And here is the actual code
module Interactors
SetPriceAndBonusPoints = lambda { |rentals, customer|
amount_owed = 0
frequent_renter_points = 0
result = "Rental record for #{customer.customer_name}\n"
rentals.each do |rental|
individual_film_amount = 0
if rental.movie.price_code == Movie::NEW_RELEASE
individual_film_amount += rental.days_rented * 3
rental.days_rented > 1 ? frequent_renter_points += 2 : frequent_renter_points += 1
elsif rental.movie.price_code == Movie::REGULAR
rental.days_rented > 2 ? individual_film_amount += (rental.days_rented - 2) * 1.5 : individual_film_amount += 2
frequent_renter_points += 1
else rental.movie.price_code == Movie::CHILDRENS_MOVIE
rental.days_rented > 3 ? individual_film_amount += (rental.days_rented - 3) * 1.5 : individual_film_amount += 1.5
frequent_renter_points += 1
end
result += "\t" + rental.movie.title_for_movie + "\t" + "$" + individual_film_amount.to_s + "\n"
amount_owed += individual_film_amount
end
result += "Amount owed is $#{amount_owed}\n"
result += "You earned #{frequent_renter_points} frequent renter points"
result
}
end
I extracted all the logic in this lamba function that you can conveniently call as such:
Interactors::SetPriceAndBonusPoints.call([new_rental, kids_rental], customer)
and it will print out the following:
Rental record for Robert
Jaws $9
Incredibles 2 $1.5
Avengers $1.5
Amount owed is $12.0
You earned 4 frequent renter points
You can find the code repo here and also other code snippets and scripts.
Have a splendid week!