Refactoring exercise - Programming ≈ Fun

I’ve just read an excellent article about refactoring rcat in Rubies in the Rough. If you are not subscribed you should be… I couldn’t resist to take a swing at it and refactor Display class since thats where most of the action is happening. In one line rcat is supposed to behave as Unix cat command for some basic cases.

This is original implementation, great stuff from the Practicing Ruby.

(display_original.rb) download

class Display
  def initialize(params)
    @line_numbering_style   = params[:line_numbering_style]
    @squeeze_extra_newlines = params[:squeeze_extra_newlines]
  end

  def render(data)
    @line_number = 1

    lines = data.lines
    loop { render_line(lines) }
  end

  private

  attr_reader :line_numbering_style, :squeeze_extra_newlines, :line_number

  def render_line(lines)
    current_line = lines.next
    current_line_is_blank = current_line.chomp.empty?

    case line_numbering_style
    when :all_lines
      print_labeled_line(current_line)
      increment_line_number
    when :significant_lines
      if current_line_is_blank
        print_unlabeled_line(current_line)
        # skip incrementing line number, want to only count significant lines
      else
        print_labeled_line(current_line)
        increment_line_number
      end
    else
      print_unlabeled_line(current_line)
      increment_line_number
    end

    if squeeze_extra_newlines && current_line_is_blank
       lines.next while lines.peek.chomp.empty?
    end
  end

  def print_labeled_line(line)
    print "#{line_number.to_s.rjust(6)}\t#{line}"
  end

  def print_unlabeled_line(line)
    print line
  end

  def increment_line_number
    @line_number += 1
  end
end

I’ve liked @JEG2 idea about not using loop and about trimming down render_line method a bit. I did not like the use of additional object that will carry state. This is what I’ve ended up with:

(display.rb) download

module RCat
  class Display
    def initialize(params)
      @line_numbering_style   = params[:line_numbering_style]
      @squeeze_extra_newlines = params[:squeeze_extra_newlines]
    end

    def render(data)
      reset_line_number_counter

      data.reduce(false) do |previous_is_blank, line|
        next if squeeze_extra_newlines &&
                previous_is_blank && blank?(line)
        print format(line)
        blank?(line)
      end
    end

    private

    attr_reader :line_numbering_style,
                :squeeze_extra_newlines,
                :line_number

    def format(line)
      numbered?(line) ? numbered(line) : line
    end

    def numbered?(line)
      line_numbering_style  == :all_lines ||
      (line_numbering_style == :significant_lines && !blank?(line))
    end

    def blank?(line)
      line.chomp.empty?
    end

    def reset_line_number_counter
      @line_number = 0
    end

    def numbered(line)
      @line_number += 1
      "%6d\t%s" % [@line_number, line]
    end

  end
end

The code is so self explanatory :) that I don’t need to say anything except maybe that I am in reduce-happy-phase as visible from above.

Afterwards I’ve played a bit with removing that boolean logic from reduce and also with extracting the counter logic. This is the result:

(display_ii.rb) download

class Display
  def initialize(params)
    @line_numbering_style   = params[:line_numbering_style]
    @squeeze_extra_newlines = params[:squeeze_extra_newlines]
  end

  def render(data)
    counter = Counter.new

    data.reduce(nil) do |previous, current|
      next if squeeze_extra_newlines &&
              blank?(previous) && blank?(current)
      print numbered?(current) ? numerize(current, counter) : current
      current
    end
  end

  private

  attr_reader :line_numbering_style,
              :squeeze_extra_newlines,
              :line_number

  def numbered?(line)
    line_numbering_style  == :all_lines || significant?(line)
  end

  def significant?(line)
    line_numbering_style == :significant_lines && !blank?(line)
  end

  def blank?(line)
    line.nil? ? false : line.chomp.empty?
  end

  def numerize(line, counter)
    counter.increment
    "%6d\t%s" % [counter.current, line]
  end

end

class Counter
  attr_reader :current
  def initialize() @current  = 0 end
  def increment()  @current += 1 end
end

I like the last example the best because domain is really starting to show. I get to talk about being blank or significant etc… In theory .each_cons(2) seems to be better suited than .reduce for the job of “sliding” trough lines, but I am unable to make it work that way. (@ryanlecompte suggested it since I completely forgot about that method.)

Also it looks to me as if I need “Line” class that would have blank?, significant? and numbered? methods. Probably also “Configuration” class for line style. But that is something I would do if things begin to get complex, right now there is no need. I am pretty pleased on how the code is communicating what it is suppose to do.