How do you refactor a class that is constantly being edited?

Question

Over the course of time, my team has created a central class that handles an agglomeration of responsibilities and runs to over 8,000 lines, all of it hand-written, not auto-generated.

The mandate has come down. We need to refactor the monster class. The biggest part of the plan is to define categories of functionality into their own classes with a has-a relationship with the monster class.

That means that a lot of references that currently read like this:

var monster = new orMonster();
var timeToOpen = monster.OpeningTime.Subtract(DateTime.Now);

will soon read like this:

var monster = new Monster();
var timeToOpen = monster.TimeKeeper.OpeningTime.Subtract(DateTime.Now);

The question is: How on Earth do we coordinate such a change? References to "orMonster" litter every single business class. Some methods are called in literally thousands of places in the code. It's guaranteed that, any time we make such a chance, someone else (probably multiple someone elses) on the team will have code checked out that calls the .OpeningTime property

How do you coordinate such a large scale change without productivity grinding to a halt?

Answer 1

One thing you can do is to temporarily leave proxy methods in the monster class that will delegate to the new method. After a week or so, once you are sure all code is using the new method, then you can safely remove the proxy.

Answer 2

You should make the old method call the new method. Then over time change the references to the old method to call the new method instead. Once all the client references are changed, you can delete the old method.

For more information, see Move Method in Martin Fowler's classic, Refactoring.

Answer 3

Don't refactor it.

Start over and follow the law of demeter. Create a second monster class and start from scratch. When the second monster class is finished and working, then replace occurrences of the first monster. Swap it out. Hopefully they share an interface, or you can make that happen.

And instead of this: "monster.TimeKeeper.OpeningTime.Subtract(DateTime.Now)"

Do this: monster.SubtractOpeningTime(DateTime.Now). Don't kill yourself with dot-notation (hence the demeter)

Answer 4

Suggest using a tool such as nDepend to identify all of the references to the class methods. The output from nDepend can be used to give you a better idea about how to group the methods.

Answer 5

I've handled this before by going ahead and refactoring the code, but then adding methods that match the old signature that forward the calls to the new method. If you add the "Obsolete" attribute to these temporary methods, your code will still build with both the old method calls and the new method calls. Then over time you can go back through and upgrade the code that is calling the old method. The difference here is that you'll get "Warnings" during the build to help you find all of the code that needs upgrading.

Answer 6

I'm not sure what language you're using but in .Net you can create compiler warnings which will allow you to leave the old references for a time so that they will function as expected but place a warning for your other developers to see.

http://dotnettipoftheday.org/tips/ObsoleteAttribute.aspx

Answer 7

Keep the old method in place and forward to the new method (as others have said) but also send a log message in the forwarding method to remind yourself to remove it.

You could just add a comment but that's too easy to miss.

Answer 8

Several people have provided good answers regarding the orchestration of the refactor itself. That's key. But you also asked about coordinating the changes between multiple people (which I think was the crux of your question). What source control are you using? Anything like CVS, SVN, etc can handle incoming changes from multiple developers at once. The key to making it go smoothly is that each person must make their commits granular and atomic, and each developer should pull other people's commits often.

Answer 9

var monster = new Monster();
var timeToOpen = monster.TimeKeeper.OpeningTime.Subtract(DateTime.Now);

I'm not sure that divvying it up and just making portions of it publically available is any better. That's violating the law of demeter and can lead to NullReference pain.

I'd suggest exposing timekeeper to people without involving the monster.

If anything you'd be well off analysing the API and seeing what you can cut and encapsulate within monster. Certainly giving monster toys to play with as opposed to making monster do all of the work itself is a good call. The main effort is defining the toys monster needs to simplify his work.

Answer 10

Develop your changes in a branch. Break out a subset of code to a new class, make changes across the client base, test thoroughly, and then merge back.

That concentrates the breakage to when you merge — not the entire development cycle.

Combine this with Patrick's suggestion to have the monster call the small monsters. That'll let you easily revert if your merged client code breaks changes to that client. As Patrick says, you'll be able to remove the monster's methods (now stubs) once you prove nobody's using it.

I also echo several posters' advice to expose the broken out classes directly — not via the monster. Why apply only half a cure? With the same effort, you could apply a complete cure.

Finally: write unit tests. Write lots of unit tests. Oh, boy, do you need unit tests to safely pull this one off. Did I mention you need unit tests?

Answer 11

Such a huge class is really an issue. Since it grew so big and nobody felt uncomfortable, there must be something wrong with project policies. I'd say you should split into pairs and do pair programming. Create a branch for every pair of programmers. Work for 1-2 days on refactoring. Compare your results. This will help you avoid the situation when the refactoring would go from the start into the wrong direction and finally that would lead to the need of rewriting the monster class from scratch.

Answer 12

I will look at first using partial class to split the single monster class over many files, grouping methods into categories.

You will need to stop anyone editing the monster class while you split in into the files.

From then on you are likely to get less merge conflicts as there will be less edits to each file. You can then change each method in the monster class, (one method per checkin) to call your new classes.

Answer 13

I don't think leaving a mess and planning on cleaning up a week later is a good idea. Always leave the code cleaner than when you found it.

First: Disable exclusive checkout in your source sontrol system. Then: Change one thing at a time, make it clean, and check in the change.

If anyone else on the team gets a conflict they will have to deal with it. Provided they check in early and often it won't cause them any pain.