Reputation: 328
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
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
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