It Aint Broke

.... so why fix it? Many managers and programmers don't see the wisdom in changing a piece of code if it already "works".

Maybe someone should come up with a pre-built Powerpoint presentation which explains to people that code that just does what you need today is next to worthless if it can't later be easily changed to do what you'll need next month or next year.

-- RusHeywood

I disagree. To me, this seems a case of YouArentGonnaNeedIt. One can argue that you're going to pay for your TechnicalDebt later. But I believe you should only change the "ugly" code when you would have to pay that debt. We have a certain class in our system. Is the implementation ugly? You bet! Does it work? UnitTests say so. Are we going to change it? No, because this is going to take a week and our program will not be better (less bugs, more/improved functions) because of it. -- ThomasMaeder

We teach people to clean as they go. The rule DoTheSimplestThingThatCouldPossiblyWork has two parts: first, add the feature in a very simple way; second, make the resulting total code as simple as it can possibly be.

This link leads to the complete chapter of a book I'm working on: WeWillCleanItUpLater.

That said, once the code has been deserted and is no longer under change, I'm mostly with Thomas: refactor it next time you need to change it. There was a time on C3, however, when things slowed down. We looked into it and figured out that we had been skipping the second step a lot, and wherever we went, things were grubby. We actually spent an iteration refactoring (not a good thing to have to do) and things sped back up.

One effect of poorly-factored code is that the objects aren't what they should be ... which means that when you need something, the objects aren't what you need. When you need something that ought to be in the system, and it isn't, that's a good time to refactor over there where the grubbiness is, to break out the part of the thing you wanted.

-- RonJeffries

Shouldn't that be three steps? First, refactor the code so the feature can be added in a simple way (if necessary). Second, add the feature. Third, refactor the resulting total code (if necessary). -- FalkBruegmann

One effect of grubbiness is that it becomes hard to estimate your work. In many cases, we remember the simple thing that should be rather than the grubby thing that is. Add to that the fact that if your system is not kept clean, you'll get that sinking feeling... you don't have enough to time to reduce the grubbiness before you add your feature, and you know that it'll pile up, making it harder to add the next feature.

See also CruftMultiplies.

-- MichaelFeathers

Speaking of the "sinking feeling", the book ThePragmaticProgrammer sets forth a basic concept summed up as 'Don't Leave Broken Windows' [cf FixBrokenWindows]. They cite a study that an abandoned car with intact windows will generally be left alone, but if you break one of the small rear windows, within a few days the entire car has been broken and abused. This happens with buildings too, and the neighboorhood deteriorates as a result. If you leave bad code as you go, it's a sign that you don't care and, consequently, others will stop caring as well. -- DaveCantrell

I see the unwillingness to refactor as one of the biggest problems of "if it ain't broke don't fix it" - I once worked for a (very pointy-haired) manager who literally said that "the objective of maintenance is to minimize the output of diff." Refactoring, in this organization, was overtly discouraged. Even changing local variable names to better reflect their use was discouraged.

This bad attitude seems to come from the belief that you can minimize the cost of maintenance (and make it go faster) by minimizing the number of lines being changed.

Like... "there's a cost, per line, to writing and changing code, so if we minimize the number of lines changed, the cost will go down." However, this is a mistaken belief (an AntiPattern): The accumulation of "cruft" / TechnicalDebt quickly slows maintenance - eventually to the point that the system must be discarded and rebuilt from scratch.

RefactorMercilessly, even without the other XP practices, is the solution.

-- JeffGrigg

To be effective, code has to stay in sync with reality. Attempts to keep the world from changing by preventing code changes cause a great deal of tension. -- mf

All of the above is true. As soon as you have a concrete reason to rewrite the offensive code (as, for example, you have to program around it to implement something else), you should go and refactor/rewrite/whatever. I'm just against gratuitous rewrites. If you're doing major rewrites of some components, this means more work, which again means less time to refactor where your system really hurts. -- ThomasMaeder

I agree. I wouldn't do a major rewrite against ugly code unless I had no confidence in it. I'd incrementally refactor as needed and as time permits. RefactoringIsNotRewriting. ExtremeReuse deals with this theme also. -- mf

Possibly unrelated, but the "minimizing output of diff" caught my eye...

I work with a lot of projects as a patch submitter (as opposed to a core developer). I like to practice "minimal-diff-+", i.e. minimize the number of "+" lines (added or modified lines in a unified diff). This isn't the same as "minimal diff", i.e. minimize the total number of lines. It's mostly about reading the actual changes I made after the fact, and checking to see that I've done everything I set out to do and I haven't accidentally left crud in the code. It helps the code's real maintainer out too, since the shorter patches tend to be somewhat clearer about intent (and often maintainers just look at the patch and say "That's a good idea, but I'm going to do it myself, differently"). It also tends to make the code smaller on average, since I don't restrict the number of "-" (removed) lines. -- ZygoBlaxell

Submitting patches to a project is very different from "normal" development. The most important constraint is that the patches have to be understood by a human, who will determine if they should be applied or not. Refactoring is much harder to do within these constraints, since even a few simple renamings can generate a huge patch. These kinds of projects also generally don't use ContinuousIntegration.

I am a committer (developer with write access) for the FreeBSD project, and we *do* attempt to minimize the amount of new code and the amount of changed code in each diff. The basic concept is to make each diff DoOneThingAndOneThingOnly in order to make it easy to review, and make the VersionControl history as meaningful as possible. I practice the same in my personal development, doing careful review of each diff before I commit it, and attempting to minimize it. I find this gives me a very good focus on what I'm doing, and hinders spurious changes (e.g. debugging code) from entering the repository.

This does not hinder me from refactoring - it just makes me split refactoring changes from other changes *at the diff level*. I do this by editing the diffs after I have produced them, and backing out parts of the diffs before I commit (keeping the generated patch files around). If I want to change a variable name, I usually add an XXX comment near the definition, pull this out to a separate patch file before doing the commit, and do the relevant refactoring as a separate commit. Slightly more work, but it makes each change clean, and makes for a much safer working environment - at least if you do not have a complete UnitTest for everything. -- EivindEklund