CODEWITHSUNDEEP
ruby-on-rails-3refactoringdelegation
David R
David R

Reputation: 328

Delegate or instantiate additional class?

Let's say I have an Account class and an AccountReport class. In accounts#show I want to show a report of an account. Both Account and AccountReport have a number of public methods. Which technique of the following techniques is better?

1) Instantiate an Account and an AccountReport, initializing the AccountReport with the account data.

class AccountsController < ActionController::Base
  def show
    @account = current_user.account
    @account_report = AccountReport.new(@account.orders)

    respond_with(@account)
  end

  # ...
end

2) Allow an instance of Account to instantiate AccountReport and delegate method calls

class Account < ActiveRecord::Base
  attr_reader :account_report

  delegate :method_a, :method_b, :method_c, :method_d, :to => :account_report

  after_initialize :setup_account_report

  def setup_account_report
    @account_report = AccountReport.new(orders)
  end

  # ...
end

Option 2 seems to be a cleaner approach to me but loading up Account with lots of methods makes it feel like a God class.

Upvotes: 1

Views: 135

Answers (2)

Joel McCracken
Joel McCracken

Reputation: 2335

I like the first option because it keeps coupling low. The second option ties Account and AccountReport together in a way that is probably unnecessary. What happens whenever you get another type of report? You'll probably need to change a bunch of things in Account, which is sad because they are seemingly unrelated.

You can keep the logic / verbosity low in the controller by combining these two things in a service object, and handing that off to your views. An AccountReporting service can handle the logic behind combining these two classes together, e.g.: 

class AccountReporting
    def initialize(account)
       @account = account
    end
    def report
       AccountReport.new(account.orders)
    end
end

Then, to use it in the controller:

AccountReporting.new(current_user.account)

Does this make sense?

Upvotes: 0

Code-Source
Code-Source

Reputation: 2243

Well, i think you have to make a mix of both option.

The first one is good, if you only use reports on show. The second one is good, if you use all the time reports for your account.

With the second one, all the time your report will be instantiate and it could reduce performances.

You should perhaps try something like this:

class Account < ActiveRecord::Base
  # ...

  @report = nil
  def report
    if @report.nil?
       @report = AccountReport.new(self.orders)
    end
  end

  # ...
end

The good thing of this solution is that report is loaded only if needed, but will not be loaded every time. The bad thing of this solution is that if you add some orders your report will not be up to date.

UPDATE: To improve this, you could replace the condition with this one

if @report.nil || self.created_at_changed?

Upvotes: 2

Related Questions