Upgrade to Pro — share decks privately, control downloads, hide ads and more …

Why Our Code Smells

Why Our Code Smells

Odors are communication devices. They exist for a reason and are usually trying to tell us something.

Our code smells and it is trying to tell us what is wrong. Does a test case require an abundance of setup? Maybe the code being tested is doing too much, or it is not isolated enough for the test? Does an object have an abundance of instance variables? Maybe it should be split into multiple objects? Is a view brittle? Maybe it is too tightly coupled to a model, or maybe the logic needs abstracted into an object that can be tested?

In this talk, I will walk through code from projects that I work on every day, looking for smells that indicate problems, understanding why it smells, what the smell is trying to tell us, and how to refactor it.

Brandon Keepers

May 23, 2012
Tweet

More Decks by Brandon Keepers

Other Decks in Programming

Transcript

  1. Kent Beck Smalltalk Best Practice Patterns “Code doesn’t lie. If

    you’re not listening, you won’t hear the truths it tells.”
  2. 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.
  3. 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.
  4. our code smells when UNIT TESTS ARE COMPLEX & SLOW.

    IT IS TIGHTLY COUPLED TO A FRAMEWORK.
  5. Steve Freeman, Nat Pryce Growing Object-Oriented Software, Guided by Tests

    “We find that the effort of writing a test first 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.”
  6. Exhibit A: GitHub Notifications 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
  7. Exhibit A: GitHub Notifications context Notifications::Emails::Message do 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 test "#to uses global email" do @settings.email :global, '[email protected]' assert_equal '[email protected]', @message.to end
  8. Steve Freeman, Nat Pryce Growing Object-Oriented Software, Guided by Tests

    “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.”
  9. Exhibit A: GitHub Notifications context Notifications::Emails::Message do 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 test "#to uses global email" do @settings.email :global, '[email protected]' assert_equal '[email protected]', @message.to end end
  10. Exhibit A: GitHub Notifications context Notifications::Emails::Message do setup do @comment

    = Issue.make @settings = Notifications::Settings.new(-1) @message = Notifications::Emails::Message.new( @comment, @settings) end test "#to uses global email" do @settings.email :global, '[email protected]' assert_equal '[email protected]', @message.to end end
  11. Exhibit A: GitHub Notifications context Notifications::Emails::Message do setup do @comment

    = Issue.make @settings = Notifications::Settings.new(-1) @message = Notifications::Emails::Message.new( @comment, @settings) end 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
  12. Steve Freeman, Nat Pryce Growing Object-Oriented Software, Guided by Tests

    “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.”
  13. Every class should have a single responsibility, and that responsibility

    should be entirely encapsulated by the class. SINGLE RESPONSIBILITY PRINCIPLE
  14. Steve Freeman, Nat Pryce Growing Object-Oriented Software, Guided by Tests

    “Our heuristic is that we should be able to describe what an object does without using any conjunctions (‘and,’ ‘or’).”
  15. 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('<li>' + data.text + '</li>'); } }); return false; }); }); example lovingly stolen from @searls
  16. 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) }); });
  17. 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( '<div>This is starting stink!</div>'); }); }); });
  18. Why is this hard to test? 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('<li>' + data.text + '</li>'); } }); return false; }); }); example lovingly stolen from @searls
  19. Why is this hard to test? 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('<li>' + data.text + '</li>'); } }); return false; }); }); 1. page event example lovingly stolen from @searls
  20. Why is this hard to test? 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('<li>' + data.text + '</li>'); } }); return false; }); }); 1. page event 2. user event example lovingly stolen from @searls
  21. Why is this hard to test? 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('<li>' + data.text + '</li>'); } }); return false; }); }); 1. page event 2. user event 3. network IO example lovingly stolen from @searls
  22. Why is this hard to test? 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('<li>' + data.text + '</li>'); } }); return false; }); }); 1. page event 2. user event 3. network IO 4. user input example lovingly stolen from @searls
  23. Why is this hard to test? 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('<li>' + data.text + '</li>'); } }); return false; }); }); 1. page event 2. user event 3. network IO 4. user input 5. network event example lovingly stolen from @searls
  24. Why is this hard to test? 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('<li>' + data.text + '</li>'); } }); return false; }); }); 1. page event 2. user event 3. network IO 4. user input 5. network event 6. HTML templating example lovingly stolen from @searls
  25. 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('<li>' + data.text + '</li>'); } }); return false; }); }); So we start to refactor…
  26. 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('<li>' + data.text + '</li>'); } }); return false; }); }); Refactor to use a model
  27. Refactor to use a model jQuery(function($) { var statuses =

    new Collection.Statuses(); $('#new-status').on('submit', function() { statuses.create({text: $(this).find('textarea').val()}); return false; }); statuses.on('add', function(status) { $('#statuses').append( '<li>' + status.get('text') + '</li>'); }); });
  28. Refactor to use a model jQuery(function($) { var statuses =

    new Collection.Statuses(); $('#new-status').on('submit', function() { statuses.create({text: $(this).find('textarea').val()}); return false; }); statuses.on('add', function(status) { $('#statuses').append( '<li>' + status.get('text') + '</li>'); }); }); RESPONSIBILITY: Sync state with server
  29. Refactor handling of user input jQuery(function($) { var statuses =

    new Collection.Statuses(); $('#new-status').on('submit', function() { statuses.create({text: $(this).find('textarea').val()}); return false; }); statuses.on('add', function(status) { $('#statuses').append( '<li>' + status.get('text') + '</li>'); }); });
  30. Refactor handling of user input jQuery(function($) { var statuses =

    new Collection.Statuses(); new View.PostStatus({collection: statuses}); statuses.on('add', function(status) { $('#statuses').append( '<li>' + status.get('text') + '</li>'); }); });
  31. Refactor handling of user input jQuery(function($) { var statuses =

    new Collection.Statuses(); new View.PostStatus({collection: statuses}); statuses.on('add', function(status) { $('#statuses').append( '<li>' + status.get('text') + '</li>'); }); }); RESPONSIBILITY: Create statuses from user input
  32. jQuery(function($) { var statuses = new Collection.Statuses(); new View.PostStatus({collection: statuses});

    statuses.on('add', function(status) { $('#statuses').append( '<li>' + status.get('text') + '</li>'); }); }); Refactor templating
  33. Refactor templating jQuery(function($) { var statuses = new Collection.Statuses(); new

    View.PostStatus({collection: statuses}); new View.StatusList({collection: statuses}); });
  34. Refactor templating jQuery(function($) { var statuses = new Collection.Statuses(); new

    View.PostStatus({collection: statuses}); new View.StatusList({collection: statuses}); }); RESPONSIBILITY: Render statuses to the page
  35. jQuery(function($) { var statuses = new Collection.Statuses(); new View.PostStatus({collection: statuses});

    new View.StatusList({collection: statuses}); }); RESPONSIBILITY: Initialize application on page load
  36. Our tests only have one concern describe("View.StatusList", function() { beforeEach(function()

    { $el = $('<ul></ul>'); 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!'); }); });
  37. Steve Freeman, Nat Pryce Growing Object-Oriented Software, Guided by Tests

    Poor quality tests can slow development to a crawl, and poor internal quality of the system being tested will result in poor quality tests.
  38. Christian Johansen Test-Driven JavaScript Development “If you write bad unit

    tests, you might find that you gain none of the benefits, and instead are stuck with a bunch of tests that are time-consuming and hard to maintain.”
  39. $ bx rake test:units ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ...............................................................................................

    ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ............................................................................................... ........................................................................................ Finished in 274.623286 seconds. 2273 tests, 6765 assertions, 0 failures, 0 errors
  40. You don't run them often You waste time waiting for

    tests WHAT’S WRONG WITH SLOW TESTS?
  41. You don't run them often You waste time waiting for

    tests You distracted others while waiting WHAT’S WRONG WITH SLOW TESTS?
  42. You don't run them often You waste time waiting for

    tests You distracted others while waiting WHAT’S WRONG WITH SLOW TESTS? http://xkcd.com/303/
  43. You don't run them often You waste time waiting for

    tests You distracted others while waiting You commit failing changes WHAT’S WRONG WITH SLOW TESTS?
  44. You don't run them often You waste time waiting for

    tests You distracted others while waiting You commit failing changes You lose the rapid feedback cycle WHAT’S WRONG WITH SLOW TESTS?
  45. context Notifications::Emails::Message do setup do @comment = Issue.make! # create

    record in database @settings = Notifications::Settings.new(-1) @message = Notifications::Emails::Message.new( @comment, @settings) end 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
  46. context Notifications::Emails::Message do setup do @comment = Issue.make # create

    in memory @comment.id = -1 # make it appear persisted @settings = Notifications::Settings.new(-1) @message = Notifications::Emails::Message.new( @comment, @settings) end 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
  47. 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
  48. 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
  49. 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
  50. BEFORE $ ruby test/unit/notifications/emails/commit_mention_test.rb .... Finished in 0.576135 seconds. AFTER

    $ ruby test/unit/notifications/emails/commit_mention_test.rb .... Finished in 0.052412 seconds.
  51. $ time ruby test/unit/notifications/email_handler_test.rb ........ Finished in 0.084729 seconds. 8

    tests, 10 assertions, 0 failures, 0 errors real 0m7.065s user 0m4.948s sys 0m1.961s
  52. test/fast/notifications/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
  53. $ time ruby test/fast/notifications/web_handler_test.rb .... Finished in 0.001577 seconds. 4

    tests, 22 assertions, 0 failures, 0 errors real 0m0.139s user 0m0.068s sys 0m0.063s
  54. SLOW TEST MYTHS “Our tests are slow because we have

    too many of them.” “To speed up our tests, we just need to parallelize them.”
  55. SLOW TEST MYTHS “Our tests are slow because we have

    too many of them.” “To speed up our tests, we just need to parallelize them.” “We can’t use test doubles because we’ll loose confidence that it still works.”
  56. God Objects class Issue < ActiveRecord::Base # validations validates_presence_of :title,

    :user_id, :repository_id # associations belongs_to :user
  57. God Objects class Issue < ActiveRecord::Base # validations validates_presence_of :title,

    :user_id, :repository_id # associations belongs_to :user # data integrity before_validation :set_state
  58. God Objects class Issue < ActiveRecord::Base # validations validates_presence_of :title,

    :user_id, :repository_id # associations belongs_to :user # data integrity before_validation :set_state # misc concerns before_save :audit_if_changed
  59. God Objects class Issue < ActiveRecord::Base # validations validates_presence_of :title,

    :user_id, :repository_id # associations belongs_to :user # data integrity before_validation :set_state # misc concerns before_save :audit_if_changed # querying named_scope :watched_by, lambda {|user| ... }
  60. God Objects class Issue < ActiveRecord::Base # validations validates_presence_of :title,

    :user_id, :repository_id # associations belongs_to :user # data integrity before_validation :set_state # misc concerns before_save :audit_if_changed # querying named_scope :watched_by, lambda {|user| ... } # who knows what these do? include Mentionable, Subscribable, Summarizable
  61. God Objects class Issue < ActiveRecord::Base # validations validates_presence_of :title,

    :user_id, :repository_id # associations belongs_to :user # data integrity before_validation :set_state # misc concerns before_save :audit_if_changed # querying named_scope :watched_by, lambda {|user| ... } # who knows what these do? include Mentionable, Subscribable, Summarizable # domain logic def active_participants [self.user] + watchers + commentors end end
  62. MAKE THE FRAMEWORK DEPEND ON YOUR APPLICATION, INSTEAD OF MAKING

    YOUR APPLICATION DEPEND ON THE FRAMEWORK.
  63. 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
  64. …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
  65. 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 def encrypt(password, salt) Digest::SHA1.hexdigest(password+salt) end end
  66. 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 …
  67. 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
  68. 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
  69. describe PasswordAuthentication do 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
  70. class PasswordAuthentication def initialize(username, password) @username = username @password =

    password end def user User.find_by_username(@username) end end
  71. No changes necessary class PasswordAuthentication def initialize(username, password) @username =

    username @password = password end def user User.find_by_username(@username) end end
  72. describe PasswordAuthentication do 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 subject.user.should be_nil end end end end
  73. class PasswordAuthentication # … 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 def encrypt(password, salt) Digest::SHA1.hexdigest(password+salt) end end
  74. describe PasswordAuthentication do describe 'user' do let!(:user) do create :user,

    :username => 'bkeepers', :password => 'testing' # hits the DB :( end # … end end
  75. describe PasswordAuthentication do 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
  76. 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 subject.user.should be_nil end end
  77. POSITIVE FEEDBACK LOOP Unit tests help us isolate our code

    and reduce coupling to frameworks, which makes our tests faster.
  78. 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.’”
  79. 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