When to refactor code

http://mikemainguy.blogspot.com/2013/05/when-to-refactor-code.html

by mainguy • 13 years ago

19 17 13 years ago

mikemainguy.blogspot.com

Always. The development cycle should be Test, Code, Refactor, as one atomic block of work. Sarah Mei explains why this is vitally important to creating good code in her talk "Work, Play & Code", I highly recommend checking it out.

http://confreaks.com/videos/2333-mwrc2013-work-play-code

Also, if you don't have a test suite to prove your refactoring didn't change functionality, IMO you aren't refactoring, you are simply rewriting. This is the point many people miss about TDD. TDD is first and foremost about letting you refactor your code; doing TDD right gives you the best environment for writing great code. On the other hand, doing TDD wrong makes it more likely your entire code base will be very difficult to maintain.

InclinedPlane2• 13 years ago

One advantage of TDD is that encourages your code to be more easily testable. Which tends to encourage you to develop properly modular code. Code that can only be tested indirectly through integration testing (spinning up huge unwieldy hunks of context that are required for the individual sub-component to work properly) is not very modular.

Modular code tends to have fewer dependencies and tends to produce fewer side effects. All of these things make for code which is generally easier to maintain and modify, easier to understand, easier to debug, and easier to reuse. All of which tend to lead to higher code quality naturally.

A high level of unit test coverage also gives you other benefits, of course, but I thought I'd highlight these side benefits because they are perhaps the most important.

drharris• 13 years ago

This, exactly. You should always be looking to improve code. If you have a truly comprehensive test suite, there is no danger in always improving code. It's easier to make refactoring continuous than it is to figure out a checklist of when to do it. The answer is "always".

h2s• 13 years ago

    > "this 10 item checklist."
    > 11 items in the checklist

Heh, it's not often you see an off-by-one error in prose!

cs648• 13 years ago

I guess it has to do this item 2: "Did you completely understand what cyclomatic complexity was without following that link?", I don't really see how this fits into the rest of the checklist.

Hansi2• 13 years ago

I have a 1134 line function with zero tests defined for it sitting open in the background which most likely scores something like 32 out of 11 on that list. I'm afraid of touching that monster...

steveklabnik• 13 years ago

I use 'break out method object' from "Working Effectively with Legacy Code" in these situations:

    1. Create a new class for the computation
    2. Define a method on that class to do the new work.
    3. Copy the body of the old method over, and change variable references to instance variables.
    4. Give it an initialize method that takes arguments to set the instance variables used in step 3.
    5. Make the old method delegate to the new class and method.

Now, at least, it's isolated into once place, and you can use your other refactoring tricks without affecting global state, like extract method.

I'm not sure if this is the _best_ example, but I wrote about this a few years ago, in Ruby: http://blog.steveklabnik.com/posts/2011-09-06-the-secret-to-...

tetha• 13 years ago

If you touch it, it will never work again. Even if you revert all your changes.

precisioncoder• 13 years ago

I've had this happen often enough that when refactoring something tricky I like to copy the function and rework it. Then I'll change references from the old method to the new one one at a time and test. One of my favorite methods returned a Map<Long, Map<Long, Map<Long, Map<Long, Long>>>> with no comment explaining what any of those Long values were...

Hansi2• 13 years ago

My thoughts exactly. Needs to be done though unfortunately. I'm going to get some chainsaws and duct tape and attack it on Monday. Mondays suck anyway so it can't make the day much worse.

MostAwesomeDude• 13 years ago

This seems like a checklist for when to factor, not when to refactor; he appears to be describing massive mutant code that has never actually been split into manageable chunks.

loup-vaillant3• 13 years ago

Such Chtuloid horrors do exist. I have seen some. (I could tell more, but I care for the sanity of my readers.)

columbo• 13 years ago

> Did you completely understand what cyclomatic complexity was without following that link?

Seems confusing that answering "No" to this (meaning you did NOT know what cyclomatic complexity was) would be a net positive for refactoring?

loup-vaillant3• 13 years ago

Probably has something to do with this: https://news.ycombinator.com/item?id=5718455

InclinedPlane2• 13 years ago

100 lines, hah....

Anyway, I don't think you need a big, complicated checklist like this. Writing code by checklist is not going to solve your problems. The more important questions are these:

- Do you find yourself spending more time than you think you should making changes and additions (or bug fixes) to the same sections of code?

- Do you often run into difficulty figuring out or remembering what a given section of code does?

If either of those things are true then refactoring might be a good idea. Use extract method until it hurts. Use extract method on complicated logic tests as well. Figure out where the control flow is wonky and switch to using flow based on method calls and returns to simplify things. Be ruthless. Don't be lulled into the "meaty method" trap, don't be afraid of short methods, even if they're only one line. Make your code so that it looks almost too simple, as though even a novice could look at it and tell you what it does. Push until you get to a state where making changes seems almost like cheating because it's so easy. That's when you know you're in the sweet spot. Programming is about making things easier, even future programming, not about proving how incredibly skilled you are at maintaining complex code.

loup-vaillant3• 13 years ago

Aand, while where at it, minimize the side effects. Avoid methods that modify objects, replace them by equivalent methods that create new objects. Likewise, expunge init() methods. Their code belong to constructors, factories, or curried functions. Basically, avoid the assignment statement: http://loup-vaillant.fr/tutorials/avoid-assignment

This is especially crucial when you break up your code in a myriad of small functions/methods: the whole point of refactoring was to make them readable, which means small and self contained —you don't want to keep a gazillion dependencies in mind while reading and testing a given method. Avoiding side effects is a big help there, because it will make your interfaces more explicit, and your dependencies more visible.

npsimons• 13 years ago

You probably just want to rename variables because you don't like the previous developer's naming convention

If the last developer named his or her variables along the lines of 'a', 'b', 'c', you're damn right I have an issue with their naming convention and I'm going to fix it.

When to refactor code

Related Stories