Low impact refactoring and code cleaning of sloppy code while waiting for requirements

Question

I inherited an existing code base for a product that is reprehensibly sloppy. The fundamental design is woefully inadequate which unfortunately I can do little about without a complete refactor (HIGH coupling, LOW cohesion, rampant duplication of code, no technical design documentation, integration tests instead of unit tests). The product has a history, high exposure to critical "cash-cow" clients with minimal tolerance for risk, technical debt that will make the Greeks blush, VERY large codebase and complexity, and a battle-weary defeatist approach to bugs by the team before me.

The old team jumped ship to another division so that they have the opportunity to ruin another project. It is very rare that I experience a Technical Incompetency Project Failure as opposed to a Project Management Failure but this is indeed one of those cases.

For the moment I am by myself but I have a lot of time, freedom of decision and future direction and the ability to build a team from scratch to help me.

My question is to gather opinion on low impact refactoring on a project like this when you have some free time during the functional requirements gathering phase. There are thousands of compiler warnings, almost all of them unused imports, unread local variables, absence of type checking and unsafe casts. Code formatting is so unreadable and sloppy that it looks like the coder suffered from Parkinsons disease and couldn't control the amount of times the space bar was pressed on any given line. Further database and file resources are typically opened and never closed safely. Pointless method arguments, duplicate methods that do the same thing, etc..

While I am waiting for requirements for the next feature I have been cleaning low-impact low risk things as I go and wondered if I am wasting my time or doing the right thing. What if the new feature means ripping out code that I spent time on earlier? I am going to start an Agile approach and I understand this is acceptable and normal to constantly refactor during Agile development.

Can you think of any positive or negative impacts of me doing this that you would like to add?

Answer 1

Firstly I'd like to point out that unit tests are not a replacement for integration tests. The two need to exist side-by-side. Be grateful that you have integration tests, otherwise one of your small refactorings could well make one of the low-tolerance cash cows go ballistic on you.

I would start to work on the compiler warnings and unused variables and imports. Get a clean build first. Then start to write unit tests to document the current behaviour, then start the real refactoring.

I can't really see any negative impact. You will gain a lot of deep understanding of the code base, which will help with bigger refactorings. It is almost always preferable to refactor than to rewrite, since during refactoring your still have a working product, whereas during the rewrite you don't. And in the end the sales of the product have to pay your salary.

Once the requirements are starting to come in, I would use what I call the spotlight approach. Do the usual agile thing (prioritize, then cut off a small slab for an iteration, and work through that) and leave quite a bit of time for code improvements. Then refactor where you are working anyway. Over time this will cover wide areas of the code base without you ever having to venture into areas where you would have difficulty justifying to management why you are working on that part of the code.

Answer 2

Writing unit tests might be more valuable use of your time: it will give you some insights into the current workings of the codebase, and should you decide to start from what you have rather than rewriting everything from scratch, you'll have a solid base to make changes without taking too much risk. Chances are that in writing unit tests, you will also make changes to the codebase just to get it into a testable shape; those are good, because unit-testable usually also means modular, encapsulated and mostly independent from external state. And, most of all, well-written unit tests are invaluable technical documentation. With unit tests in place, you will be able to judge what the current codebase can and cannot do, rather than making wild guesses or rewriting things just in case.

Answer 3

Blending answers - my thought would be to blend testing and cleanup - maybe 50/50? If you work an area doing a TDD sort of approach - set up the tests you need to know that the unit and integration testing is as desired and then start fixing. Move on as time allows.

It probably means you won't make as much progress, but it means that you'll have a nicely stable code base in some areas, at least, and you'll have a good starting base.

My gut reaction initially was "can it really hurt to clean up broken code?" (aka, compiler errors), but then I remembered times where fixing the problem actually broke functionality in some really weird cases because instead of a thread dying, it left memory around in bad places - essentially a bad break was masking an even worse error. It can literally be a Pandora's Box of unpleasant surprises.

Given that you're doing this while waiting for more requirements, I think anything you can to diagnose the chaos will greatly add to your long term sanity.