Oleg Andreev

Consider the following code (Ruby):

class Customer
  attr_reader :name

  def initialize(name)
    @name = name
    @rentals = []
  end

  def add_rental(arg)
    @rentals << arg
  end

  def statement
    total_amount, frequent_renter_points = 0, 0
    result = "Rental Record for #{@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
        this_amount += 1.5
        this_amount += (element.days_rented - 3) * 1.5 if element.days_rented > 3
      end

      # add frequent renter points
      frequent_renter_points += 1
      # add bonus for a two day new release rental
      frequent_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 + "\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 #{frequent_renter_points.to_s} frequent renter points"
    result
  end
end

Many code for a single routine, huh? Let's refactor it. See one-line comments like "# show figures for this rental"? These are signs you need outsource a code to another method. So, we do it.

class Rental
  ...
  def determine_amount  
      this_amount = 0
      case movie.price_code
      when Movie::REGULAR
        this_amount += 2
        this_amount += (days_rented - 2) * 1.5 if days_rented > 2
      when Movie::NEW_RELEASE
        this_amount += days_rented * 3
      when Movie::CHILDRENS
        this_amount += 1.5
        this_amount += (days_rented - 3) * 1.5 if days_rented > 3
      end
  end

  def bonus
     if movie.price_code == Movie.NEW_RELEASE && days_rented > 1
       1
     else
       0
     end
  end

  def frequent_renter_points; 1; end
end
class Customer
  attr_reader :name

  def initialize(name)
    @name = name
    @rentals = []
  end

  def add_rental(arg)
    @rentals << arg
  end

  def statement
    total_amount, frequent_renter_points = 0, 0
    result = "Rental Record for #{@name}\n"
    @rentals.each do |element|
      this_amount = element.determine_amount
      
      frequent_renter_points += element.frequent_renter_points
      frequent_renter_points += element.bonus

      # show figures for this rental
      result += "\t" + element.movie.title + "\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 #{frequent_renter_points.to_s} frequent renter points"
    result
  end
end

First, we have thrown away operations involving calls only to "element". Not exactly away, but into element's class - Rental. We have preserved comments over operations involving routine-local variables.

I hope to describe this more detailed to make a tool like tidy/beautifier which could cope with this somehow automatically.