Why Our Code Smells

SMELLS
http://www.ﬂickr.com/photos/kanaka/3480201136/
WHY OUR CODE

View Slide

github.com/bkeepers
@bkeepers

View Slide

I AM TIRED OF
WRITING
BAD CODE.

View Slide

I AM TIRED OF
MAINTAINING
BAD CODE.

View Slide

Kent Beck
Smalltalk Best Practice Patterns
“Code doesn’t lie. If
you’re not listening,
you won’t hear the
truths it tells.”

View Slide

A CODE SMELL
USUALLY INDICATES
A DEEPER PROBLEM
IN THE SYSTEM.

View Slide

View Slide

CODE SMELLS ARE
HEURISTICS TO
SUGGEST WHEN TO
REFACTOR AND WHAT
TECHNIQUES TO USE.

View Slide

DIVERGENT CHANGE

View Slide

DIVERGENT CHANGE
Occurs when one class is commonly
changed in different ways for different
reasons. Any change to handle a
variation should change a single class.

View Slide

DIVERGENT CHANGE
Occurs when one class is commonly
changed in different ways for different
reasons. Any change to handle a
variation should change a single class.
Refactoring:
Identify everything that changes for a
particular cause and use Extract Class
to put them all together.

View Slide

OUR CODE SMELLS IN
THE 21 WAYS THAT
BECK AND FOWLER
DESCRIBE, BUT…

View Slide

our code smells when
UNIT TESTS ARE
COMPLEX & SLOW.
IT IS TIGHTLY COUPLED
TO A FRAMEWORK.

View Slide

UNIT TESTS
ARE COMPLEX.

View Slide

TDD IS A
DESIGN
PROCESS.

View Slide

WHY DO WE WRITE TESTS?

View Slide

1.Guard against regressions

View Slide

2.Gain conﬁdence to change

View Slide

2.Gain conﬁdence to change
3.Discover better designs

View Slide

WARNING
Excessive use of
quotations ahead.

View Slide

Steve Freeman, Nat Pryce
Growing Object-Oriented Software, Guided by Tests
“We ﬁnd that the effort of writing
a test ﬁrst also gives us rapid
feedback about the quality of our
design ideas—that making code
accessible for testing often
drives it towards being cleaner
and more modular.”

View Slide

unit tests can be complex when
OBJECTS ARE
TOO TIGHTLY
COUPLED.

View Slide

Exhibit A: GitHub Notiﬁcations
context Notifications::Emails::Message do
test "#to uses global email" do
@settings.email :global, '[email protected]'
assert_equal '[email protected]', @message.to
end
test "#body includes comment body" do
assert_match @comment.body, @message.body
end
end

View Slide

setup do
@comment = Issue.make
@summary = Notifications::Summary.from(@comment)
@handlers = [Notifications::EmailHandler.new]
@delivery = Notifications::Delivery.new(
@summary, @comment, @handlers)
@settings = Notifications::Settings.new(-1)
@message = Notifications::Emails::Message.new(
@delivery, @settings)
end
end

View Slide

“When we find a feature
that’s difficult to test, we
don’t just ask ourselves
how to test it, but also
why is it difficult to test.”

View Slide

setup do
@summary = Notifications::Summary.from(@comment)
@handlers = [Notifications::EmailHandler.new]
@delivery = Notifications::Delivery.new(
@summary, @comment, @handlers)
@delivery, @settings)
end
end
end

View Slide

setup do
@comment, @settings)
end
end
end

View Slide

setup do
end
end
end
end

View Slide

unit tests can be complex when
OBJECTS ARE
DOING TOO
MUCH.

View Slide

“An element’s cohesion is a measure
of whether its responsibilities form a
meaningful unit…Think of a machine
that washes both clothes and dishes
—it’s unlikely to do both well.”

View Slide

Every class should have a single responsibility,
and that responsibility should be entirely
encapsulated by the class.
SINGLE
RESPONSIBILITY
PRINCIPLE

View Slide

“Our heuristic is that we
should be able to describe
what an object does
without using any
conjunctions (‘and,’ ‘or’).”

View Slide

jQuery(function($) {
$('#new-status').on('submit', function() {
$.ajax({
url: '/statuses',
type: 'POST',
dataType: 'json',
data: {text: $(this).find('textarea').val()},
success: function(data) {
$('#statuses').append('' + data.text + '');
}
});
return false;
});
});
example lovingly stolen from @searls

View Slide

