views:

586

answers:

11

A good rule of thumb by is that I intelligently refactor any method over 50 lines.

The count does not include comments and white space but actual code. The reason I also say intelligently is there are plenty of times where a class over 50 lines is acceptable and cannot or should not be changed.

I do not have a rule of thumb for classes. Generally I don't check classes to see if they should be refactored.

On my currently project I have just about complete a class that is almost 4000 lines long. However no methods over 50, and most of the lines and methods are private and not acting on any data outside of the class.

What is the rule of thumb for refactoring classes?

+1  A: 

Refactoring is not so much about reducing the number of LOC, though that is a side effect, but improving the quality. Particularly, you want to try to follow the DRY (Don't Repeat Yourself) principle as much as you can, reasonably.

BobbyShaftoe
+19  A: 

When the class violates the SRP, it's time to refactor.

Nebakanezer
This class doesn't violate that principal. So number of lines of code become meaningless. Interesting.
David Basarab
I love SRP. Nice comment.
Ed Altorfer
Nice answer. ;)
DaDa
Longhorn213: Thats right. If it truly does not violate SRP, then breaking it up into a separate class is counter-productive. You can, however, refactor the class itself into smaller methods.
Brian Genisio
I think, just looking at SRP, is not allways enough. It's hard to find a good example, but there are cases, where a class has only a single responsibility, but still shows internal structure that could be extracted. So I would say, you should refactor if you can increase the entropy of the code.
Daniel Brückner
@Daniel - Surely you mean *decrease* the entropy (i.e. put it into a more ordered state)?
Greg Beech
No, increase is correct - it only sounds wrong. Thinking about it as increasing the information of the code might be more natural. Extracting (repetitive) structure from a class will only leave the unstructures parts in and those increase the entropy of the class. So, good programming style means creating chaos ... :D
Daniel Brückner
If you have a class with hundreds of lines, but can find only a single responsibility from it, then you are not looking hard enough. ;) In my current project (http://dimdwarf.sourceforge.net/) the core module has 156 Java classes; 75% of them are less than 50 lines long (including empty lines), 10% of them are over 100 lines long, and the longest file is 194 lines long (and it has about 3 responsibilities so I'm going to split it).
Esko Luontola
The problem with SRP is, that it depends on the level of abstraction - one could argue that the responsibility of Windows is to provide services to simplfy interaction with the machine, hence should be a single class...
Daniel Brückner
+4  A: 

I wouldn't say that there's any "rule of thumb" for refactoring large classes. Sometimes a class really encapsulates a lot of business logic and should be as large as it is. However, you might consider asking yourself a few questions:

  1. (Assuming you're writing object oriented code) Is my code truly object-oriented? That is, does it follow the Single Responsibility Principle (thanks, Nebakanezer)? Is this class a helper class? If so, how could I refactor its methods into more appropriate object classes?

  2. Do I have a solid architecture? That is, am I making use of abstraction and inheritance rather than re-inventing the wheel in every class? Are overloaded methods calling base methods as appropriate?

  3. Do I really need all this code? Could some of the logic be externalized using xpath, lambda expressions, or some form of database driven expressions?

  4. Is the code extensible? Is it easy to maintain? Was it well-architected from the beginning, or are we always making small patches to try to fix problems?

I hope that this helps a little; it can be difficult to refactor large classes, but I think if you start looking through my questions, you may find pretty quickly that there is room for improvement in your code...I know I typically do.

Especially look at #1--it's really common for people to create a ton of helper classes all over the place, which is very anti-object oriented (in my opinion). That's a different topic, but you may want to see what really -belongs- in the class you've made and what could/should be somewhere else.

As a rule of thumb, if the class is maintainable and flexible, it may not need to be changed. :)

Ed Altorfer
+1  A: 

I would like to share this article with you

Refactoring in the Large

DaDa
+1  A: 

In C#, my rule of thumb for classes is anything over 500 lines is getting too long. I really like methods under 10 lines, and I guess under 20 is acceptable. I think it really depends on the context.

David Hodgson
+1  A: 

My rule of thumb is for methods, more than classes - If I can't see the whole method on screen it needs to be refactored. Of course, the traditional smells apply.

Gary.Ray
+2  A: 

Refactor whenever you have the opportunity to:

  • introduce the appropriate design patterns in lieu of spaghetti code
  • reduce code complexity and increase readability
  • remove redundant code by introducing a single method call
  • extricate UI from business logic

etc, etc.

EDIT: Obviously, the decision to refactor should take into account time, potential pay-off, other responsibilities, etc.

Garrett
+3  A: 

Don't let LOC be your primary metric. 50 lines seems really small to me. With 50 line files, you will end up having an unfriendly number of class files in the solution. Your productivity will be dampened by all the navigation you will be doing between files and your IDE will always be littered with too many tabs. I personally try to organize classes into logical groups by namespace first. On a class by class basis, I try to make the code smaller and easier to read. Sometimes, class files do get large. I start to get a sick feeling when the class file is 2000+ lines. Anything less than that, I deal with on a case by case basis.

Steve
I love your word choice--a sick feeling. Haha. I get the same feeling whenever I see people implementing their own sorting algorithm just to sort an array of numbers. :)
Ed Altorfer
LOC isn't the main point (+1) but with a good IDE, you should never be forced to know where code actually is to. You should rarely navigate by filename/line-number or even "above blaa in file whatever"
BCS
+1  A: 

I tend to look at the cyclomatic complexity of each member instead of number of lines of code for the entire class.

James
A: 

This class could be a candidate for the refactoring exercise of "extract an object that you don't think is there"?

Perhaps, for example, you could extract a method object that would allow you to refactor several of the 50 line methods into members of an extracted class?

Alternatively, encapsulating some of those private methods that contain business logic in a new object that collaborates with this one might provide a new way to reuse either this object, or the extracted one.

One line is enough to consider refactoring. 40 each 50 line methods is starting to scream "I'm too complicated, there's another object hiding in here" to my ear.

Tetsujin no Oni
+2  A: 

If they have broken one of following ... it's time to refactor. Hey, guy you are looking for SOLID ... screencasts are here

  • SRP: Single Responsibility Principle, THERE SHOULD NEVER BE MORE THAN ONE REASON FOR A CLASS TO CHANGE

  • OCP: Open Closed Principle, SOFTWARE ENTITIES (CLASSES, MODULES, FUNCTIONS, ETC.) SHOULD BE OPEN FOR EXTENSION BUT CLOSED FOR MODIFICATION

  • LSP: Liskov Substitution Principle, FUNCTIONS THAT USE ... REFERENCES TO BASE CLASSES MUST BE ABLE TO USE OBJECTS OF DERIVED CLASSES WITHOUT KNOWING IT.

  • ISP: Interface Segregation Principle, CLIENTS SHOULD NOT BE FORCED TO DEPEND UPON INTERFACES THAT THEY DO NOT USE

  • DIP: Dependency Inversion Principle,

    A. HIGH LEVEL MODULES SHOULD NOT DEPEND UPON LOW LEVEL MODULES. BOTH SHOULD DEPEND UPON ABSTRACTIONS

    B. ABSTRACTIONS SHOULD NOT DEPEND UPON DETAILS. DETAILS SHOULD DEPEND UPON ABSTRACTIONS

And it's all, have fun :)

ruslander
I was going to suggest SOLID as a baseline myself, but you beat me to it. ;)
Tracker1