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! ( /,$/ , "

" ) 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 ( /,$/ , "

" ) 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! ( /,$/ , "

" ) 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 ( /,$/ , "

" ) 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! ( /,$/ , "

" ) 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 ( /,$/ , "

" ) 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 \"

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

" got: "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

" ( 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!