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

From Code Smells to Code Sense - RailsConf 2019 - Kyle Annen, Cyrus Vandrevala - 8th Light

From Code Smells to Code Sense - RailsConf 2019 - Kyle Annen, Cyrus Vandrevala - 8th Light

Please help us improve by taking a moment to fill out our post-conference survey!

http://bit.ly/code_smells_survey

Opinionated web frameworks like Rails allow us to focus on what’s unique to our domain, rather than reinventing the wheel. But under deadline pressures, we often find ourselves with a codebase that works in the short term, but takes more and more effort over time to extend, test, and maintain.

To speed our development back up and make coding fun again, we need to be able to recognize issues and refactor them away. In this workshop, we will explore some strategies for identifying common code smells in production Rails apps, and get hands-on practice in cleaning them up.

Cyrus Vandrevala
Cyrus Vandrevala is a crafter at 8th Light who has developed software in a variety of industries covering finance, insurance, travel, and education. He is passionate about sharing knowledge and has helped developers acquire new skills through training sessions including object-oriented development, DevOps philosophies, and Agile methodologies. Before joining 8th Light, Cyrus earned his Ph.D. in physics from Purdue University.

Kyle Annen
Kyle Annen is a software crafter at 8th Light in Chicago who is passionate about functional languages, object oriented languages, DevOps, and building sustainable systems. Kyle learned programming through online open access bootcamps prior to joining 8th Light. His past career in business took him around the world, including building low income housing in the borderlands of Mexico, and he spent half a decade working in Shanghai, China.

47b12cde1d183e92e323f9b8481161af?s=128

Kyle Annen

April 30, 2019
Tweet

