Topic: How many lines? Why/when to refactor?

In general, when you are writing your code, what do you consider 'too many' lines for methods and classes? How many methods do you try to limit a class to? I've been bombarded with the standards for other languages, but Rails pulls so much out of the code that methods that would be 50 lines in PHP are now 5.

I am working (with someone else's code) where there are 25 methods in under 500 lines, but it seems like so much is going on. I'm hoping to refactor it, but I need some strong justification to do it.

What are your thoughts on line counts/refactoring in general, and tips for aproaching people more concerned with getting everything out as quick as we can and not too worried about DRYing it out?

Re: How many lines? Why/when to refactor?

In general, if a method gets over 10 lines I start getting the urge to refactor it (this is in Ruby, it's not so low in other languages for me). I'm probably too picky for my own good though. The average lines per method in one particular project of mine is 3-4 (rake stats).

I think more so than lines of code, I try to keep the number of local variables down. I find Ruby makes it really easy to move local variables into methods since you usually don't have to change much if you gave the variable a good name originally.

KittyKate wrote:

What are your thoughts on line counts/refactoring in general, and tips for aproaching people more concerned with getting everything out as quick as we can and not too worried about DRYing it out?

The Refactoring book gives a good argument to this IIRC. Think of the code as a bedroom, and Refactoring as cleaning up the room. Certainly you can leave everything a mess, but sooner or later it will become such a mess that you can't find anything or do anything. When it gets this messy it becomes harder to clean as well. The moral of the story? Refactor often. It is just general maintenance.

Railscasts - Free Ruby on Rails Screencasts

Re: How many lines? Why/when to refactor?

I know that my urge to refactor kicks in around 5 lines of body code (ie not counting the "def name" and "end" in model code.  For a while I was ok with up-to about 10 in controller, but as I've pushed more logic to the models I'm finding that five lines tend to me enough for the controllers too, but I'm still less strict there.

I tend to use line count total (from rake stats) to identify places where I'm systematically using too many lines -- typically it identifies places that a DSL is waiting to be discovered.  Which will lead to a large refactoring that's hard to see if your only worrying about a method at a time.  This is especially true if the code is already well factored..

My RoR journey  -- thoughts on learning RoR and lessons learned in applying TDD and agile practices.

Re: How many lines? Why/when to refactor?

We currently have about ten models that are all being treated to the same sort of instruction sets (new, edit, view, save, delete) with the methods (with names like new_modelA, edit_modelA, ..., new_modelB, edit_modelB) that are all in the same controller. Would you split them up?

I'm finding it's starting to become a pain to sift through it all to find the method I need (CTRL+F anyone?), and I'm afraid I'm missing redundant code because it's all crammed into one controller. I need to convince others that it's necessary, though (and I need something stronger than "this is getting butt ugly and stupid").

Oh, and we'll only be adding more models to the set.

Re: How many lines? Why/when to refactor?

I'd say you should move the model manipulation to multiple controllers.

vinnie - rails forum admin

Re: How many lines? Why/when to refactor?

KittyKate wrote:

We currently have about ten models that are all being treated to the same sort of instruction sets (new, edit, view, save, delete) with the methods (with names like new_modelA, edit_modelA, ..., new_modelB, edit_modelB) that are all in the same controller. Would you split them up?

Yep. However, if the site has already been deployed you'll have to make redirects on all of the old URLs. This can make it a little more difficult, but I still think it's worth splitting it up.

KittyKate wrote:

I need to convince others that it's necessary, though (and I need something stronger than "this is getting butt ugly and stupid").

To me, keeping the controllers small helps keep things simple, clean, and therefore more productive. If there's a 1 to 1 mapping between controller and model, and most of them are just basic CRUD operations, that just adds to the simplicity and it is easier to find things.

Whenever there's a model's name in a controller's action, that tells me there is a scope issue, and that it should go in its own group/controller. Of course there are exceptions though.

If there's no desire to keep the code clean and simple, then there's really no other argument that I can think of.

You may want to wait until Rails 1.2 (or Edge Rails now) and use Simply RESTful to make simple URLs. It sounds like you already have the CRUD operations in place, they just need to be moved into their own controller.

Railscasts - Free Ruby on Rails Screencasts