cronaca di un refactoring

gistfile1.rb

# Giovedì e venerdì io e il Racco ci siamo messi a sistemare le spec rotte di miumiu, siamo subito giunti alla conclusione che urgeva un
# refactoring serio di alcune parti basilari del codice dell'estore. Quanto segue è la cronaca di questo refactoring.
# 
# 1) la classe presentava pochi metodi, sembrava poco codice, ma era decisamente denso e oscuro, faceva troppe cose: si
# trattava fondamentalmente un grumo di merda che settava una pletora di variabili di istanza.
# Il codice era speccato, ma le spec erano  indecenti e quasi tutte fallivano poiché erano
#  disallienate col codice corrente. Fixarle sarebbe costato molta fatica visto che:
# - i blocchi before lunghi e decisamente oscuri
# - il codice dei test era lungo e decisamente oscuro
# Le spec probabilmente non coprivano un numero sufficiente di casistiche, questo era dovuto principalmente al fatto che pochi metodi 
# facevano tantissime cose, il loro nome era insufficiente a descrivere la loro responsabilità, in pratica il codice era molto difficile
# da manutenere soprattutto per chi non l'avesse scritto.

class Estore::EstoreController < ApplicationController
  COUNTRY_CODES = NATION_CODES['countryCodes']
  DEFAULT_DEPARTMENT = 'new-arrivals'

  before_filter :set_country_code
  before_filter :init_marketplace
  before_filter :load_countries
  before_filter :fetch_departments
  before_filter :fetch_categories

protected
  def load_countries
    @available_countries = COUNTRY_CODES.map do |key, value|
      { :code => value, :name => I18n.t("countries.#{key.downcase}") }
    end
    @available_countries.sort! { |a, b| a[:name] <=> b[:name] }
  end

  def set_country_code
    if params[:countryCode]
      fixed_locale = LocaleManager.correct_locale(I18n.locale, params[:countryCode])
      if fixed_locale != I18n.locale
        redirect_to params.merge(:controller => controller_name, :action => params[:action], :locale => fixed_locale)
        return false
      end
      @countryCode = params[:countryCode]
    else
      @countryCode = 'IT'
    end
  end

  def init_marketplace
    if code = NATION_CODES['marketplaces'][country_code]
      @marketplace = Marketplace.first :conditions => { :value => code }
      unless @marketplace.catalogue_only
        @documents = DOCUMENTS[@marketplace.value].try(:symbolize_keys)
      end
      @documents ||= {}
    end
    @marketplace
  end

  def fetch_departments
    @departments = Department.visibles_in_marketplace @marketplace
    if params[:department]
      dep_id = params[:department]
      #in the db, departments are, usually, 5/6 records
      @selected_department = @departments.select { |d| d.value == dep_id }.first
    end
    @selected_department ||= @departments.find_by_value(DEFAULT_DEPARTMENT) || @departments.first # fallback in case DEFAULT_DEPARTMENT is not available
    @grid_config = GridViewConfig.get @selected_department.value
  end

  def fetch_categories
    @categories = @selected_department.main_categories.visibles_in_marketplace @marketplace
    if params[:category]
      cat_id = params[:category]
      #in the db, categories are, usually, 8/9 records
      @selected_category = @categories.select { |c| c.handle == cat_id }.first
    end
    @selected_category ||= @grid_config.view_all && nil or @categories.first #nil => view all
    if @selected_category
      @grid_config = GridViewConfig.get @selected_department.value, @selected_category.handle
    end
  end
end


# 2) I pochi metodi presenti prima sono stati separati in vari metodi con funzionalità atomiche. A questo punto è stato possibile
# riscrivere le spec in modo atomico e testare in modo preciso il funzionamento di ciascun metodo.
# Notate che ci sono innumerevoli metodi privati e che tali devono restare visto che siamo in un controller: non vogliamo assolutamente che
# siano chiamabili dal browser. Per testarli è quindi necessario rivolgersi a send, primo code smell che ci fa pensare di essere ancora
# lontani dal refactoring definitivo. A questo aggiungiamo che tutti questi metodi non hanno niente a che fare con un controller, 
# guardandoli appare abbastanza evidente che si potrebbe estrarli in una classe a parte: questo ci permetterebbe di speccarli in modo
# corretto senza passare per send. La sensazione di disagio è ulteriormente rafforzata dal fatto che in alcuni casi solleviamo delle
# eccezioni (ActiveRecord::RecordNotFound) che non hanno nulla a che fare col normale ciclo di funzionamento dei controller.
# Ultima considerazione, si noti la proliferazione di helper_method, altro code smell.


class Estore::EstoreController < ApplicationController
  DEFAULT_DEPARTMENT = 'new-arrivals'

  before_filter :redirect_to_home_on_wrong_locale

  before_filter :set_selected_department
  before_filter :set_selected_category

  helper_method :selected_department
  helper_method :selected_category
  helper_method :visible_categories
  helper_method :grid_config
  helper_method :country_code
  helper_method :marketplace
  helper_method :visible_departments

