Refactor/rewrite code or continue?

Question

I just completed a complex piece of code. It works to spec, it meets performance requirements etc etc but I feel a bit anxious about it and am considering rewriting and/or refactoring it. Should I do this (spending time that could otherwise be spent on features that users will actually notice)?

The reasons I feel anxious about the code are:

1. The class hierarchy is complex and not obvious
2. Some classes don't have a well defined purpose (they do a number of unrelated things)
3. Some classes use others internals (they're declared as friend classes) to bypass the layers of abstraction for performance, but I feel they break encapsulation by doing this
4. Some classes leak implementation details (eg, I changed a map to a hash map earlier and found myself having to modify code in other source files to make the change work)
5. My memory management/pooling system is kinda clunky and less-than transparent

They look like excellent reasons to refactor and clean code, aiding future maintenance and extension, but could be quite time consuming. Also, I'll never be perfectly happy with any code I write anyway...

So, what does stackoverflow think? Clean code or work on features?

Answer 1

Is it likely that others will be subjected to the horrors that you have wrought? When I have horrid code about to be released into the wild I tend to clean it up, but if I'm the only one that's going to be subjected to maintenance and dealing with it, I'll let it fester for a while since it works well enough and if I sit on it a bit I may even come up with a better solution than I would have had I been inspired to "fix" it right away.

IMHO, of course.

Answer 2

"Some classes leak implementation details (eg, I changed a map to a hash map earlier and found myself having to modify code in other source files to make the change work)"

This is enough of a reason to make it worth the effort imo. By definition it's something that users will actually notice.

Some other important things to consider:

• Maintainability - How easy will it be to fix any inevitable bugs that crop up?
• Testability - Working your code into a more testable form is a great way to refactor it. Adding unit tests then means you can change it and know exactly what breaks.
• Expandability - Do you think you'll ever need to add new features?

Really the only time you wouldn't want to consider making your code better is if you know for certain you'll never need to touch it again (i.e. ALMOST never).

Answer 3

I would argue that if you feel the need to refactor this code, then you haven't yet "completed" it.

I think it's far better to refactor as you go (assuming you have tests and are following the red, green, refactor method of TDD), but given where you are, there's not much point in telling you what you "should" have done.

I would advise you to refactor it. However, the pragmatist in me would give the following caveat - code is never "done", so timebox your refactoring this time (so give yourself an hour or two, keep the tests passing, and then call yourself "done for now") and then every future time you touch the code, follow the boy scout rule and clean the code a little bit every time you check it in. Over time, you'll clean up this code - and in future, try to refactor as you go, as part of the development process, rather than as a big task to do at the end of development.

Answer 4

I'd say refactor always. Every extra line of code and all those nagging little gotchas will come back and bite you in the ass.

In the future you might even refactor as you go--the time is never waisted--even if you throw it away you've still learned how to do something better.

Answer 5

From what I can tell you are a bit of a perfectionist and want to make your code as clean as possible, a worthy trait, but in business you have to consider the cost benefit analysis of what you are doing. If the code is up to spec and there are more important features to work on, make a note that you would like to refactor the code and move on. If you ever get some free time, you can come back to it.

Answer 6

"I just completed..."

It sounds like you are actively writing legacy code, which isn't a good thing. You've highlighted lots of reasons why you might want to refactor your code, but as you say it works to spec so there is no present cost to keeping it as it is.

On the plus side, how the code works (and should work) is probably fresh in your mind so refactoring now is likely to be cheaper than refactoring later; on the other hand refactoring later with an actual change request in mind will increase the likelihood of refactoring in the right direction.

In future, I would seriously consider slowing down and thinking ahead as you write code. Refactoring (or factoring!) as you go is much easier and you don't have the "but it works" pressure to just keep things as they are.

Watch. If you notice a class seems to be gaining multiple responsiblities, stop and think if you need to separate out the responsibilities into separate classes.

If you notice that you want to reach into the internals of a class, stop and work out what the interface between the classes should be so that you don't need to know and use a concrete class' internals.

Answer 7

If this code has already passed QA, then perhaps you should release it as is and accept the code debt. If there's time to spend on it, I would pick the pieces of this that are most egregious and try to refactor those, for example wrapping up your hashtable implementation. Otherwise, make a list of things to improve and work the improvements in with your future change requests.

It's good to know about yourself that you won't ever feel content with the code. Remember that so you don't let your perfectionism get in the way of regular releases.

Answer 8

Refactor it now.

Since you are even considering refactoring, it sounds like you can make the time for it. If you leave it for later, chances are you will get busy with other things and lose this opportunity. When schedules become tight, clients can understand if you need some extra time to finish a new feature. They are much less understanding when you tell them you need extra time to rewrite something that is already working.

Answer 9

If you have a set of tests to verify that your code works now, and after your refactoring, then I would say, if you have time, fix it, since it will cost you more time later. But, if time is pressing, then it's perfectly acceptable to put this down as code debt and come back to it at a better time. If the code works, you can cut a perfectly good release with it now and come back to it later when you have more time and less pressure.

Answer 10

Work on the aspects that will lead to bugs later on. Don't goldplate.

Answer 11

In one of your comments you said "a lot of code will be built on top of it." You say it works properly, but if it's sloppy and functions on bad design principles, it will become harder and harder to change anything as time goes on. In general, the more important a piece of code is, the better designed it should be. Get the design right before you add things on top of it (within reason).

Plus, one of the main goals of writing software (in my opinion) is to manage complexity. If you got it working, you have half the job done. The other VERY important half of the job is to make things reasonably easy to understand and change. Without that, you create software that does not respond well to changes. And software needs to change. Often.

My company has some software that was built very sloppily in its infancy, and every little change that was added over the years was sloppily nailed on top of the already sloppy foundation. Now it's at the point where people are asking for simple things (such as mouse wheel support for scrolling), and the lead developer responds with "You don't know how much I would have to change in order to do that." However if the code was cleaned while it was being written (or shortly after), we wouldn't have this problem.

If you have sloppy, poorly designed code now, it will bite you in the butt later. Especially if lots of other code will be written on top of it.

Answer 12

If you have a set of automated tests that verifies your code is working correctly, wonderful - go ahead and refactor it; now is the best time, while it's fresh in your mind. If you don't - use this time, while it's fresh in your mind, to write the tests. If you have time for only tests or refactoring: write the tests so that it's easier (some would say possible) to refactor later.

The TDD cycle is RED-GREEN-REFACTOR, and it says you're not "completed" until the design is good - that is to say, until it doesn't need refactoring any more.

Answer 13

It's actually not that complicated. Just follow this rule:

Refactor code often. Refactor every time when it's possible.

Hence, if you think you can refactor your code, you should just do it now before it becomes much complicated later when all the codes are so tightened together.

Answer 14

Now you have your refernce implementation. If you feel motivated, take a shot at refactoring. Don't be afraid to throw away and move on or restart.

Perhaps redo the api/interface based on the consumer code and drive the refactoring top down.

You shouldn't really be bothered by having public data members. People write way too many accessors which end up being typing for fun and frankly the source of really stupid and frustrating bugs (the only code without bugs is code that's never been written). That's irritating. Only encapsulate to protect fragile states (when member data state is coupled).

Answer 15

So, what does stackoverflow think? Clean code or work on features?

The question I see here is what price you will have to pay if you refactor vs. what price you pay if you don't.

If you leave it as it is, it will cost you for any maintenance (extra time, increased possibility of adding new defects with each change) and reputation as a developer ( any other coder looking into it may probably curse you :) ).

If you can guarantee nobody will touch that code ever again ( not because it's too horrible ;) ) you can safely leave it as it is. (In 10+ years as a developer I have never found a situation where I could guarantee this).

In all other cases, you're better off refactoring.

They look like excellent reasons to refactor and clean code, aiding future maintenance and extension, but could be quite time consuming.

You don't have to make the code perfect. In fact, you should take it one change at a time, and refactor in iterations, and if you do this right it won't affect any other code.

This means you should be able to refactor depending on your available time, not the other way around.

For example, if you only break one of your multi-purpose classes in three or four separate classes and update the code using that, you'll have the rest of the code working the same as before. If you're out of time when you finished this change, you can leave it as it is (just a bit clearer, in just one area).

If you have more time, you can take the next refactoring and repeat.

Furthermore, if you use a branching source control system you can even work in parallel for refactoring and adding new features with relatively little additional cost (and continue refactoring as you have more time).