1

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
Improve this question

1 Answer 1

Reset to default
2

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:

  1. Model Inheritance
  2. Controller Inheritance
  3. 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

<!-- YOUR SHARED "FINANCIALS" INDEX HTML HERE -->

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.

Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

This is great! I haven't done any abstraction like this before so this definitely helps. I'm currently sharing some of the methods in a service object but this feels cleaner to me. Thank you!
@thyrootest no prob! :)

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.