The test tells me the code is complex.
describe("Updating my status", function() {
var $form, $statuses;
beforeEach(function(){
$form = affix('form#new-status');
$form.affix('textarea').val('sniffing code');
$statuses = affix('#statuses');
spyOn($, "ajax");
$form.trigger('submit');
});
it("posts status to the server", function() {
expect($.ajax).toHaveBeenCalledWith({
url: '/statuses',
data: {text: 'sniffing code'},
success: jasmine.any(Function)
});
});

View Slide

The test tells me the code is complex.
});
it("posts status to the server", function() {
expect($.ajax).toHaveBeenCalledWith({
url: '/statuses',
data: {text: 'sniffing code'},
success: jasmine.any(Function)
});
});
describe("with a successful response", function() {
beforeEach(function() {
$.ajax.mostRecentCall.args[0].success({
text: "This is starting stink!"
});
});
it("appends text", function() {
expect($statuses).toHaveHtml(
'This is starting stink!');
});
});
});

View Slide

Why is this hard to test?
$.ajax({
url: '/statuses',
type: 'POST',
dataType: 'json',
}
});
return false;
});
});

View Slide

$.ajax({
url: '/statuses',
type: 'POST',
dataType: 'json',
}
});
return false;
});
});
1. page event

View Slide

$.ajax({
url: '/statuses',
type: 'POST',
dataType: 'json',
}
});
return false;
});
});
1. page event
2. user event

View Slide

$.ajax({
url: '/statuses',
type: 'POST',
dataType: 'json',
}
});
return false;
});
});
1. page event
2. user event
3. network IO

View Slide

$.ajax({
url: '/statuses',
type: 'POST',
dataType: 'json',
}
});
return false;
});
});
1. page event
2. user event
3. network IO
4. user input

View Slide

$.ajax({
url: '/statuses',
type: 'POST',
dataType: 'json',
}
});
return false;
});
});
1. page event
2. user event
3. network IO
4. user input
5. network event

View Slide

$.ajax({
url: '/statuses',
type: 'POST',
dataType: 'json',
}
});
return false;
});
});
1. page event
2. user event
3. network IO
4. user input
5. network event
6. HTML templating

View Slide

$.ajax({
url: '/statuses',
type: 'POST',
dataType: 'json',
}
});
return false;
});
});
So we start to refactor…

View Slide

$.ajax({
url: '/statuses',
type: 'POST',
dataType: 'json',
}
});
return false;
});
});
Refactor to use a model

View Slide

var statuses = new Collection.Statuses();
statuses.create({text: $(this).find('textarea').val()});
return false;
});
statuses.on('add', function(status) {
$('#statuses').append(
'' + status.get('text') + '');
});
});

View Slide

return false;
});
});
});
RESPONSIBILITY:
Sync state with server

View Slide

Refactor handling of user input
return false;
});
});
});

View Slide

new View.PostStatus({collection: statuses});
});
});

View Slide

});
});
RESPONSIBILITY:
Create statuses from user input

View Slide

});
});
Refactor templating

View Slide

Refactor templating
new View.StatusList({collection: statuses});
});

View Slide

Refactor templating
});
RESPONSIBILITY:
Render statuses to the page

View Slide

});
RESPONSIBILITY:
Initialize application on page load

View Slide

Our tests only have one concern
describe("View.StatusList", function() {
beforeEach(function() {
$el = $('');
collection = new Backbone.Collection();
view = new View.StatusList({
el: $el,
collection: collection
});
});
it("appends newly added items", function() {
collection.add({text: 'this is fun!'});
expect($el.find('li').length).toBe(1);
expect($el.find('li').text()).toEqual('this is fun!');
});
});

View Slide

PAY ATTENTION TO
TESTING PAINS AND
ADJUST THE DESIGN
ACCORDINGLY.

View Slide

Poor quality tests can slow
development to a crawl, and
poor internal quality of the
system being tested will result
in poor quality tests.

View Slide

Christian Johansen
Test-Driven JavaScript Development
“If you write bad unit tests, you
might ﬁnd that you gain none of
the beneﬁts, and instead are
stuck with a bunch of tests that
are time-consuming and hard to
maintain.”

View Slide

UNIT TESTS
ARE SLOW.

View Slide

$ bx rake test:units
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
...............................................................................................
........................................................................................
Finished in 274.623286 seconds.
2273 tests, 6765 assertions, 0 failures, 0 errors

View Slide

WHAT’S WRONG WITH SLOW TESTS?

View Slide

You don't run them often

View Slide

You waste time waiting for tests

View Slide

You distracted others while waiting

View Slide

http://xkcd.com/303/

View Slide

You commit failing changes

View Slide

You commit failing changes
You lose the rapid feedback cycle

View Slide

unit tests can be slow when they
INTERACT
WITH SLOW
COMPONENTS.

View Slide

setup do
@comment = Issue.make! # create record in database
end
end
end
end

View Slide

$ ruby test/unit/notifications/emails/message_test.rb
...................

View Slide

setup do
@comment = Issue.make # create in memory
@comment.id = -1 # make it appear persisted
end
end
end

View Slide

$ ruby test/unit/notifications/emails/message_test.rb
...................

