A (re)-factoring Story

A set of example a up an comer ruby programer can use to improve their code.

Transcript

1. (re) Factoring Friday, June 8, 12
2. mejorar el código Friday, June 8, 12
3. Friday, June 8, 12
4. Friday, June 8, 12
5. REFACTOR Friday, June 8, 12
6. @ivanacostarubio bakedweb.net Friday, June 8, 12
7. Friday, June 8, 12
8. def it_knows_the_positive_reviews Review.create(:face => “happy”) assert_equal 1, attempt.positive_review end Friday, June 8, 12
9. def positive_reviews happyfaces = 0 reviews.each do |review| happyfaces += 1 if review.face == "happy" end happyfaces end Friday, June 8, 12
10. def positive_reviews reviews.reject { |review| review.sad_review? } end Friday, June 8, 12
11. Estudia la libreria Standard Friday, June 8, 12
12. +-105 Clases en Ruby Core Friday, June 8, 12
13. ¿Qué otros tipos de mejoras? Friday, June 8, 12
14. Friday, June 8, 12
15. Friday, June 8, 12
16. 5+ Minutos para generar los reportes! Friday, June 8, 12
17. :joins => :nos_mataron Friday, June 8, 12
18. let’s kill them Friday, June 8, 12
19. Aleluya por CUCUMBERS Friday, June 8, 12
20. Aleluya por cucumber las pruebas Friday, June 8, 12
21. It made the refactoring easier Friday, June 8, 12
22. Friday, June 8, 12
23. def get_report_data return if session[:account].blank? or session[:account].zero? start_date = params[:start].blank? ? (session[:report_start_date].blank? ? Date.today - 8.days : Date.parse(session[:report_start_date])) : Date.civil(params[:start][:year].to_i, params[:start][:month].to_i, params[:start][:day].to_i) end_date = params[:end].blank? ? (session[:report_end_date].blank? ? Date.today - 1.days : Date.parse(session[:report_end_date])) : Date.civil(params[:end][:year].to_i, params[:end][:month].to_i, params[:end][:day].to_i) end_date = start_date if start_date > end_date session[:report_end_date] = end_date.to_s session[:report_start_date] = start_date.to_s type = action_name session[:report_type] = type if session[:account].blank? or session[:account].zero? @venues = Venue.find(:all) end span = case (end_date - start_date).to_i when 0:'day' when 1..8:'week' else 'month' end superuser_data = { :total_events => 0, :events_with_data => 0, :accounts => {}, :venues => {} } total_reservations = 0 total_guests = 0 total_passes = 0 report_venues = [] # Only venues with events show up in the reports. @venues.each do |venue| events = venue.events.find(:all, :conditions => [ "date >= :start_date AND date <= :end_date", { :start_date => start_date, :end_date => end_date } ], :order => 'date').uniq # This 'uniq' is required because this query can return # the same event record more than once in the response. venue_reservations = 0 venue_guests = 0 venue_passes = 0 venue_revenue = 0 venue_returns = 0 venue_tables = 0 staff_stats = {} daily_staff_stats = {} meta_events = [] events.each do |event| superuser_data[:total_events] += 1 event_reservations = 0 event_guests = 0 event_passes = 0 event_revenue = 0 event_returns = 0 event_tables = 0 # Get total revenue for each staff member. staff_data = {} staff_data = {} reservation_count = event.reservations.find(:all).size total_reservations += reservation_count venue_reservations += reservation_count event_reservations += reservation_count reservations = event.reservations.find(:all, :conditions => "total_spend > 0 or checked_in") reservations.each do |reservation| next if reservation.staff_id.blank? # Skip orphan reservations. # For event: staff_data[reservation.staff_id] = {} if staff_data[reservation.staff_id].nil? staff_data[reservation.staff_id][:total_revenue] = 0 if staff_data[reservation.staff_id][:total_revenue].nil? staff_data[reservation.staff_id][:total_revenue] += reservation.total_spend unless reservation.total_spend.nil? staff_data[reservation.staff_id][:tables] = {} if staff_data[reservation.staff_id][:tables].nil? staff_data[reservation.staff_id][:tables][reservation.table_id] = 1 event_revenue += reservation.total_spend unless reservation.total_spend.nil? event_tables += 1 # For date: date_hash = reservation.event.date.to_s :db staff_stats[reservation.staff_id] = {} if staff_stats[reservation.staff_id].nil? staff_stats[reservation.staff_id][:total_revenue] = 0 if staff_stats[reservation.staff_id][:total_revenue].nil? staff_stats[reservation.staff_id][:total_revenue] += reservation.total_spend unless reservation.total_spend.nil? daily_staff_stats[date_hash] = {} if daily_staff_stats[date_hash].nil? daily_staff_stats[date_hash][reservation.staff_id] = {} if daily_staff_stats[date_hash][reservation.staff_id].nil? daily_staff_stats[date_hash][reservation.staff_id][:total_revenue] = 0 if daily_staff_stats[date_hash][reservation.staff_id][:total_revenue].nil? daily_staff_stats[date_hash][reservation.staff_id][:total_revenue] += reservation.total_spend unless reservation.total_spend.nil? daily_staff_stats[date_hash][reservation.staff_id][:tables] = {} if daily_staff_stats[date_hash][reservation.staff_id][:tables].nil? daily_staff_stats[date_hash][reservation.staff_id][:tables][reservation.table_id] = 1 # For venue: venue_revenue += reservation.total_spend unless reservation.total_spend.nil? venue_tables += 1 # Superuser totals superuser_data[:accounts][reservation.account_id] = 1 unless reservation.account_id.nil? superuser_data[:venues][venue.id] = 1 end # Get guest list returns for each staff member. unless type.eql? 'staff' guests = event.guests.find(:all, :conditions => "checked_in = 1 OR total_returns > 0") guest_count = guests.size guests.each do |guest| next if guest.guestlist.staff_id.blank? staff_data[guest.guestlist.staff_id] = {} if staff_data[guest.guestlist.staff_id].nil? staff_data[guest.guestlist.staff_id][:total_returns] = 0 if staff_data[guest.guestlist.staff_id][:total_returns].nil? staff_data[guest.guestlist.staff_id][:total_returns] += guest.total_returns unless guest.total_returns.nil? staff_data[guest.guestlist.staff_id][:guestlist_returns] = 0 if staff_data[guest.guestlist.staff_id][:guestlist_returns].nil? staff_data[guest.guestlist.staff_id][:guestlist_returns] += guest.total_returns unless guest.total_returns.nil? event_returns += guest.total_returns unless guest.total_returns.nil? total_guests += guest.total_returns unless guest.total_returns.nil? venue_guests += guest.total_returns unless guest.total_returns.nil? event_guests += guest.total_returns unless guest.total_returns.nil? date_hash = guest.guestlist.event.date.to_s :db staff_stats[guest.guestlist.staff_id] = {} if staff_stats[guest.guestlist.staff_id].nil? staff_stats[guest.guestlist.staff_id][:total_returns] = 0 if staff_stats[guest.guestlist.staff_id][:total_returns].nil? staff_stats[guest.guestlist.staff_id][:total_returns] += guest.total_returns unless guest.total_returns.nil? staff_stats[guest.guestlist.staff_id][:guestlist_returns] = 0 if staff_stats[guest.guestlist.staff_id][:guestlist_returns].nil? staff_stats[guest.guestlist.staff_id][:guestlist_returns] += guest.total_returns unless guest.total_returns.nil? daily_staff_stats[date_hash] = {} if daily_staff_stats[date_hash].nil? daily_staff_stats[date_hash][guest.guestlist.staff_id] = {} if daily_staff_stats[date_hash][guest.guestlist.staff_id].nil? daily_staff_stats[date_hash][guest.guestlist.staff_id][:total_returns] = 0 if daily_staff_stats[date_hash][guest.guestlist.staff_id][:total_returns].nil? daily_staff_stats[date_hash][guest.guestlist.staff_id][:total_returns] += guest.total_returns unless guest.total_returns.nil? daily_staff_stats[date_hash][guest.guestlist.staff_id][:guestlist_returns] = 0 if daily_staff_stats[date_hash][guest.guestlist.staff_id][:guestlist_returns].nil? daily_staff_stats[date_hash][guest.guestlist.staff_id][:guestlist_returns] += guest.total_returns unless guest.total_returns.nil? venue_returns += guest.total_returns unless guest.total_returns.nil? superuser_data[:accounts][guest.guestlist.account_id] = 1 unless guest.guestlist.account_id.nil? superuser_data[:venues][venue.id] = 1 end # Get guest list returns for each staff member. unless type.eql? 'staff' passes = event.passes.find(:all, :conditions => "total > 0") pass_count = passes.size passes.each do |pass| next if pass.staff_id.blank? staff_data[pass.staff_id] = {} if staff_data[pass.staff_id].nil? staff_data[pass.staff_id][:total_returns] = 0 if staff_data[pass.staff_id][:total_returns].nil? staff_data[pass.staff_id][:total_returns] += pass.total unless pass.total.nil? staff_data[pass.staff_id][:pass_returns] = 0 if staff_data[pass.staff_id][:pass_returns].nil? staff_data[pass.staff_id][:pass_returns] += pass.total unless pass.total.nil? event_returns += pass.total unless pass.total.nil? total_passes += pass.total unless pass.total.nil? venue_passes += pass.total unless pass.total.nil? event_passes += pass.total unless pass.total.nil? date_hash = pass.event.date.to_s :db staff_stats[pass.staff_id] = {} if staff_stats[pass.staff_id].nil? staff_stats[pass.staff_id][:total_returns] = 0 if staff_stats[pass.staff_id][:total_returns].nil? staff_stats[pass.staff_id][:total_returns] += pass.total unless pass.total.nil? staff_stats[pass.staff_id][:pass_returns] = 0 if staff_stats[pass.staff_id][:pass_returns].nil? staff_stats[pass.staff_id][:pass_returns] += pass.total unless pass.total.nil? daily_staff_stats[date_hash] = {} if daily_staff_stats[date_hash].nil? daily_staff_stats[date_hash][pass.staff_id] = {} if daily_staff_stats[date_hash][pass.staff_id].nil? daily_staff_stats[date_hash][pass.staff_id][:total_returns] = 0 if daily_staff_stats[date_hash][pass.staff_id][:total_returns].nil? daily_staff_stats[date_hash][pass.staff_id][:total_returns] += pass.total unless pass.total.nil? daily_staff_stats[date_hash][pass.staff_id][:pass_returns] = 0 if daily_staff_stats[date_hash][pass.staff_id][:pass_returns].nil? daily_staff_stats[date_hash][pass.staff_id][:pass_returns] += pass.total unless pass.total.nil? venue_returns += pass.total unless pass.total.nil? superuser_data[:accounts][pass.account_id] = 1 unless pass.account_id.nil? superuser_data[:venues][venue.id] = 1 end end end # Sort by revenue and put a whole object into an array. meta_staff = [] staff_data.keys.each do |staff_id| unless staff_id.blank? begin person = Staff.find(staff_id) rescue next # Orphan reservations cause this. end tables = (staff_data[staff_id].nil? or staff_data[staff_id][:tables].nil?) ? 0 : staff_data[staff_id][:tables].keys.size revenue = staff_data[staff_id].nil? ? 0 : staff_data[staff_id][:total_revenue] returns = (staff_data.nil? or staff_data[staff_id].nil?) ? 0 : staff_data[staff_id][:total_returns] guestlists = (staff_data.nil? or staff_data[staff_id].nil?) ? 0 : staff_data[staff_id][:guestlist_returns] passes = (staff_data.nil? or staff_data[staff_id].nil?) ? 0 : staff_data[staff_id][:pass_returns] unless (tables.blank? or tables.zero?) and (revenue.blank? or revenue.zero?) and (returns.blank? or returns.zero?) meta_staff << { :object => person, :total_tables => tables, :total_revenue => revenue || 0, :total_returns => returns, :guestlist_returns => guestlists, :pass_returns => passes } end end end meta_staff.sort! {|a,b| b[:total_revenue] <=> a[:total_revenue]} meta_events << { :object => event, :staff => meta_staff, :total_reservations => event_reservations, :total_guests => event_guests, :total_passes => event_passes, :total_tables => event_tables, :total_returns => event_returns, :total_revenue => event_revenue } superuser_data[:events_with_data] += 1 if(event_tables > 0 or event_returns > 0 or event_revenue > 0) end # Sort by revenue and put a whole object into an array. dates = [] daily_staff_stats.keys.sort.each do |date_hash| dates << Date.parse(date_hash) end meta_staff = [] staff_stats.keys.each do |staff_id| unless staff_id.blank? begin person = Staff.find(staff_id) rescue next end tables = (staff_stats[staff_id].nil? or staff_stats[staff_id][:tables].nil?) ? 0 : staff_stats[staff_id][:tables].keys.size revenue = staff_stats[staff_id].nil? ? 0 : staff_stats[staff_id][:total_revenue] returns = staff_stats[staff_id].nil? ? 0 : staff_stats[staff_id][:total_returns] unless (tables.blank? or tables.zero?) and (revenue.blank? or revenue.zero?) and (returns.blank? or returns.zero?) # logger.warn "STAFF: #{person.name} '#{tables}' '#{revenue}' '#{returns}'" meta_staff << { :object => person, :total_tables => tables, :total_revenue => revenue, :total_returns => returns } end end end meta_staff.sort! {|a,b| (b[:total_revenue].blank? ? 0 : b[:total_revenue]) <=> (a[:total_revenue].blank? ? 0 : a[:total_revenue]) } if events.size > 0 report_venues << { :object => venue, :events => meta_events, # Array of meta hashes for events with object pointer and meta data. :staff => meta_staff, # A sorted list of staff for this venue, for this report. Only totals. :daily_staff_stats => daily_staff_stats, # The actual day-by-day breakdown for staff members. :dates => dates, # Array of meta hashes for each date. :total_reservations => venue_reservations, :total_guests => venue_guests, :total_tables => venue_tables, :total_returns => venue_returns, :total_revenue => venue_revenue } end end venue_data_index = {} report_venues.each_with_index do |venue, index| venue_data_index[venue[:object].id] = index end if span.eql? 'month' report_venues.each do |venue| weeks = {} venue[:weekly_staff_stats] = {} venue[:staff].each do |person| venue[:dates].each do |date| days_in = date - start_date week_number = (days_in / 7).to_i weeks[week_number] = true venue[:weekly_staff_stats][week_number] = {} if venue[:weekly_staff_stats][week_number].nil? if venue[:weekly_staff_stats][week_number][person[:object].id].nil? venue[:weekly_staff_stats][week_number][person[:object].id] = { :total_revenue => 0, :total_returns => 0 } end stats_for_date = venue[:daily_staff_stats][date.to_s(:db)] next if stats_for_date[person[:object].id].nil? stats_for_week = venue[:weekly_staff_stats][week_number] stats_for_week[person[:object].id] = {} if stats_for_week[person[:object].id].nil? stats_for_week[person[:object].id][:total_revenue] += stats_for_date[person[:object].id][:total_revenue] unless stats_for_date[person[:object].id][:total_revenue].nil? stats_for_week[person[:object].id][:total_returns] += stats_for_date[person[:object].id][:total_returns] unless stats_for_date[person[:object].id][:total_returns].nil? end end venue[:weeks] = weeks.keys.sort end end # logger.warn "VENUES: #{report_venues.to_json}" # logger.warn "DATA: #{superuser_data[:venues].keys.join(", ")}" superuser_data[:total_reservations] = total_reservations superuser_data[:total_guests] = total_guests @report_data = { :start_date => start_date, :end_date => end_date, :span => span, :type => type, :venues => report_venues, :venue_data_index => venue_data_index, :superuser_data => superuser_data } end un metodo en el controllador de 356 lineas Friday, June 8, 12
24. gem install fog Friday, June 8, 12
25. https://codeclimate.com/ Friday, June 8, 12
26. +- 20 => super! +- 40 => OK +- 60 => needs refactor + 80 => WTF! Friday, June 8, 12
27. PELIGRO Friday, June 8, 12
28. 1325.1: Reports::VenuesController #get_report_data venues_controller.rb:157 Friday, June 8, 12
29. del controlador al modelo Friday, June 8, 12
30. def index @cached_event = CachedEvent.find(params[:id]) end Friday, June 8, 12
31. 1 documento Friday, June 8, 12
32. no joins Friday, June 8, 12
33. módulos para agregar la data Friday, June 8, 12
34. TDD Friday, June 8, 12
35. pero primero Friday, June 8, 12
36. rm app/controllers/ reports_controller.rb Friday, June 8, 12
37. def test_replicates_an_event Event.create assert_equal 1, CachedEvent.count end Friday, June 8, 12
38. class Event < ActiveRecord::Base after_save :create_replicated_record def create_replicated_record CachedEvent.create(self.attributes) end end Friday, June 8, 12
39. REFACTOR Friday, June 8, 12
40. class Event < ActiveRecord::Base include MongoRelication::Event end Friday, June 8, 12
41. module MongoReplication module Event def self.included(base) base.class_eval do after_create :create_delayed_replication after_update :update_delayed_replication after_destroy :destroy_cached_event end end # define methods as usual def create_delayed_replicated ; .... ; end # .... end end Friday, June 8, 12
42. AR::Callbacks vs AR::Observers Friday, June 8, 12
43. 27.7: CachedEvent::replicate_all_accounts 25.2: ReportAgregator#guest_returns_count 17.4: Date::to_mongo 17.0: CachedEvent::find_or_create_by_event_id 15.7: ReportAgregator#file_name_export 13.3: CachedEvent::create_indices Friday, June 8, 12
44. 33.3: Reports::EventsController#index 14.3: Reports::EventsController#have_account_but_not_venue 13.2: Reports::EventsController#export_ticket_sales_group_by_staff 9.7: Reports::EventsController#show 9.1: Reports::EventsController#have_venue_selected Friday, June 8, 12
45. Report Aggregator class CachedEvent include ReportAggregator end Friday, June 8, 12
46. module ReportAggregator def returns_count ... end def total_reservations_sat ... end end Friday, June 8, 12
47. MIENTE class Event < ActiveRecord::Base include ReportAgregator end VS class EventFake include ReportAgregator end Friday, June 8, 12
48. Friday, June 8, 12
49. git grep "rescue nil" | wc -l h/t @mperham Friday, June 8, 12
50. module ApiError HttpExceptions = [ RestClient::Exception, SystemCallError, SocketError, EOFError, Timeout::Error, ] end Friday, June 8, 12
51. begin # esto puede explotar rescue *ApiError::HttpExceptions => e logger.error(e) end h/t @ped Friday, June 8, 12
52. Friday, June 8, 12
53. def index if (session[:account] != nil) && (session[:venue] == nil or session[:venue] == 0) # ... elsif (session[:venue] != nil) && (session[:venue] != 0) # ... else # ... end end Friday, June 8, 12
54. def index if have_account_but_not_venue # ... elsif have_venue_selected # ... else # ... end end Friday, June 8, 12
55. def have_account_but_not_venue (session[:account] != nil) && (session[:venue] == nil or session[:venue] == 0) end def have_venue_selected (session[:venue] != nil) && (session[:venue] != 0) end Friday, June 8, 12
56. Friday, June 8, 12
57. class MyView attr_reader :target def initialize(target) @target = target end def double case target when Student then target * 2 when Profesor then target.next when DictrictAdmin then target ** 2 else raise "don't know how to double #{target.class} #{target.inspect}" end end end Friday, June 8, 12
58. class MyView attr_reader :target def initialize(target) @target = target end def double target.double end end Friday, June 8, 12
59. class Student def double self * 2 end end class Profesor def double self.next end end Friday, June 8, 12
60. class DistrictAdmin def double self ** 2 end end class Object def double raise "...." end end h/t Sandi Metz Friday, June 8, 12
61. inyección de dependencias Friday, June 8, 12
62. geek speak for: - Pasalo como un parametro - dejame cambiarlo sin tocar tu clase Friday, June 8, 12
63. def party_like_an_animal Classroom.party_hard end Friday, June 8, 12
64. def party_like_an_animal(cr=Classroom) cr.party_hard end Friday, June 8, 12
65. party_like_an_animal party_like_an_animal(CrowdInteractive) party_like_an_animal(MagmaRails) Friday, June 8, 12
66. Friday, June 8, 12
67. /ruby/rails/activesupport/lib/ active_support/core_ext/ object_and_class.rb Friday, June 8, 12
68. class Object def blank? if respond_to? :empty? empty? elsif respond_to? :zero? zero? else !self end end end Friday, June 8, 12
69. class Object def blank? respond_to?(:empty?) ? empty? : !self end end class NilClass def blank? true end end class FalseClass def blank? true end end Friday, June 8, 12
70. class Array alias_method :blank?, :empty? end class Hash alias_method :blank?, :empty? end class String def blank? self !~ /[^[:space:]]/ end end Friday, June 8, 12
71. class Numeric #:nodoc: def blank? false end end Friday, June 8, 12
72. ¿Vale la pena tanto trabajo? Friday, June 8, 12
73. Friday, June 8, 12
74. ¿Dónde aprendo más de esto? Friday, June 8, 12
75. google: fast rails tests destroyallsoftware.com http://cleancoders.com/ Sandi Metz | Practical Object oriented Design in Ruby http://martinfowler.com/refactoring/ Friday, June 8, 12
76. Recapitulando Friday, June 8, 12
77. estudia la libreria standard Friday, June 8, 12
78. - Frameworks +Objetos Friday, June 8, 12
79. + Objetos + MENSAJES Friday, June 8, 12
80. knowledge++ Friday, June 8, 12
81. gracias @ivanacostarubio Friday, June 8, 12