Transcript

  1. From Code Smells to Code Sense RailsConf - April 30th,

    2019 (Please Pull Down the Latest Changes in the Repo) bit.ly/rails-footprints
  2. Introductions Cyrus Kyle

  3. We are hiring! • Technical Expert • Consulting Crafter •

    Principal Crafter • Lead Crafter • Crafter We are hiring! • Chicago • Los Angeles • New York • London https://8thlight.com/job-opportunities/
  4. Schedule • Brief Introduction to Code Smells • Code Smells

    in Rails Apps ◦ Model Smells ◦ Controller Smells ◦ View Smells ◦ Test Smells • Final Thoughts
  5. Footprints • Retired HR app written by 8th Light apprentices

    • Was in production use for a number of years • Has since been converted into an educational tool • Chock full of code smells and strange abstractions (again, written by apprentices)
  6. Footprints https://github.com/cmvandrevala/footprints-public or bit.ly/rails-footprints

  7. Workshop Structure • Describe some code smells • Dive into

    potential root causes of these smells • Break out into small groups and discuss smelly code • Reconvene as a large group and report findings Rinse and repeat!
  8. What Exactly is a Code Smell? An identifiable characteristic of

    your source code that might indicate a deeper issue
  9. Schedule • Brief Introduction to Code Smells • Code Smells

    in Rails Apps ◦ Model Smells ◦ Controller Smells ◦ View Smells ◦ Test Smells • Final Thoughts
  10. Origins of the Phrase “Code Smell” • Coined by Kent

    Beck in the 90’s while working with Martin Fowler • Further popularized by Refactoring: Improving the Design of Existing Code by Martin Fowler Martin Fowler has written an excellent blog post describing some of the details of code smells - https://martinfowler.com/bliki/CodeSmell.html
  11. Examples of Code Smells • Overly Large Classes and Methods

    • Long Lists of Parameters • Slow or Flaky Tests • Feature Envy • Long Chains of Messages • And Many More!
  12. Why Is This Topic Important? “Code Smells are signals that

    your code should be refactored in order to improve extendability, readability, and supportability” https://8thlight.com/blog/georgina-mcfadyen/2017/01/19/common-code-smells.html
  13. Schedule • Brief Introduction to Code Smells • Code Smells

    in Rails Apps ◦ Model Smells ◦ Controller Smells ◦ View Smells ◦ Test Smells • Final Thoughts
  14. Code Smells in Models • Fat Models • Model Callbacks

    Create or Call Other Objects
  15. Fat Models Single Responsibility Principle: “A class should have only

    one reason to change” What is the purpose of an ActiveRecord model? “Active Record facilitates the creation and use of business objects whose data requires persistent storage to a database” https://guides.rubyonrails.org/active_record_basics.html
  16. Fat Models Active Record is not your entire business layer!

  17. Fat Models # Active Record Used Well def student? self.skill

    == "student" end def resident? self.skill == "resident" end
  18. Fat Models # Gray Area def outstanding?(how_many) first_notification = Notification.where(:applicant_id

    => self.id, :crafter_id => self.crafter_id).first date = first_notification.try(:created_at) || self.created_at !has_steward && (date < how_many.days.ago if date) end
  19. Fat Models # Not Good (This is in the Applicant

    Model...) def set_crafter_by_name(crafter_name) crafter = Footprints::Repository.crafter.find_by_name(crafter_name) if crafter self.crafter_id = crafter.id else self.crafter_id = nil end end
  20. Callbacks Call or Create Other Objects before_save :set_crafter_data def set_crafter_data

    crafter_name = self.assigned_crafter if crafter_name.blank? set_no_crafter else set_crafter_by_name(crafter_name) end self end
  21. Callbacks Call or Create Other Objects def set_no_crafter self.assigned_crafter =

    nil self.crafter_id = nil self.has_steward = false end def set_crafter_by_name(crafter_name) crafter = Footprints::Repository.crafter.find_by_name(crafter_name) if crafter self.crafter_id = crafter.id else self.crafter_id = nil end end
  22. What is the Takeaway? • Active Record is a powerful

    tool that we love to use • Unfortunately, we try to put everything in Active Record Models • Break business logic out into plain old Ruby objects!
  23. Small Group Discussion Let’s get some groups made! • Break

    into groups 3 - 5 people • Mix of experience levels if possible • At least one computer per group with Footprints
  24. Small Group Discussion • Identify some code smells • Discuss

    some refactors • Discuss trade-offs of various solutions to code smells • Have fun with some interesting code
  25. Small Group Discussion Identify some of the code smells in

    the model! lib/ar_repository/models/applicant.rb
  26. Schedule • Brief Introduction to Code Smells • Code Smells

    in Rails Apps ◦ Model Smells ◦ Controller Smells ◦ View Smells ◦ Test Smells • Final Thoughts
  27. Code Smells in Controllers • Fat controllers • Controller Actions

    know about the internal state of models
  28. Why do controllers get fat? • Business logic in the

    controller • God classes • Acting on the result of an HTTP request • Controllers manage many disparate Models and actions • And more! Fat Controllers
  29. Fat Controllers def assign_crafter ... if automatically_assigned? ApplicantDispatch::Dispatcher.new(applicant,steward) .assign_applicant else

    ApplicantDispatch::Dispatcher.new(applicant, steward) .assign_applicant_specific(chosen_crafter) end ... end
  30. Controller Actions Know About Model Internal State It is totally

    fine for the Controller to tell the Model to update. It is not good for the Controller to “reach into” the Model and update it.
  31. Controller Actions Know About Model Internal State def index render

    json: get_name_suggestions(params[:term]) end def get_name_suggestions(prefix) suggestions = [] results = repo.applicant.where("name like ?", "%#{prefix}%").limit(10) results.each { |a| suggestions << a.name } suggestions end
  32. What is the Takeaway? • Controllers are in charge of

    managing the interactions between the model and the view • They should have no direct knowledge of the structure of the models or views • There are many actions that don’t fit into Active Record Models (e.g. API calls). They should go into plain old Ruby objects instead of directly in the Controller.
  33. Small Group Discussion Identify some of the code smells in

    the controller! app/controllers/applicants_controller.rb (it’s a doozy...)
  34. Schedule • Brief Introduction to Code Smells • Code Smells

    in Rails Apps ◦ Model Smells ◦ Controller Smells ◦ View Smells ◦ Test Smells • Final Thoughts
  35. Code Smells in Views • Extensive Display Logic • Contains

    Business Logic • Inflexible (Hard-Coded Values)
  36. <% if @presenter.has_application_questions? %> <% if @applicant.about != nil %>

    <h2>What's your story?</h2> <p> <%= @applicant.about .gsub(/\n/, '<br/>') .delete("\\") .delete("---") .html_safe %> </p> <% end %> Extensive Display Logic in Views
  37. Contains Business Logic in Views <%= f.select( :chosen_crafter, options_for_select( Footprints::Repository.crafter.all.map

    { |employee| [employee['name'], employee['name']] }.unshift([app.assigned_crafter, 'current'])), {}, class: "dropdown-height dropdown") %>
  38. Inflexible (Hard-Coded Values) <div class='form-whole-width'> <%= form.label :location %> <p>

    <%= form.select :location, ["Chicago", "London", "Los Angeles"], {}, class: "dropdown", style: "width: 140px" %> </p> </div>
  39. What is the Takeaway? • Views should be working off

    of pre-defined values and hashes • Helper methods might be a good way to encapsulate repeated view logic
  40. Small Group Discussion Identify some of the code smells in

    the view! app/views/analytics/index.html.erb
  41. Tools You Might Consider • Reek - https://github.com/troessner/reek • Code

    Metrics - https://www.ruby-toolbox.com/categories/code_metrics
  42. Any Questions So Far?

  43. Schedule • Brief Introduction to Code Smells • Code Smells

    in Rails Apps ◦ Model Smells ◦ Controller Smells ◦ View Smells ◦ Test Smells • Final Thoughts
  44. Let’s Focus Our Scope • Tests Are Difficult to Read/Write

    • Tests Are Slow Running • Tests Are Brittle or Fail Intermittently
  45. Tests Are Difficult to Read/Write

  46. Tests Are Difficult to Read/Write • Overly complicated shared contexts

    and examples • Many include statements (DRY tests) • Large before or before(:each) blocks
  47. The User Mindset Approach your tests with the mindset that

    your test suite is a “user” of your source code (e.g. another developer). If tests are difficult to read and write, that is analogous to another developer finding it difficult to expand on your code! • Asynchronous code • Untestable code • Overly complicated setup/teardown • Highly coupled code (I’m looking at you, ActiveRecord…)
  48. Overly Complicated Shared Contexts and Examples # requires user and

    persona to be defined shared_context "setup user address decorator integration" do let(:addresses) { AddressFactory.create(user, 40) } let(:location) { seed_locations(user, address) } let(:partner) { PartnerFactory.create(user, location) } let(:household) { setup_household(user, partner, assets, location) } before do # requires household to be defined setup_test end # tests would be here end
  49. Overly Complicated Shared Contexts and Examples # spec/decorator/spec_helper.rb def setup_household_for_decoration(user,

    address, assets) addresses = AddressFactory.create(user, 40) location = seed_locations(user, address) partner = PartnerFactory.create(user, location) setup_household(user, partner, assets, location) end
  50. Overly Complicated Shared Contexts and Examples # spec/decorator/household_decorator_spec.rb require "spec/decorator/spec_helper.rb"

    describe HouseholdDecorator do before(:all) do user = FactoryBot.build(:user) address = FactoryBot.build(:address) assets = FactoryBot.build(:assets) household = setup_household_for_decoration(user, address, assets) setup_test(house_hold) end it "..." do end end
  51. Shared Context & Shared Examples Two minds Only general shared

    examples or shared contexts No shared examples or shared contexts
  52. Using Generalized Shared Examples Using rspec keywords it "not valid

    on build with no parameters" do model = subject.build expect(model.valid?).to be(false) end it "is valid with valid params" do model = subject.build(valid_params) expect(model.valid?).to be(true) end it "is invalid with invalid params" do model = subject.build(invalid_params) expect(model.valid?).to be(false) end
  53. Many Include Statements in Spec File (DRY Tests) # spec/integration/services/crafter_decorator.rb

    require "integration/decorators/crafter_spec" require "integration/decorators/address_spec" require "integration/decorators/profile_spec" require "integration/decorators/resume_spec" require "integration/decorators/experience_spec" describe "Crafter decorator decorates a crafter relation" do include DecoratorHelper include_context "setup crafter integration" include_context "setup rendering variables" describe "models" do include_examples "non model rendering params" describe "CrafterModel" do include_examples "decorates the crafter" end ...
  54. Many Include Statements in Spec File (DRY Tests) We want

    our code to be DRY • Don’t Repeat Yourself We want our tests to be DAMP • Descriptive and Meaningful Phrases)
  55. Many Include Statements in Spec File (DRY Tests) it "is

    a test" do # Setup # Exercise # Verify # Teardown end Focus on the Four Phase Test:
  56. Before… but when? Before is a bit misleading before(:all) do

    puts "before(:all)" end before(:each) do puts "before(:each)" end before do puts "before" end
  57. Before… but when? Before is a bit misleading Randomized with

    seed 54912 before(:all) before(:each) before .before(:each) before .before(:each) before .before(:each) before .before(:each) before . Call Total before(:all) => 1 before(:each) => 5 before called => 5
  58. What is the Takeaway? • Your tests can be thought

    of as a “user” of your code • If you find something hard to test, it is probably hard to use • Large numbers of nested include or before statements can create misdirection - you may be setting the stage for a pattern that will persist and get more complicated
  59. Let’s Discuss Some Tests! 1. What are some test smells

    that you see? 2. What do you think a root cause of the smell might be? 3. What would be a good refactoring for this test smell? /spec/footprints-ar-repository/user_repository_spec.rb
  60. Tests Are Slow Running Minutes Per File, Instead of Milliseconds

    or Seconds
  61. Tests Are Slow Running • Using Build vs. Create •

    Dealing with pre-existing, slow test suites ◦ ParallelTests ◦ Potential Causes of Slow Tests
  62. The Testing Pyramid + Fast, Focused, Descriptive - Does Not

    Indicate Working System + Acceptance Level, Validates System - Slow, Not Focused
  63. The Testing Pyramid Slow tests might be an indication that

    you are creeping up the pyramid...
  64. Using Build vs. Create Creating records is a slow operation

    describe Car do it "tests a rails model directly" do car = Car.create(make: "Tesla") expect(car.make).to eq("Tesla") end end
  65. Using Build vs. Create Only create when needed, build otherwise

    (in memory). Limit database hits. describe Garage do it "a performant test" do car = Car.build(make: "Tesla") garage = Garage.build(car: car) expect(garage.car.make).to eq("Tesla") end end
  66. Using Build vs. Create If you need to reach out

    to the database, do it as infrequently as possible. describe Applicant do before(:all) do @applicant = Applicant.create(name: "Me", email: "me@foo.com") end it "accepts a name" do expect(@applicant.name).to eq("Me") end it "accepts a color" do expect(@applicant.email).to eq("me@foo.com") end end
  67. Using Build vs. Create Cannot test some relations with in

    memory ActiveRecord relations. “Adding an object to a collection (#has_many or #has_and_belongs_to_many) automatically saves that object, except if the parent object (the owner of the collection) is not yet stored in the database.” https://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html
  68. But Our Test Suite is Slow Already!

  69. Profile Your Tests! How do we find out the source

    of a slow-running test suite? Profile your tests! $ bundle exec rspec spec/ --profile
  70. Some Tools for Profiling Ruby • StackProf • FlameProf •

    EventProf • RBSpy
  71. Profiling Isn’t Enough for Us… Results NOW!!! We have 36,000

    tests... And rspec uses a single core... Why not (try to) use all of the cores?
  72. Introducing ParallelTests $ gem install parallel_tests

  73. Introducing ParallelTests • Databases: One instance per core • Rails

    cache: memory • Can be done with interpolation similar to MySQL DB
  74. Introducing ParallelTests Setting up the database(s) # database.yml mysql: adapter:

    mysql2 database: development_footprints<%= ENV['TEST_ENV_NUMBER'] %> # clone the databases $ rake parallel:create $ rake parallel:prepare
  75. Other Causes of Slow Tests • Huge spec_helper loaded in

    every spec • FactoryBot factory composition • Serialization and deserialization ◦ OJ (Optimized JSON) - https://github.com/ohler55/oj ◦ MultiJSON ◦ Increases performance in application code at the same time!
  76. What is the Takeaway? • Slow tests are often caused

    by creep up the testing pyramid • Careful consideration should be taken when choosing between build and create • Parallelization is not a cure for slow tests. ◦ It can be a temporary fix ◦ It can be an enforcer of good tests ◦ It can help you scale your app in the future
  77. No Exercise Here! We Don’t Want to Wait Around While

    Slow Tests Run
  78. Tests Are Brittle or Fail Intermittently

  79. • Tests might be dependent on the database state ◦

    Eliminate Assertions Reliant on Database State ◦ Use Universally Unique Identifiers ◦ Randomize Test Order Tests Are Brittle or Fail Intermittently
  80. Coupling vs. Cohesion What is wrong with the diagram on

    the right? • High Coupling • Low Cohesion
  81. Coupling vs. Cohesion

  82. Eliminate Assertions Reliant on Database State Tests ought to be

    able to be run concurrently similar to our application code. expect { user.documents.create(file: Faker::File) }.to change{ Document.count }.by(1)
  83. Eliminate Assertions Reliant on Database State Test assertions should not

    depend on database state, but rather on the state of a relation. user = User.create(uuid: SecureRandom.uuid) # works, but dependent on database state expect { user.address.create(street: "25 East Washington, Suite 509") }.to change{ Address.count }.by(1) # better, dependent on relation, not brittle expect { user.address.create(street: "25 East Washington, Suite 509") }.to change{ user.contacts.count }.by(1)
  84. Eliminate Assertions Reliant on Database State Reusable, not dependent on

    state, in memory def setup_crafter crafter = Crafter.build(name: Faker::Person.name) application = crafter.build_application( file: "tmp/file/path", date: Date.today ) crafter end
  85. Use Universally Unique Identifiers • Eliminates collisions • Reduces chance

    of flaky tests • Enables use of database regardless of state • UUID or GUID are common
  86. Use Universally Unique Identifiers • UUID - Universally Unique Identifier

    • Example of a UUID: 671549be-0aa0-4fe4-806b-ba385544a7a1 • SecureRandom is shipped with Ruby. • 2.71 quintillion UUIDs needed for a 50% collision chance
  87. Use Universally Unique Identifiers irb(main):001:0> require "securerandom" => true irb(main):002:0>

    SecureRandom.uuid => "a4e21991-bcaa-4b81-aff4-153fa346471a"
  88. Use Universally Unique Identifiers uuid = SecureRandom.uuid User.create(uuid: SecureRandom.uuid) documents

    = Document.where(user_uuid: uuid) expect(documents.count).to eq(1)
  89. Randomize Test Order Use the rspec option: `--order defined` $

    rspec spec --order defined spec/file1.rb spec/file2.rb
  90. Randomize Test Order - A Helpful Script file_with_failure = "spec/folder/flaky_spec.rb"

    spec_files = Dir["spec/folder/**/*_spec.rb"] .reject {|file| file == file_with_failure} spec_files.each do |spec_file| command = “rspec spec --order defined” result = system("#{command} #{spec_file} #{file_with_failure}" result = {outcome: outcome, first_file: spec_file} File.open("tmp/spec_order.log", "a"){ |file| file.puts result } end Gist for the script: http://bit.ly/rspec_order_script
  91. What is the Takeaway? • Coupling between the database and

    Rails objects can make tests brittle due to the fact that they might depend on database state. • Techniques like randomizing test order and leveraging UUIDs can help decouple your code from the database, making tests faster and more robust. • You should still have acceptance level tests that assert that all of the components of your app work together!
  92. Let’s Discuss Some Tests! 1. What are some test smells

    that you see? 2. What do you think a root cause of the smell might be? 3. What would be a good refactoring for this test smell? /spec/controllers/applicants_controller_spec.rb /spec/footprints/reporting/employment_data_generator_spec.rb
  93. What did we cover today? • The definition of a

    code smell • Why are code smells important? Final Thoughts
  94. What did we cover today? • A Few Code Smells

    in Models ◦ Fat Models ◦ Model Callbacks Create or Call Other Objects Final Thoughts
  95. What did we cover today? • A Few Code Smells

    in Controllers ◦ Fat Controllers ◦ Controller Actions Know About the Internal State of Models Final Thoughts
  96. What did we cover today? • A Few Code Smells

    in Views ◦ Extensive Display Logic ◦ Contains Business Logic ◦ Inflexible (Hard-Coded Values) Final Thoughts
  97. What did we cover today? • A Few Code Smells

    in Tests ◦ Tests Are Difficult to Read/Write ◦ Tests Are Slow Running ◦ Tests Are Brittle or Fail Intermittently Final Thoughts
  98. Questions? Thank you for your time! Please help us improve

    by taking a moment to fill out our post-conference survey http://bit.ly/code_smells_survey