Skip to content

Commit

Permalink
Move type-specific question code into subclasses
Browse files Browse the repository at this point in the history
* Fixes Large Class smell
* Performed Replace Type Code With Subclasses refactoring
* Builds Question subclasses in QuestionsController
  • Loading branch information
jferris committed Nov 29, 2012
1 parent 5125668 commit a08f801
Show file tree
Hide file tree
Showing 13 changed files with 119 additions and 111 deletions.
13 changes: 8 additions & 5 deletions example_app/app/controllers/questions_controller.rb
@@ -1,9 +1,10 @@
class QuestionsController < ApplicationController
def new
@survey = Survey.find(params[:survey_id])
@question = @survey.questions.new
@question.options = [Option.new, Option.new, Option.new]
@question.type = params[:type]
build_question
if @question.type == 'MultipleChoiceQuestion'
@question.options = [Option.new, Option.new, Option.new]
end
end

def create
Expand All @@ -19,12 +20,14 @@ def create
private

def build_question
@question = @survey.questions.new(question_params, without_protection: true)
type = params[:question][:type]
@question = type.constantize.new(question_params)

This comment has been minimized.

Copy link
@bemurphy

bemurphy Feb 8, 2013

I'm wondering if the strong_params are generally enough to allow arbitrary object creation from user input like the type param, might give somebody a dangerous idea. In this specific instance it's unlikely because of the specificity of params, but generally I don't think I'd ever constantize straight off user input. Fortunately this is covered in the "Convention Over Configuration" chapter so people won't be unaware.

Maybe additional security concerns would clutter this code as an example, just wanted to throw it out there.

This comment has been minimized.

Copy link
@jferris

jferris Feb 11, 2013

Author Member

@bemurphy thanks for the comment. That's definitely a valid concern, and it's something that we'd likely address in a real application.

