Topic: refactoring long methods

Hi,
I have an accounting application with a DocumentsController (and almost everything in it). In accounting there are many various things you need to do with the present documents, different sorts, different listings, different calculations, and they don't really fit into the REST scheme, as they are all index-like. So my question is, what is the best way to refactor these methods? Should I create a new controller for all of them, or should each method get a new controller with only index action implemented, or should they just stay in the DocumentsController?

"Every program has (at least) two purposes: the one for which it was written, and another for which it wasn't."

Re: refactoring long methods

squil wrote:

Hi,
I have an accounting application with a DocumentsController (and almost everything in it). In accounting there are many various things you need to do with the present documents, different sorts, different listings, different calculations, and they don't really fit into the REST scheme, as they are all index-like. So my question is, what is the best way to refactor these methods? Should I create a new controller for all of them, or should each method get a new controller with only index action implemented, or should they just stay in the DocumentsController?

Generally each controller action should do two things:
a) get some records (and optionally do some simple stuff with them)
b) redirect to another action or render a view

Everything else can go in your models, as class methods or instance methods (which include rails associations etc).  With MVC your business logic goes in the models, so this should be where the actual accounting happens.  All the controller should do is decide what accounting to do, not actually do it, and decide what to do when the accouting has been completed.

If that helps at all...sorry to be vague but your question was pretty vague smile

###########################################
#If i've helped you then please recommend me at Working With Rails:
#http://www.workingwithrails.com/person/ … i-williams

Re: refactoring long methods

Thanks for the reply, I already lost hope for any advice smile

Basically I need to display some (or all) of the documents, and do some additions. I agree that this should be done in the model, although it probably will get messy. Let's say I need a view to display netto and vat values of some of the documents, then sum it up and display it so that it could be printed out. Let's call this a vat_view. My question is, should I leave it in the DocumentsController as a vat_view action, or should I create a SummariesController and have the vat_view action in there, or should I create a VatController and rename vat_view to index?

"Every program has (at least) two purposes: the one for which it was written, and another for which it wasn't."

Re: refactoring long methods

I would have a 'vat_view' action int the DocController:

  def vat_view
    @document = Document.find(params[:id])
  end

In the Document model I would add
def netto
    # Calculate the netto
end

def vat
  #calculate the VAT
end

def total
  vat + netto
end


and then in the vat_view.html.erb you just use @document.vat, @document.netto and @document.total. That way you keep all the logic inside your model, and your view and controller are nice and clean.

Re: refactoring long methods

This is way more complicated than what you wrote, satanir. I have a model called Client, which has_many :documents. Document has_many :vat_amounts, because we now have five vat rates, and they need to be configurable. So the VatAmount model also belongs_to :vat_rate.

So the @document.netto method needs to do self.vat_amounts.sum(:netto_amount), and similarly, @document.vat does self.vat_amounts.sum(:vat_amount). Then every document has an attribute called column_number, and I need to sum all of the netto amounts of a client's documents, but only those which have been issued during a specified month, and also only those which have specified column_number. There are about five such views, each of them a bit different. I now have it all in the DocumentsController, and I definitely need to refactor some things into the model, but I'm still not quite sure about those views.

"Every program has (at least) two purposes: the one for which it was written, and another for which it wasn't."

Re: refactoring long methods

phew....
OK, no so hard. First of all, this is pure business logic and should go to the controller.
then in the model:

def netto
  netto_amounts.sum(:netto_amount)
end

def vat
  vat_amounts.sum(vat_amount)
end

def self.total_by_month_and_column (client, month, column)
  @documents = find(:client_id => client, :issue_month => month, :column_number => column)
  @documents.sum(:netto_amount)
end


so basically I create a class method that retrieves all the documents based on what you described(client, month, column), and return the sum of their nettos.
All you need to do is call Document.total_by_month_and_column from wherever you want.

I'm not sure if this is exactly what you are looking for, but it can give you a pretty good idea on what to do.

[EDIT] The find would probably won't work. Just use :conditions. Another thing is that with this solution you have a single DB query - it fetches all the required docs at once.

Last edited by satanir (2008-07-08 11:10:54)

Re: refactoring long methods

Yeah, that's probably the best solution. The total_by_month_and_column method will probably need some working on, but it's a place to start. Anyway I guess I'll just leave all of the views in the DocumentsController, like you said. Thanks for the help smile

"Every program has (at least) two purposes: the one for which it was written, and another for which it wasn't."