Code Smells & Refactoring

Code smells are usually not bugs—they are not
technically incorrect and don't currently prevent
the program from functioning. Instead, they
indicate weaknesses in design that may be slowing
down development or increasing the risk of bugs
or failures in the future.
Code smells

View Slide

Code refactoring is a disciplined technique for
restructuring an existing body of code, altering its
internal structure without changing its external
behavior
Refactoring

View Slide

Exam ExamQuestion
ExamEntry ExamAnswer

View Slide

class ExamEntry < ActiveRecord::Base
def result
correct_answers = []
for answer in answers
question = answer.question
if (question.type == 'single' && question.correct_answer == answer.text) ||
(question.type == 'multi' && question.correct_answers.include?(answer.text))
correct_answers << answer
end
end
wrong_answers = []
unless (question.type == 'single' && question.correct_answer == answer.text) ||
wrong_answers << answer
end
end
score = 0
for answer in correct_answers
score += answer.question.points
end
{
correct_answers: correct_answers,
wrong_answers: wrong_answers,
score: score
}
end
end

View Slide

Long Method

View Slide

A method, function, or procedure that has grown
too large.
Long Method

View Slide

describe ExamEntry do
describe "#result" do
it "works ( I hope :P )" do
exam = create :exam
single_1 = create :single_question, exam: exam, points: 1
multi_1 = create :multi_question, exam: exam, points: 1
entry = create(:exam_entry, answers: [
create(:answer, question: single_1, text: single_1.correct_answer),
create(:answer, question: single_2, text: '-wrong-answer-'),
create(:answer, question: multi_1, text: multi_1.correct_answers.first),
create(:answer, question: multi_2, text: '-wrong-answer-')
])
result = entry.result
result.keys.should eq [:correct_answers, :wrong_answers, :score]
result[:correct_answers].map(&:question).should eq [single_1, multi_1]
result[:wrong_answers].map(&:question).should eq [single_2, multi_2]
result[:score].should eq single_1.points + multi_1.points
end
end
end

View Slide

exam = create :exam
])
end
end
end

View Slide

exam = create :exam
])
end
end
end

View Slide

exam = create :exam
])
end
end
end

View Slide

exam = create :exam
])
end
end
end

View Slide

exam = create :exam
])
end
end
end

View Slide

exam = create :exam
])
end
end
end

View Slide

exam = create :exam
])
end
end
end

View Slide

exam = create :exam
])
end
end
end

View Slide

Text

View Slide

def result
end
end
wrong_answers = []
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
end
end
wrong_answers = []
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
end
end
wrong_answers = []
end
end
score = 0
end
{
score: score
}
end

View Slide

Conditional complexity

View Slide

Branches that check lots of unrelated conditions
and edge cases that don't seem to capture the
meaning of a block of code.
Complex conditionals

View Slide

(question.type == 'single' && question.correct_answer == answer.text) ||

View Slide

describe ExamQuestion do
describe "#correct_answer?" do
context "single" do
it "returns true if answer is correct"
it "returns false if answer is wrong"
end
context "multi" do
it "returns true if answer included in answers list"
it "returns false if answer isn’t included in answers list"
end
end
end

View Slide

class ExamQuestion < ActiveRecord::Base
def correct_answer?(answer)
if type == 'single'
correct_answer == answer
else
correct_answers.include? answer
end
end
end

View Slide

class ExamQuestion < ActiveRecord::Base
def correct_answer?(answer)
if type == 'single'
correct_answer == answer
else
correct_answers.include? answer
end
end
end

View Slide

Text

View Slide

def result
if (question.type == 'single' && question.correct_answer == answer.text) || (question.t
end
end
wrong_answers = []
unless (question.type == 'single' && question.correct_answer == answer.text) || (questi
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
end
end
wrong_answers = []
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
end
end
wrong_answers = []
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
if question.correct_answer? answer.text
end
end
wrong_answers = []
unless question.correct_answer? answer.text
end
end
score = 0
end
{
score: score
}
end

View Slide

Text

View Slide

def result
end
end
wrong_answers = []
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
end
end
wrong_answers = []
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
end
end
wrong_answers = []
end
end
score = 0
end
{
score: score
}
end

View Slide

Code duplication

View Slide

Identical or very similar code exists in more than
one location.
Duplicated code

View Slide

def result
end
end
wrong_answers = []
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
end
end
wrong_answers = []
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
end
end
wrong_answers = []
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
wrong_answers = []
else
end
end
score = 0
end
{
score: score
}
end

View Slide

Text

View Slide

def result
wrong_answers = []
else
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
wrong_answers = []
else
end
end
score = 0
end
{
score: score
}
end

View Slide

def result
wrong_answers = []
score = 0
score += question.points
else
end
end
{
score: score
}
end

View Slide

Text

View Slide

def result
wrong_answers = []
score = 0
else
end
end
{
score: score
}
end

View Slide

def result
wrong_answers = []
score = 0
else
end
end
{
score: score
}
end

View Slide

def result
wrong_answers = []
score = 0
else
end
end
{
score: score
}
end

View Slide

Feature Envy

View Slide

A class that uses methods of another class
excessively.
Feature envy

View Slide

def result
wrong_answers = []
score = 0
else
end
end
{
score: score
}
end

View Slide

describe ExamAnswer do
describe "#correct?" do
it "returns true if answer is correct"
it "returns false if answer is wrong"
end
end

View Slide

Text

View Slide

class ExamAnswer < ActiveRecord::Base
def correct?
question.correct_answer? text
end
end

View Slide

Text

View Slide

def result
wrong_answers = []
score = 0
else
end
end
{
score: score
}
end

View Slide

def result
wrong_answers = []
score = 0
else
end
end
{
score: score
}
end

View Slide

def result
wrong_answers = []
score = 0
else
end
end
{
score: score
}
end

View Slide

def result
wrong_answers = []
score = 0
if answer.correct?
else
end
end
{
score: score
}
end

View Slide

Text

View Slide

def result
wrong_answers = []
score = 0
if answer.correct?
else
end
end
{
score: score
}
end

View Slide

def result
wrong_answers = []
score = 0
if answer.correct?
else
end
end
{
score: score
}
end

View Slide

Message chain

View Slide

Long sequences of method calls to get routine
data. Intermediaries are dependencies in disguise.
Message chain

View Slide

describe "#points" do
context "correct answer" do
it "returns its question points"
end
context "wrong answer" do
it "returns 0"
end
end
end

View Slide

describe "#points" do
context "correct answer" do
it "returns its question points"
end
context "wrong answer" do
it "returns 0"
end
end
end

View Slide

Text

View Slide

class ExamAnswer < ActiveRecord::Base
def points
return 0 unless correct?
question.points
end
end

View Slide

Code Smells & Refactoring

Code Smells & Refactoring

Radoslav Stankov

More Decks by Radoslav Stankov

Other Decks in Technology

Featured

Transcript