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 :)

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. This was a video in the presentation ;)
4. Some of you may ask: How did you get the video in an acceptable quality?
5. This was a video in the presentation ;)
6. I started “refactoring” • Currently hacken.in is not really well tested • This makes refactoring hard • I’m treating the code as untested
7. Refactoring untested code • Extract units of code that belong together
8. The Controller
9. 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
10. 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
11. 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
12. 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:
13. Then Dirk jumped in What are you doing?
14. @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
15. 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
16. 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
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 ! @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
18. 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
19. What I think a Controller should do
20. Controller Web World Domain World Cookie current_user params[:date] @start_date View Router
21. 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…
22. 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
23. Domain Object: Start Selector Domain Object: Calendar
24. 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:
25. 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)
26. 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
27. Then Dirk jumped in What are you doing?
28. 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
29. 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
30. Changes in the View • I introduced presenters • I pulled almost all logic out of the views
31. The Presenters • They don’t inherit anything • They get a value they should wrap in their constructor • They use one trick to leverage Rails
32. #calendar-entries - calendar.entries.each |entry| = render partial: 'entry', locales: { entry: entry } Before: After: #calendar-entries= render calendar.entries
33. 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
34. 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
35. What do you do when you refactor Rails apps?