In the example, we do validate the submittable_type, so you wouldn't actually be able to save it except with the three allowed question types. It's usually safe to instantiate a class (initialize shouldn't have side effects), so it's unlikely that you could compromise an application using this code. However, it might be possible.

I updated the chapter in 884cd85 to make sure readers know to sanitize the type.

@question.survey = @survey
end

def question_params
params.
require(:question).
permit(:type, :title, :options_attributes, :minimum, :maximum)
permit(:title, :options_attributes, :minimum, :maximum)
end
end
13 changes: 13 additions & 0 deletions example_app/app/models/multiple_choice_question.rb
@@ -1,2 +1,15 @@
class MultipleChoiceQuestion < Question
has_many :options, foreign_key: :question_id

accepts_nested_attributes_for :options, reject_if: :all_blank

def summary
total = answers.count
counts = answers.group(:text).order('COUNT(*) DESC').count
percents = counts.map do |text, count|
percent = (100.0 * count / total).round
"#{percent}% #{text}"
end
percents.join(', ')
end
end
3 changes: 3 additions & 0 deletions example_app/app/models/open_question.rb
@@ -1,2 +1,5 @@
class OpenQuestion < Question
def summary
answers.order(:created_at).pluck(:text).join(', ')
end
end
44 changes: 0 additions & 44 deletions example_app/app/models/question.rb
Expand Up @@ -3,53 +3,9 @@ class Question < ActiveRecord::Base

QUESTION_TYPES = %w(OpenQuestion MultipleChoiceQuestion ScaleQuestion).freeze

validates :maximum, presence: true, if: :scale?
validates :minimum, presence: true, if: :scale?
validates :type, presence: true, inclusion: QUESTION_TYPES
validates :title, presence: true

belongs_to :survey
has_many :answers
has_many :options

accepts_nested_attributes_for :options, reject_if: :all_blank

def summary
case type
when 'MultipleChoiceQuestion'
summarize_multiple_choice_answers
when 'OpenQuestion'
summarize_open_answers
when 'ScaleQuestion'
summarize_scale_answers
end
end

def steps
(minimum..maximum).to_a
end

private

def scale?
type == 'ScaleQuestion'
end

def summarize_multiple_choice_answers
total = answers.count
counts = answers.group(:text).order('COUNT(*) DESC').count
percents = counts.map do |text, count|
percent = (100.0 * count / total).round
"#{percent}% #{text}"
end
percents.join(', ')
end

def summarize_open_answers
answers.order(:created_at).pluck(:text).join(', ')
end

def summarize_scale_answers
sprintf('Average: %.02f', answers.average('text'))
end
end
10 changes: 10 additions & 0 deletions example_app/app/models/scale_question.rb
@@ -1,2 +1,12 @@
class ScaleQuestion < Question
validates :maximum, presence: true
validates :minimum, presence: true

def steps
(minimum..maximum).to_a
end

def summary
sprintf('Average: %.02f', answers.average('text'))
end
end
4 changes: 2 additions & 2 deletions example_app/app/views/questions/new.html.erb
@@ -1,4 +1,4 @@
<%= simple_form_for @question, url: survey_questions_path(@survey) do |form| -%>
<%= simple_form_for @question, as: :question, url: survey_questions_path(@survey) do |form| -%>
<%= form.hidden_field :type %>
<%= form.input :title %>
Expand All @@ -14,5 +14,5 @@
<%= form.input :maximum %>
<% end -%>
<%= form.submit %>
<%= form.submit 'Create Question' %>
<% end -%>
6 changes: 3 additions & 3 deletions example_app/app/views/surveys/show.html.erb
Expand Up @@ -3,17 +3,17 @@

<%= link_to(
'Add Multiple Choice Question',
new_survey_question_path(@survey, type: 'MultipleChoiceQuestion')
new_survey_question_path(@survey, question: { type: 'MultipleChoiceQuestion' })
) %>
<%= link_to(
'Add Open Question',
new_survey_question_path(@survey, type: 'OpenQuestion')
new_survey_question_path(@survey, question: { type: 'OpenQuestion' })
) %>
<%= link_to(
'Add Scale Question',
new_survey_question_path(@survey, type: 'ScaleQuestion')
new_survey_question_path(@survey, question: { type: 'ScaleQuestion' })
) %>
<%= simple_form_for [@survey, @completion] do |form| -%>
Expand Down
13 changes: 4 additions & 9 deletions example_app/spec/factories/application.rb
Expand Up @@ -15,12 +15,11 @@
text 'Hello'
end

factory :question do
factory :question, class: 'OpenQuestion' do
survey
title 'Question'
type 'OpenQuestion'

factory :multiple_choice_question do
factory :multiple_choice_question, class: 'MultipleChoiceQuestion' do
ignore do
options_texts { [] }
end
Expand All @@ -30,16 +29,12 @@
FactoryGirl.build(:option, text: text, question_id: attributes.id)
end
end

type 'MultipleChoiceQuestion'
end

factory :open_question do
type 'OpenQuestion'
factory :open_question, class: 'OpenQuestion' do
end

factory :scale_question do
type 'ScaleQuestion'
factory :scale_question, class: 'ScaleQuestion' do
end
end

Expand Down
20 changes: 20 additions & 0 deletions example_app/spec/models/multiple_choice_question_spec.rb
@@ -0,0 +1,20 @@
describe MultipleChoiceQuestion do
it { should have_many(:options) }
end

describe MultipleChoiceQuestion, '#summary' do
it 'returns a percentage breakdown' do
survey = create(:survey)
question = create(
:multiple_choice_question,
options_texts: %w(Blue Red),
survey: survey
)
taker = SurveyTaker.new(survey)
taker.answer question, 'Red'
taker.answer question, 'Blue'
taker.answer question, 'Red'

question.summary.should eq '67% Red, 33% Blue'
end
end
13 changes: 13 additions & 0 deletions example_app/spec/models/open_question_spec.rb
@@ -0,0 +1,13 @@
describe OpenQuestion, '#summary' do
it 'returns all answers' do
survey = create(:survey)
question = create(:open_question, survey: survey)
taker = SurveyTaker.new(survey)

taker.answer question, 'Hey'
taker.answer question, 'Hi'
taker.answer question, 'Hello'

question.summary.should eq 'Hey, Hi, Hello'
end
end
48 changes: 0 additions & 48 deletions example_app/spec/models/question_spec.rb
Expand Up @@ -13,52 +13,4 @@

it { should belong_to(:survey) }
it { should have_many(:answers) }
it { should have_many(:options) }

context 'scale' do
subject { build_stubbed(:scale_question) }

it { should validate_presence_of(:maximum) }
it { should validate_presence_of(:minimum) }
end
end

describe Question, '#steps' do
it 'returns all numbers starting at the minimum and ending at the maximum' do
question = build_stubbed(:scale_question, minimum: 2, maximum: 5)
question.steps.should eq [2, 3, 4, 5]
end
end

describe Question, '#summary' do
it 'returns all answers to open questions' do
question = create(:open_question)
answer question, 'Hey'
answer question, 'Hi'
answer question, 'Hello'

question.summary.should eq 'Hey, Hi, Hello'
end

it 'returns a percentage breakdown for multiple choice questions' do
question = create(:multiple_choice_question, options_texts: %w(Blue Red))
answer question, 'Red'
answer question, 'Blue'
answer question, 'Red'

question.summary.should eq '67% Red, 33% Blue'
end

it 'returns the average for scale questions' do
question = create(:scale_question, minimum: 0, maximum: 10)
answer question, 6
answer question, 6
answer question, 8

question.summary.should eq 'Average: 6.67'
end

def answer(question, answer_text)
create(:answer, question: question, text: answer_text)
end
end
26 changes: 26 additions & 0 deletions example_app/spec/models/scale_question_spec.rb
@@ -0,0 +1,26 @@
describe ScaleQuestion do
subject { build_stubbed(:scale_question) }

it { should validate_presence_of(:maximum) }
it { should validate_presence_of(:minimum) }
end

describe ScaleQuestion, '#steps' do
it 'returns all numbers starting at the minimum and ending at the maximum' do
question = build_stubbed(:scale_question, minimum: 2, maximum: 5)
question.steps.should eq [2, 3, 4, 5]
end
end

describe ScaleQuestion, '#summary' do
it 'returns the average' do
survey = create(:survey)
question = create(:scale_question, minimum: 0, maximum: 10, survey: survey)
taker = SurveyTaker.new(survey)
taker.answer question, 6
taker.answer question, 6
taker.answer question, 8

question.summary.should eq 'Average: 6.67'
end
end
17 changes: 17 additions & 0 deletions example_app/spec/support/survey_taker.rb
@@ -0,0 +1,17 @@
class SurveyTaker
include FactoryGirl::Syntax::Methods

def initialize(survey)
@survey = survey
@completion = create(:completion, survey: @survey)
end

def answer(question, answer_text)
create(
:answer,
question: question,
text: answer_text,
completion: @completion
)
end
end

0 comments on commit a08f801

Please sign in to comment.