private

  def country_code
    @countryCode ||= params[:countryCode]
  end

  def marketplace
    @marketplace ||= begin
      code = NATION_CODES['marketplaces'][country_code]
      Marketplace.find_by_value! code
    end
  end

  def visible_departments
    @departments ||= Department.visibles_in_marketplace marketplace
  end

  def visible_categories
    @categories ||= selected_department.main_categories.visibles_in_marketplace marketplace
  end

  def grid_config
    @grid_config ||= begin
      if selected_category
        GridViewConfig.get selected_department.value, selected_category.value
      else
        GridViewConfig.get selected_department.value
      end
    end
  end

  def selected_department
    @selected_department
  end

  def selected_category
    @selected_category
  end

  def set_selected_department
    if params[:department]
      @selected_department = visible_departments.detect { |d| d.value == params[:department] }
      raise ActiveRecord::RecordNotFound unless @selected_department
    else
      # in case DEFAULT_DEPARTMENT is not available we'll use the first available
      @selected_department = visible_departments.detect { |d| d.value == DEFAULT_DEPARTMENT } || visible_departments.first
    end
    @selected_department
  end

  def set_selected_category
    if params[:category]
      @selected_category = visible_categories.detect { |c| c.handle == params[:category] }
      raise ActiveRecord::RecordNotFound unless @selected_category
    else
      # in case department doesn't support view all we use the first available category
      @selected_category = grid_config.view_all ? nil : visible_categories.first
    end
    @selected_category
  end

  def redirect_to_home_on_wrong_locale
    unless LocaleManager.correct_locale? country_code
      redirect_to params.merge :locale => LocaleManager.correct_locale(country_code)
      return false
    end
  end
end


# 3) I metodi privati sono quasi spariti, finiti nella nuova classe Estore. E' rimasto 1 solo metodo privato, chiamato dall'unico
# before_filter. La classe Estore è ora completamente speccata a parte, in ogni sua sfaccettatura. Qualora le specifiche dovessero cambiare
# sarà molto più semplice aggiornare il codice e le spec. Resta ancora parecchio lavoro da fare, ma si iniziano a vedere i frutti del
# refactoring: la classe è pulita e senza responsabilità che non le competono. Restano da modificare le viste e i controller che ereditano da
# questo, dove le chiamate agli helper method vanno sostituiti con chiamate dirette verso l'oggetto Estore memoizzato nel metodo estore. Fatto
# questo si potranno eliminare tutti questi helper method (ne resterà solo 1, ora commentato) ed eliminare il delegate di questi metodi a
# estore.

class Estore::EstoreController < ApplicationController
  before_filter :redirect_to_home_on_wrong_locale

  # these lines should be replaced by only 1 single
  # helper method definition:
  # helper_method :estore
  # then in views and controller the removed helper
  # methods should be dispatched to the estore object
  # rather than to the controller
  # at that point we could also remove the delegation
  # at line 26
  helper_method :country_code
  helper_method :marketplace
  helper_method :selected_department
  helper_method :selected_category
  helper_method :visible_departments
  helper_method :visible_categories
  helper_method :grid_config

private

  def estore
    @estore ||= Estore.new(params)
  end

  delegate :selected_category, :selected_department, :visible_departments, :visible_categories, :grid_config, :country_code, :marketplace, :to => :estore

  def redirect_to_home_on_wrong_locale
    unless LocaleManager.correct_locale? country_code
      redirect_to params.merge :locale => LocaleManager.correct_locale(country_code)
      return false
    end
  end
end


# 4) Il codice client di questa classe è stato aggiornato, è ora possibile eliminare quanto non serve più. Per ora il lavoro di 
# rifattorizzazione è finito, lasciandoci con una classe decisamente più comprensibile e manutenibile rispetto a quella precedente.

class Estore::EstoreController < ApplicationController
  before_filter :redirect_to_home_on_wrong_locale

  helper_method :estore

private

  def estore
    @estore ||= Estore.new(params)
  end

  def redirect_to_home_on_wrong_locale
    unless LocaleManager.correct_locale? country_code
      redirect_to params.merge :locale => LocaleManager.correct_locale(country_code)
      return false
    end
  end
end


# qui di seguito la classe Estore estratta dal controller. Non è ancora perfetta, ma dovremmo quasi esserci.

class Estore
  DEFAULT_DEPARTMENT = 'new-arrivals'

  class Estore::CategoryNotFound < ActiveRecord::RecordNotFound; end
  class Estore::DepartmentNotFound < ActiveRecord::RecordNotFound; end
  class Estore::MarketplaceNotFound < ActiveRecord::RecordNotFound; end

  def initialize(params)
    # all required instance variables will be autodefined when needed
    # so there is no need to initialize any of the variables required
    # by the app to work properly
    @params = params
  end

  def country_code
    @countryCode ||= @params[:countryCode]
  end

  attr_writer :country_code # this is because of line 107 in checkout_controller, can we remove it?

  def marketplace
    @marketplace ||= begin
      value = NATION_CODES['marketplaces'][country_code]
      Marketplace.find_by_value(value) or raise MarketplaceNotFound
    end
  end

  def grid_config
    @grid_config ||= GridViewConfig.get selected_department.value, selected_category.try(:value)
  end

  def visible_departments
    @departments ||= Department.visibles_in_marketplace marketplace
  end

  def visible_categories
    @categories ||= selected_department.main_categories.visibles_in_marketplace marketplace
  end

  def selected_department
    @selected_department ||= set_selected_department
  end

  def selected_category
    @selected_category ||= set_selected_category
  end

private

  def set_selected_department
    @selected_department ||= begin
      if @params[:department]
        visible_departments.detect { |d| d.value == @params[:department] } or raise DepartmentNotFound
      else
        # in case DEFAULT_DEPARTMENT is not available we'll use the first available
        visible_departments.detect { |d| d.value == DEFAULT_DEPARTMENT } or visible_departments.first or raise DepartmentNotFound
      end
    end
  end

  def set_selected_category
    @selected_category ||= begin
      if @params[:category]
        visible_categories.detect { |c| c.handle == @params[:category] } or raise CategoryNotFound
      else
        # in case department doesn't support view all we use the first available category
        selected_department.view_all? ? nil : (visible_categories.first or raise CategoryNotFound)
      end
    end
  end
end

well done, guys!