View Slide

3.517926
÷ 0.073752
~50 X FASTER

View Slide

DON’T TEST
OBJECTS IN
ISOLATION.

View Slide

context Notifications::Emails::CommitMention do
setup do
@repo = Repository.make!
readonly_example_repo :notification_mentions, @repo
@commit = @repo.commit('a62c6b20')
@comment = CommitMention.new(:commit_id => @commit.sha)
@message = Emails::CommitMention.new(@comment)
end
test 'subject' do
expected = "[testy] hello world (#{@comment.short_id})"
assert_equal expected, @message.subject
end
end

View Slide

context Notifications::Emails::CommitMention do
setup do
@commit = stub(
:sha => Sham.sha,
:short_id => '12345678',
:short_message => 'hello world',
:message => 'goodbye world'
)
@comment = CommitMention.new(:commit_id => @commit.sha)
@comment.stubs(:commit => @commit)
@message = Emails::CommitMention.new(@comment)
end
test 'subject' do
expected = "[testy] hello world (#{@comment.short_id})"
assert_equal expected, message.subject
end

View Slide

BEFORE
$ ruby test/unit/notifications/emails/commit_mention_test.rb
....
AFTER
$ ruby test/unit/notifications/emails/commit_mention_test.rb
....

View Slide

0.576135
÷ 0.052412
~10 X FASTER

View Slide

BOOTSTRAP
HEAVY
FRAMEWORKS.

View Slide

View Slide

$ time ruby test/unit/notifications/email_handler_test.rb

View Slide

........

View Slide

........
real 0m7.065s
user 0m4.948s
sys 0m1.961s

View Slide

test/fast/notiﬁcations/web_handler.rb
require 'notifications/summary_store'
require 'notifications/memory_indexer'
require 'notifications/web_handler'
context Notifications::WebHandler do
def web
@web ||= WebHandler.new(
:indexer => MemoryIndexer.new,
:store => SummaryStore.new
)
end
def test_insert_increments_count
assert_equal 0, web.count(1)
web.add build_summary, 1
assert_equal 1, web.count(1)
end

View Slide

$ time ruby test/fast/notifications/web_handler_test.rb
....
real 0m0.139s
user 0m0.068s
sys 0m0.063s

View Slide

7.065
÷ 0.139
~50 X FASTER

View Slide

SLOW TEST MYTHS

View Slide

SLOW TEST MYTHS
“Our tests are slow because we have too
many of them.”

View Slide

SLOW TEST MYTHS
many of them.”
“To speed up our tests, we just need to
parallelize them.”

View Slide

SLOW TEST MYTHS
many of them.”
“To speed up our tests, we just need to
parallelize them.”
“We can’t use test doubles because we’ll
loose conﬁdence that it still works.”

View Slide

PAY ATTENTION
TO THE SPEED OF
YOUR TESTS AS
YOU WRITE THEM.

View Slide

IT IS TIGHTLY
COUPLED TO A
FRAMEWORK.

View Slide

FRAMEWORKS
ENCOURAGE YOU TO
PUT ALL OF YOUR
APPLICATION INSIDE
THEIR SANDBOX.

View Slide

…WHICH MAKES
CODE DIFFICULT TO
TEST, CHANGE AND
REUSE.

View Slide

View Slide

God Objects

View Slide

God Objects
class Issue < ActiveRecord::Base

View Slide

God Objects
# validations
validates_presence_of :title, :user_id, :repository_id

View Slide

God Objects
# validations
# associations
belongs_to :user

View Slide

God Objects
# validations
# associations
belongs_to :user
# data integrity
before_validation :set_state

View Slide

God Objects
# validations
# associations
belongs_to :user
# data integrity
# misc concerns
before_save :audit_if_changed

View Slide

God Objects
# validations
# associations
belongs_to :user
# data integrity
# misc concerns
# querying
named_scope :watched_by, lambda {|user| ... }

View Slide

God Objects
# validations
# associations
belongs_to :user
# data integrity
# misc concerns
# querying
# who knows what these do?
include Mentionable, Subscribable, Summarizable

View Slide

God Objects
# validations
# associations
belongs_to :user
# data integrity
# misc concerns
# querying
# who knows what these do?
include Mentionable, Subscribable, Summarizable
# domain logic
def active_participants
[self.user] + watchers + commentors
end
end

View Slide

MAKE THE FRAMEWORK
DEPEND ON YOUR
APPLICATION, INSTEAD
OF MAKING YOUR
APPLICATION DEPEND
ON THE FRAMEWORK.

View Slide

GOOS

View Slide

coupled to a
framework
GOOS

View Slide

coupled to a
framework
the rest of the
application
GOOS

View Slide

