This blog note is next in our cycle aimed at less-experienced developers. This time we will start with real-life code, that I’ve found in one of our projects. Through a series of steps, we will refactor it to excellent object structure, separated from other parts of the application.

Introduction

Our example here is an internal application for one-time, anonymous voting. The basic idea is to distribute printed tokens to users and let them vote using it as one time codes. One of the features is, of course, exporting the results.

So, let’s take a look at our starting code (connected with exports):

class Poll < ActiveRecord::Base
  has_many :questions, dependent: :delete_all
  has_many :answers, through: :questions

  def export_results
    headers = "TOKEN,"
    questions.order(:id).each do |que|
      que.answers.order(:id).each do |answ|
        headers << "\"#{answ.body}\","
      end
    end
    headers.gsub!(/,$/, "\n")

    lines = [headers]

    votes.registered.map do |vt|
      results = vt.token + ","
      questions.order(:id).each do |que|
        que.answers.order(:id).each do |answ|
          results << (answ.id.in?(vt.registered_answer_ids) ? "1," : "0,")
        end
      end
      lines << results.gsub(/,$/, "\n")
    end
    lines.join
  end
end

¡Ay caramba!

via GIPHY

A lot is going on in here. Starting with strange variable names, then reinventing the wheel, finishing with logic inside model which doesn’t belong there.

Fortunately, we have some specs:

require "rails_helper"

RSpec.describe Poll, type: :model do
  describe "#export_results" do
    subject { create :poll, :without_tokens, questions: [question1, question2] }

    let(:answer1) { create :answer, body: "ANSWER1" }
    let(:answer2) { create :answer, body: "ANSWER2" }
    let(:question1) { create :question, answers: [answer1, answer2], max_answers: 1 }

    let(:answer3) { create :answer, body: "ANSWER3" }
    let(:answer4) { create :answer, body: "ANSWER4" }
    let(:answer5) { create :answer, body: "ANSWER5" }
    let(:question2) { create :question, answers: [answer3, answer4, answer5], max_answers: 2 }

    before do
      subject
      create :vote, token: "token1", answered: [answer1.id, answer3.id]
      create :vote,  token: "token2", answered: [answer1.id, answer3.id, answer4.id]
      create :vote,  token: "token3", answered: [answer1.id, answer4.id, answer5.id]
      create :vote,  token: "token4", answered: [answer2.id, answer3.id, answer5.id]
      create :vote,  token: "token5", answered: [answer2.id, answer5.id]
    end

    let(:result) { File.read "spec/fixtures/export_results1.txt" }

    it "loads the file" do
      expect(subject.export_results).to eq result
    end
  end
end

and a fixture file (let’s move factories out of scope for this note):

TOKEN,"ANSWER1","ANSWER2","ANSWER3","ANSWER4","ANSWER5"
token1,1,0,1,0,0
token2,1,0,1,1,0
token3,1,0,0,1,1
token4,0,1,1,0,1
token5,0,1,0,0,1

It isn’t much, but this will work as a sanity check for us. And these tests are passing:

$ rspec spec/models/poll_spec.rb
.

Finished in 0.56384 seconds (files took 5.26 seconds to load)
1 example, 0 failures

Start refactoring

Let’s start by creating appropriate service and moving the entire method there:

# app/services/polls/export_results_service.rb
module Polls
  class ExportResultsService
    def initialize(poll:)
      @poll = poll
    end

    def export_results
      headers = "TOKEN,"
      questions.order(:id).each do |que|
        que.answers.order(:id).each do |answ|
          headers << "\"#{answ.body}\","
        end
      end
      headers.gsub!(/,$/, "\n")

      lines = [headers]

      votes.registered.map do |vt|
        results = vt.token + ","
        questions.order(:id).each do |que|
          que.answers.order(:id).each do |answ|
            results << (answ.id.in?(vt.registered_answer_ids) ? "1," : "0,")
          end
        end
        lines << results.gsub(/,$/, "\n")
      end
      lines.join
    end
  end
end

