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

Refactoring Hacken.in

Refactoring Hacken.in

This is a presentation I gave at our amazing local Ruby Usergroup Cologne.rb. This is a story where I took the Open Source Rails Application http://hacken.in and refactored one controller. I show the way I think when I do a refactoring like that.

Feedback is more than welcome :)

Lucas Dohmen

January 15, 2014
Tweet

More Decks by Lucas Dohmen

Other Decks in Programming

Transcript

  1. RWTH Aachen, Computer Science Student triAGENS GmbH, Developer moonglum moonbeamlabs

    by Lucas Dohmen Refactoring Hacken.in or: It started as a redesign
  2. I started with a redesign • The amazing @liane_thoennes is

    redesigning hacken.in • I started to implement that on a branch • Delete all old CSS (Sass), Bootstrap -> Foundation, some minor changes in HAML
  3. Some of you may ask: How did you get the

    video in an acceptable quality?
  4. I started “refactoring” • Currently hacken.in is not really well

    tested • This makes refactoring hard • I’m treating the code as untested
  5. class CalendarsController < ApplicationController ! def show @categories = Category.all

    ! # Die Monate, die angezeigt werden begin @start_date = params[:start].present? ? Date.parse(params[:start]) : Date.today rescue ArgumentError @start_date = Date.today flash.now[:error] = 'Das war kein gültiges Datum... […]’ end ! @months = [] 13.times { |i| @months << (@start_date + i.months) } ! if current_region.nil? raise ActionController::RoutingError.new('Not Found') end ! @single_events = SingleEvent.in_next_from(4.weeks, @start_date).in_region(@region) @single_events.to_a.select! { |single_event| single_event.is_for_user? current_user } if current_user @single_events.sort! end ! end Assigned, never used
  6. class CalendarsController < ApplicationController ! def show @categories = Category.all

    ! # Die Monate, die angezeigt werden begin @start_date = params[:start].present? ? Date.parse(params[:start]) : Date.today rescue ArgumentError @start_date = Date.today flash.now[:error] = 'Das war kein gültiges Datum... […]’ end ! @months = [] 13.times { |i| @months << (@start_date + i.months) } ! if current_region.nil? raise ActionController::RoutingError.new('Not Found') end ! @single_events = SingleEvent.in_next_from(4.weeks, @start_date).in_region(@region) @single_events.to_a.select! { |single_event| single_event.is_for_user? current_user } if current_user @single_events.sort! end ! end Param Parsing
  7. begin @start_date = params[:start].present? ? Date.parse(params[:start]) : Date.today rescue ArgumentError

    @start_date = Date.today flash.now[:error] = 'Das war kein gültiges Datum... […]’ end Step 1: Remove the Flash + the Comment • The ArgumentError falls back to a sensible default • It’s only reached when someone is “fiddling around” • The comment was a lie
  8. begin @start_date = params[:start].present? ? Date.parse(params[:start]) : Date.today rescue ArgumentError

    @start_date = Date.today end @start_date = Date.parse(params[:start]) rescue Date.today First Attempt:
  9. @start_date = Date.parse(params[:start]) rescue Date.today My first Attempt: @start_date ||=

    params[:start].present? ? Date.parse(params[:start]) : Date.today Our solution: • params[:start] can only be a false value if someone is setting it by hand – therefore it can result in an error page • State the intent that we want to check if the param exists
  10. class CalendarsController < ApplicationController ! def show @start_date ||= params[:start].present?

    ? Date.parse(params[:start]) : Date.today ! @months = [] 13.times { |i| @months << (@start_date + i.months) } ! if current_region.nil? raise ActionController::RoutingError.new('Not Found') end ! @single_events = SingleEvent.in_next_from(4.weeks, @start_date).in_region(@region) @single_events.to_a.select! { |single_event| single_event.is_for_user? current_user } if current_user @single_events.sort! end ! end Fill an array with months
  11. class CalendarsController < ApplicationController ! def show @start_date ||= params[:start].present?

    ? Date.parse(params[:start]) : Date.today ! @months = [] 13.times { |i| @months << (@start_date + i.months) } ! if current_region.nil? raise ActionController::RoutingError.new('Not Found') end ! @single_events = SingleEvent.in_next_from(4.weeks, @start_date).in_region(@region) @single_events.to_a.select! { |single_event| single_event.is_for_user? current_user } if current_user @single_events.sort! end ! end Check for a missing param
  12. class CalendarsController < ApplicationController ! def show raise ActionController::RoutingError.new('Not Found’)

    if current_region.nil? ! @start_date ||= params[:start].present? ? Date.parse(params[:start]) : Date.today ! @months = [] 13.times { |i| @months << (@start_date + i.months) } ! @single_events = SingleEvent.in_next_from(4.weeks, @start_date).in_region(@region) @single_events.to_a.select! { |single_event| single_event.is_for_user? current_user } if current_user @single_events.sort! end ! end
  13. class CalendarsController < ApplicationController ! def show raise ActionController::RoutingError.new('Not Found’)

    if current_region.nil? ! @start_date ||= params[:start].present? ? Date.parse(params[:start]) : Date.today ! @months = [] 13.times { |i| @months << (@start_date + i.months) } ! @single_events = SingleEvent.in_next_from(4.weeks, @start_date).in_region(@region) @single_events.to_a.select! { |single_event| single_event.is_for_user? current_user } if current_user @single_events.sort! end ! end Parse Input Business Logic
  14. Domain World • The domain world should not be aware

    that we’re on a webpage • Arguably it should not even know about the database, but that’s a topic for another day…
  15. The Controller • Translates from “web lingo” to “domain lingo”

    • It gets a set of models (or presenters) from the domain world • It provides the view with those models/ presenters
  16. class CalendarsController < ApplicationController ! def show raise ActionController::RoutingError.new('Not Found’)

    if current_region.nil? ! @start_date ||= params[:start].present? ? Date.parse(params[:start]) : Date.today ! @months = [] 13.times { |i| @months << (@start_date + i.months) } ! @single_events = SingleEvent.in_next_from(4.weeks, @start_date).in_region(@region) @single_events.to_a.select! { |single_event| single_event.is_for_user? current_user } if current_user @single_events.sort! end ! end class CalendarsController < ApplicationController ! def show raise ActionController::RoutingError.new('Not Found’) if current_region.nil? ! start_date ||= params[:start].present? ? Date.parse(params[:start]) : Date.today ! @start_selector = StartSelector.new(params) @calendar = Calendar.new(params) end ! end What I want: What we have:
  17. class CalendarsController < ApplicationController ! def show raise ActionController::RoutingError.new('Not Found’)

    if current_region.nil? ! start_date ||= params[:start].present? ? Date.parse(params[:start]) : Date.today ! @start_selector = StartSelector.new(params) @calendar = Calendar.new(params) end ! end • The current_region we are in (ApplicationController) • The current_user that is logged in (Devise) We have two additional parsed params: @start_selector = StartSelector.new(start_date) @calendar = Calendar.new(start_date, current_region, current_user)
  18. So I created two new classes • Both don’t inherit

    from a Rails class • They therefore don’t need Rails when running their test suites • I created a spec_helper_without_rails • … and following Bernhardt’s advice I put them into lib
  19. Were do we put Domain Models? • Domain Models with

    Rails: app/model • But what about Models without Rails? • Dirk and I decided that they still belong in app/model • The only difference is the spec helper they require
  20. What did we win by extracting those classes? • The

    controller code is very clean and easy to understand now • The two models are easy to test, we don’t need Rails – they have defined inputs and outputs • They turned out to be magnets for code spread across the View
  21. Changes in the View • I introduced presenters • I

    pulled almost all logic out of the views
  22. The Presenters • They don’t inherit anything • They get

    a value they should wrap in their constructor • They use one trick to leverage Rails
  23. #calendar-entries - calendar.entries.each |entry| = render partial: 'entry', locales: {

    entry: entry } Before: After: #calendar-entries= render calendar.entries
  24. What does render do when you provide a collection? •

    It iterates over the array, for each element: • It calls the methods to_partial_path • It renders the partial with that path and the object
  25. All we need to do is implementing the method •

    With this tiny trick each of our presenters has its own partial • They form a nice unit • I think I will put each of the presenter templates into app/views/presenters – not sure yet