A typical Rails controller…
class SessionsController < ApplicationController
def create
user = User.authenticate(params[:username],
params[:password])
if user
self.current_user = user
redirect_to root_path, success: 'You are signed in!'
else
render :new, warning: 'Wrong username or password.'
end
end
end

View Slide

…and model
class User < ActiveRecord::Base
def self.authenticate(username, password)
user = find_by_username(username)
user if user && user.authenticated?(password)
end
def authenticated?(password)
encrypt(password, self.salt) == self.encrypted_password
end
def encrypt(password, salt)
Digest::SHA1.hexdigest(password+salt)
end
end

View Slide

Why does this depend on Active Record?
class User < ActiveRecord::Base
def self.authenticate(username, password)
user = find_by_username(username)
user if user && user.authenticated?(password)
end
def authenticated?(password)
encrypt(password, self.salt) == self.encrypted_password
end
end
end

View Slide

The spec is already complex.
describe User do
# … a couple hundred lines of specs …
describe ".authenticate" do
let!(:user) do
create :user, :email => "bkeepers", :password => "testing"
end
it "returns user with case insensitive username" do
User.authenticate('BKeepers', 'testing').should == @user
end
it "returns nil with incorrect password" do
User.authenticate("bkeepers", "wrong").should be_nil
end
it "returns nil with unknown username" do
User.authenticate('[email protected]', 'testing').should be_nil
end
end
# … a couple hundred more lines of specs …

View Slide

Create objects to model the domain
class SessionsController < ApplicationController
def create
user = PasswordAuthentication.new(params[:username],
params[:password]).user
if user
self.current_user = user
redirect_to root_path, success: 'You are signed in!'
else
render :new, warning: 'Wrong username or password.'
end
end
end

View Slide

Plain ol’ Ruby class
class PasswordAuthentication
def initialize(username, password)
@username = username
@password = password
end
def user
end
end

View Slide

require 'spec_helper'
describe PasswordAuthentication do
describe 'user' do
context 'with a valid username & password'
context 'with an unknown username'
context 'with an incorrect password'
end
end

View Slide

describe 'user' do
let!(:user) do
create :user, :username => 'bkeepers',
:password => 'testing'
end
context 'with a valid username & password' do
subject do
PasswordAuthentication.new(user.username, 'testing')
end
it 'returns the user' do
subject.user.should == user
end
end
end

View Slide

end
def user
User.find_by_username(@username)
end
end

View Slide

context 'with an unknown username' do
subject do
PasswordAuthentication.new('unknown', 'testing')
end
it 'returns nil' do
subject.user.should be_nil
end
end

View Slide

No changes necessary
end
def user
User.find_by_username(@username)
end
end

View Slide

describe 'user' do
context 'with a valid username & password' do # …
context 'with an unknown username' do # …
context 'with an incorrect password' do
subject do
PasswordAuthentication.new(user.username, 'wrong')
end
it 'returns nil' do
end
end
end
end

View Slide

# …
def user
user = User.find_by_username(@username)
user if user && authenticated?(user)
end
private
def authenticated?(user)
encrypt(@password, user.password_salt) ==
user.encrypted_password
end
end
end

View Slide

describe 'user' do
let!(:user) do
create :user, :username => 'bkeepers',
:password => 'testing'
# hits the DB :(
end
# …
end
end

View Slide

describe 'user' do
let!(:user) do
double :user,
:username => 'bkeepers',
:encrypted_password => '…',
:password_salt => '…'
end
before do
User.stub(:find_by_username).
with(user.username).
and_return(user)
end
end
end

View Slide

context 'with an unknown username' do
before do
User.should_receive(:find_by_username).
with('unknown').
and_return(nil)
end
subject do
PasswordAuthentication.new('unknown', 'testing')
end
it 'returns nil' do
end
end

View Slide

POSITIVE FEEDBACK LOOP
Unit tests help us isolate our
code and reduce coupling to
frameworks, which makes
our tests faster.

View Slide

Before we part ways
EPILOGUE

View Slide

Robert C. Martin
Clean Code
“Writing clean code requires the
disciplined use of a myriad little
techniques applied through a
painstakingly acquired sense of
‘cleanliness.’”

View Slide

REFERENCES

View Slide

REFERENCES
Gary Bernhardt
Fast test, Slow Test
http://pyvideo.org/video/631/fast-test-slow-test
Corey Haines
Fast Rails Tests
http://confreaks.com/videos/641-gogaruco2011-fast-rails-tests

View Slide

@bkeepers
http://bit.ly/smells-slides
QUESTIONS?

View Slide

Why Our Code Smells

Why Our Code Smells

Brandon Keepers
PRO

More Decks by Brandon Keepers

Other Decks in Programming

Featured

Transcript

Why Our Code Smells

Why Our Code Smells

Brandon Keepers PRO

More Decks by Brandon Keepers

Other Decks in Programming

Featured

Transcript

Brandon Keepers
PRO