Rails controller variable mess

Question

I have a controller that I feel has too many instance variables.

The controller is pulling data from various places and it feels really sloppy. I have watched some Sandi Metz talks, read books, and other research, and I want to have good practice but I just don't know what to do here.

This method is pulling all the data and sending it to my view and I am able to get it to work, I just know this isn't a good way to go about it and I am hoping someone can point me to some code samples, documentation, videos, or help me understand how to implement a better style.

I have searched on SO and Google but I mostly find people saying to send a hash or JSON to the view, and I want to know if that is ideal before I start on that.

The Client, Project, Person, Role controllers and models have really similar code and I am working on refactoring it to be more DRY.

For example the Client, Project, Person, and Role financial controllers have almost the exact same controller index code as this. :(

I would be happy to add more code if that would help!

This is the project_financials_controller#index

It's pretty much taking in the data from the view and pulling a bunch of data from the database and sending it to a view. I'm currently using only the index method because it was only supposed to be a 'view' but now we can add filters such as time, different clients, etc so I think I need to break it out somehow.

I do have a financial_reports_nav model that this is calling that I could maybe use more, Or even make a financial_reports_controller that pulls the data from the appropriate model and I wont even need the 4 different controllers...

I am totally open to any input/criticism!

def index
  # CPPR = Client, Project, Person, Role
  @financial_type = 'project'
  @financial_params = params

  # This pulls the timeframe from the view and figures out the dates requested. (eg. "Last Week")
  @timeframe = Financial.time_frame(@financial_params[:timeframe], current_company.timezone, params[:start_date], params[:end_date])

  # This grabs all the data required to recall this financial report view at a later time
  @financial_nav = FinancialReportNav.set_financial_type(@current_user.id,@financial_type, @start_date, @end_date)

  # Grab all active and inactive people for client
  @people = Person.active.all
  @deleted_people = Person.inactive.all

  # This sends over all the info needed to generate the financial reports
  @project_financial_populate = Financial.new(@financial_params, @financial_type).populate_project_financials(current_company.default_hourly_cost, current_company.billing_rate, @timeframe[:start_date],@timeframe[:end_date])

  # This just pulls all the data from the database that the @project_financial_populate just populated (Can't we just use that??)
  @financial_rows = ProjectFinancial.all.map { |p| [ p.project_id, p.billable_hours, p.revenue,p.real_rate, p.hourly_expense, p.labor_expense_total, p.salary_expense,  p.gross_profit, p.profit_margin, p.missing_hourly_expense, p.missing_billable_rate ] }

  # Using the same view for CPPR's
  # Clients has an items count, so we just stuff everything into the first array slot
  @items = [1]

  # If these are not null then they show an option to change the financial filter type.
  @filter_by_client = Client.find_by('id = ?', @financial_params[:filter_by_client])
  @filter_by_project = Project.find_by('id = ?', @financial_params[:filter_by_project])
  @filter_by_person = Person.find_by('id = ?', @financial_params[:filter_by_person])
  @filter_by_role = PersonRole.find_by('id = ?', @financial_params[:filter_by_role])

  # This pulls a list of CPPR's that have tracked time in the requested timeframe
  @project_list = Financial.project_list(@timeframe[:start_date], @timeframe[:end_date])
  @client_list = Financial.client_list(@timeframe[:start_date], @timeframe[:end_date])
  @people_list = Financial.people_list(@timeframe[:start_date], @timeframe[:end_date])
end

Jay-Ar Polidario · Accepted Answer

I always tend to refactor code to be DRY whenever I noticed I have at least 3 instances of duplicate code, but I needed to future-proof the new code to be flexible enough for possible future changes; all of this considered however time permits.

Given your already current code and having told my preferences, this is what I would do:

Model Inheritance
Controller Inheritance
Shared template

Routes

config/routes.rb

resources :client_financial
resources :project_financial
resources :person_financial
resources :role_financial

Models

app/models/financial_record.rb

class FinancialRecord < ActiveRecord::Base # or ApplicationRecord if > Rails 5
  self.abstract_class = true

  # your shared "financials" model logic here
end

app/models/client_financial.rb

class ClientFinancial < FinancialRecord
  # override "financials" methods here if necessary
  # or, add new model specific methods / implementation
end

app/models/project_financial.rb

class ProjectFinancial < FinancialRecord
  # override "financials" methods here if necessary
  # or, add new model specific methods / implementation
end

app/models/person_financial.rb

class PersonFinancial < FinancialRecord
  # override "financials" methods here if necessary
  # or, add new model specific methods / implementation
end

app/models/role_financial.rb

class RoleFinancial < FinancialRecord
  # override "financials" methods here if necessary
  # or, add new model specific methods / implementation
end

Controllers

app/controllers/financial_controller.rb

class FinancialController < ApplicationController
  before_action :set_instance_variables, only: :index

  protected

  def set_instance_variables
    # strips the last "Controller" substring and change to underscore: i.e. ProjectFinancialsController becomes project_financials
    @financial_type = controller_name[0..(-'Controller'.length - 1)].underscore

    # get the corresponding Model class
    model = @financial_type.camelcase.constantize
    # get the correspond Financial Model class
    financial_model = "#{@financial_type.camelcase}Financial".constantize

    @financial_params = params

    @timeframe = Financial.time_frame(@financial_params[:timeframe], current_company.timezone, params[:start_date], params[:end_date])

    # I dont know where you set @start_date and @end_date
    @financial_nav = FinancialReportNav.set_financial_type(@current_user.id,@financial_type, @start_date, @end_date)

    # renamed (or you can set this instance variable name dynamically)
    @records = model.active.all
    # renamed (or you can set this instance variable name dynamically)
    @deleted_records = model.inactive.all

    @financial_populate = Financial.new(@financial_params, @financial_type).populate_project_financials(current_company.default_hourly_cost, current_company.billing_rate, @timeframe[:start_date],@timeframe[:end_date])

    @financial_rows = financial_model.all.map { |p| [ p.project_id, p.billable_hours, p.revenue,p.real_rate, p.hourly_expense, p.labor_expense_total, p.salary_expense,  p.gross_profit, p.profit_margin, p.missing_hourly_expense, p.missing_billable_rate ] }

    @items = [1]

    @filter_by_client = Client.find_by('id = ?', @financial_params[:filter_by_client])
    @filter_by_project = Project.find_by('id = ?', @financial_params[:filter_by_project])
    @filter_by_person = Person.find_by('id = ?', @financial_params[:filter_by_person])
    @filter_by_role = PersonRole.find_by('id = ?', @financial_params[:filter_by_role])

    @project_list = Financial.project_list(@timeframe[:start_date], @timeframe[:end_date])
    @client_list = Financial.client_list(@timeframe[:start_date], @timeframe[:end_date])
    @people_list = Financial.people_list(@timeframe[:start_date], @timeframe[:end_date])
  end
end

app/controllers/client_financials_controller.rb

class ClientFinancialsController < FinancialController
  def index
    render template: 'financials/index'
  end
end

app/controllers/project_financials_controller.rb

class ProjectFinancialsController < FinancialController
  def index
    render template: 'financials/index'
  end
end

app/controllers/person_financials_controller.rb

class ProjectFinancialsController < FinancialController
  def index
    render template: 'financials/index'
  end
end

app/controllers/role_financials_controller.rb

class ProjectFinancialsController < FinancialController
  def index
    render template: 'financials/index'
  end
end

Views

app/views/financials/index.html.erb

P.S. This is just a simple refactor. Without knowing the fuller scope of the project, and future plans, I'll just do this one. Having said this, I would consider using "polymorpic" associations, and then just have one routes endpoint (i.e. resources :financials) and then just pass in a params filter like: params[:financial_type] which directly already map the financial_type polymorphic column name.