and delegating the entire call to it from our model:

class Poll < ActiveRecord::Base
  has_many :questions, dependent: :delete_all
  has_many :answers, through: :questions

  delegate :export_results, to: :export_results_service

  private

  def export_results_service
    @export_results_service ||= Polls::ExportResultsService.new(poll: self)
  end
end

and our sanity check…

$ rspec spec/models/poll_spec.rb
F

Failures:

  1) Poll#export_results loads the file
     Failure/Error:
       questions.order(:id).each do |que|
         que.answers.order(:id).each do |answ|
           headers << "\"#{answ.body}\","
         end
       end

     NameError:
       undefined local variable or method `questions' for #<Polls::ExportResultsService:0x007fe51657dfe8>
    (…)

…is failing because simple copy-paste is not enough. Let’s create a private reader for poll and redirect all internal calls to it:

# app/services/polls/export_results_service.rb
module Polls
  class ExportResultsService
    def initialize(poll:)
      @poll = poll
    end

    def export_results
      headers = "TOKEN,"
      poll.questions.order(:id).each do |que|
      # we need to access poll here, instead of implicit receiver
        que.answers.order(:id).each do |answ|
          headers << "\"#{answ.body}\","
        end
      end
      headers.gsub!(/,$/, "\n")

      lines = [headers]

      poll.votes.registered.map do |vt|
        results = vt.token + ","
        poll.questions.order(:id).each do |que|
        # we need to access poll here, instead of implicit receiver
          que.answers.order(:id).each do |answ|
            results << (answ.id.in?(vt.registered_answer_ids) ? "1," : "0,")
          end
        end
        lines << results.gsub(/,$/, "\n")
      end
      lines.join
    end

    private

    attr_reader :poll
  end
end

and our sanity check

$ rspec spec/models/poll_spec.rb
.

Finished in 0.36684 seconds (files took 7.18 seconds to load)
1 example, 0 failures

It is working again. This code looks a little bit better - we traded unnecessary logic in the model for class coupling.

Next step is changing request to this method in the controller from:

@poll.export_results

to:

Polls::ExportResultsService.new(poll: @poll).call
# we're changing this method name to typical one for service
# since we can do it now easily

and of course changing tests - let’s move this whole test to separate file (without any changes):

# spec/services/polls/export_results_service_spec.rb
require 'rails_helper'

RSpec.describe Polls::ExportResultsService, type: :service do
  describe '#call' do
    subject(:service) { Polls::ExportResultsService.new(poll: poll) }

    let(:poll) { create :poll, :without_tokens, questions: [question1, question2] }

    let(:answer1) { create :answer, body: 'ANSWER1' }
    let(:answer2) { create :answer, body: 'ANSWER2' }
    let(:question1) { create :question, answers: [answer1, answer2], max_answers: 1 }

    let(:answer3) { create :answer, body: 'ANSWER3' }
    let(:answer4) { create :answer, body: 'ANSWER4' }
    let(:answer5) { create :answer, body: 'ANSWER5' }
    let(:question2) { create :question, answers: [answer3, answer4, answer5], max_answers: 2 }

    let(:result) { File.read 'spec/fixtures/export_results1.txt' }

    before do
      poll
      create :vote, token: 'token1', answered: [answer1.id, answer3.id]
      create :vote,  token: 'token2', answered: [answer1.id, answer3.id, answer4.id]
      create :vote,  token: 'token3', answered: [answer1.id, answer4.id, answer5.id]
      create :vote,  token: 'token4', answered: [answer2.id, answer3.id, answer5.id]
      create :vote,  token: 'token5', answered: [answer2.id, answer5.id]
    end

    it 'loads the file' do
      expect(service.call).to eq result
    end
  end
end

check if it’s ok:

$ rspec spec/services/polls/export_results_service_spec.rb
.

Finished in 0.22261 seconds (files took 2.82 seconds to load)
1 example, 0 failures

Great! Now remove the delegate from Poll class, leaving it as it always should be - plain and simple:

class Poll < ActiveRecord::Base
  has_many :questions, dependent: :delete_all
  has_many :answers, through: :questions
end

Now we’re in the moment that can be called ‘separation of code.’ We have part of our good code, and our crappy code is contained in one place. Unfortunately, in many real-life projects, this will be the place where we would stop our refactoring. Not here though! Let’s move to the next step:

The real refactoring

The first step would be to use csv instead of manually concatenating strings together - we don’t want to reinvent the wheel here.

module Polls
  class ExportResultsService
    def initialize(poll:)
      @poll = poll
    end

    def call
      CSV.generate do |csv|
        csv << headers
        lines.each do |line|
          csv << line
        end
      end
    end

    private

    attr_reader :poll

    def headers
      headers = %w[TOKEN]
      poll.questions.order(:id).each do |que|
        que.answers.order(:id).each do |answ|
          headers << answ.body
        end
      end
      headers
    end

    def lines
      output_lines = []
      poll.votes.registered.map do |vt|
        results = [vt.token]
        poll.questions.order(:id).each do |que|
          que.answers.order(:id).each do |answ|
            results << (answ.id.in?(vt.registered_answer_ids) ? '1' : '0')
          end
        end
        output_lines << results
      end
      output_lines
    end
  end
end

Our call method looks good. Let’s check our test:

$ rspec spec/services/polls/export_results_service_spec.rb
F

Failures:

  1) Polls::ExportResultsService#call loads the file
     Failure/Error: expect(service.call).to eq result

       expected: "TOKEN,\"ANSWER1\",\"ANSWER2\",\"ANSWER3\",\"ANSWER4\",\"ANSWER5\"\ntoken1,1,0,1,0,0\ntoken2,1,0,1,1,0\ntoken3,1,0,0,1,1\ntoken4,0,1,1,0,1\ntoken5,0,1,0,0,1\n"
            got: "TOKEN,ANSWER1,ANSWER2,ANSWER3,ANSWER4,ANSWER5\ntoken1,1,0,1,0,0\ntoken2,1,0,1,1,0\ntoken3,1,0,0,1,1\ntoken4,0,1,1,0,1\ntoken5,0,1,0,0,1\n"

       (compared using ==)

       Diff:
       @@ -1,4 +1,4 @@
       -TOKEN,"ANSWER1","ANSWER2","ANSWER3","ANSWER4","ANSWER5"
       +TOKEN,ANSWER1,ANSWER2,ANSWER3,ANSWER4,ANSWER5
        token1,1,0,1,0,0
        token2,1,0,1,1,0
        token3,1,0,0,1,1

Oops! We broke tests. But - let’s think about it for one second and take a look at CSV specification:

5.  Each field may or may not be enclosed in double quotes (however
       some programs, such as Microsoft Excel, do not use double quotes
       at all).  If fields are not enclosed with double quotes, then
       double quotes may not appear inside the fields.  For example:

       "aaa","bbb","ccc" CRLF
       zzz,yyy,xxx

6.  Fields containing line breaks (CRLF), double quotes, and commas
    should be enclosed in double-quotes.  For example:

    "aaa","b CRLF
    bb","ccc" CRLF
    zzz,yyy,xxx

So it seems out that it’s our test which have been wrong! Let’s fix the fixture file (and rename it to .csv at the end):

TOKEN,ANSWER1,ANSWER2,ANSWER3,ANSWER4,ANSWER5
token1,1,0,1,0,0
token2,1,0,1,1,0
token3,1,0,0,1,1
token4,0,1,1,0,1
token5,0,1,0,0,1

and now tests are green again!

$ rspec spec/services/polls/export_results_service_spec.rb
.

Finished in 0.235 seconds (files took 3.77 seconds to load)
1 example, 0 failures

Let’s make headers method a little bit prettier:

def headers
    %w[TOKEN].tap do |headers|
      questions.each do |question|
        question.answers.order(:id).each do |answer|
          headers << answer.body
        end
      end
    end
  end

Looks good now! At this moment we may feel an urge to stop refactoring right now. We already improved code, maybe it’s enough? We’ve removed like 80 percent of bad code; we need to move from low-hanging fruits to higher-based ones. But the rewards are worth it - our code will be Sandi-compliant.

So, what about rows? Let’s start by making a new class, dedicated only to serialization of a single row:

# app/serializers/polls/question_row.rb
module Polls
  class QuestionRow
    def initialize(question:, vote:)
      @question = question
      @vote = vote
    end

    def to_a
      question.answers.order(:id).map do |answer|
        handle_value(vote, answer)
      end
    end

    private

    attr_reader :question, :vote

    def humanize_boolean(bool)
      bool ? 1 : 0
    end

    def handle_value(vote, answer)
      humanize_boolean(vote.registered_answer_ids.include?(answer.id))
    end
  end
end

and test it:

require 'rails_helper'

RSpec.describe Polls::QuestionRow, type: :serializer do
  describe '#to_a' do
    subject(:service) { Polls::QuestionRow.new(question: question, vote: vote) }

    let(:vote) { instance_double('Vote') }
    let(:question) { instance_double('Question') }
    let(:answers) do
      instance_double('Answer::ActiveRecord_Associations_CollectionProxy')
    end
    let(:answer_1) { instance_double('Answer') }
    let(:answer_2) { instance_double('Answer') }

    before do
      allow(vote).to receive(:registered_answer_ids).and_return([1])
      allow(question).to receive(:answers).and_return(answers)
      allow(answers).to receive(:order).and_return([answer_1, answer_2])
      allow(answer_1).to receive(:id).and_return(1)
      allow(answer_2).to receive(:id).and_return(2)
    end

    it 'serialize row correctly' do
      expect(service.to_a).to eq([1, 0])
    end
  end
end

Of course, we need to check if tests are green:

$ rspec spec/serializer/polls/question_row_spec.rb
.

Finished in 0.01297 seconds (files took 4.26 seconds to load)
1 example, 0 failures

Yes they are, so let’s create a method for serializing a row with our new class:

def questions
  poll.questions.order(:id)
end

def serialize_row(vote:)
  questions.flat_map do |question|
    QuestionRow.new(question: question, vote: vote).to_a
  end
end

and finally, let’s create a method generating the whole row - together with the token:

def questions
  poll.questions.order(:id)
end

def serialize_row(vote:)
  questions.flat_map do |question|
    QuestionRow.new(question: question, vote: vote).to_a
  end
end

We need to accustom call to make use of it:

module Polls
  class ExportResultsService
  # …
    def call
      CSV.generate do |csv|
        csv << headers
        registered_votes.each do |vote|
          csv << row(vote: vote)
        end
      end
    end
  # …
  end
end

Are tests green?

$ rspec spec/services/polls/export_results_service_spec.rb
.

Finished in 0.2317 seconds (files took 3.86 seconds to load)
1 example, 0 failures

Yes, they are!

How does our whole service look at the end?

module Polls
  class ExportResultsService
    def initialize(poll:)
      @poll = poll
    end

    def call
      CSV.generate do |csv|
        csv << headers
        registered_votes.each do |vote|
          csv << row(vote: vote)
        end
      end
    end

    private

    attr_reader :poll

    def row(vote:)
      [vote.token, *serialize_row(vote: vote)]
    end

    def headers
      %w[TOKEN].tap do |headers|
        questions.each do |question|
          question.answers.order(:id).each do |answer|
            headers << answer.body
          end
        end
      end
    end

    def registered_votes
      poll.votes.registered
    end

    def questions
      poll.questions.order(:id)
    end

    def serialize_row(vote:)
      questions.flat_map do |question|
        QuestionRow.new(question: question, vote: vote).to_a
      end
    end
  end
end

Nice!

We started with a single model and ended with object-oriented structure, easily testable and looking good. In the process, we spotted incorrect tests - just another day of working with real-life code.

The code in this blog note comes from our internal application for one time voting - Revote, which we will be open-sourcing soon. Stay tuned!