Legacy code can mean different things to different people. I know not everyone shares my definition of legacy code. However, the one common thing, regardless of the definition, remains the need to transition a codebase to something better.
This poses an issue because the most common conclusion in these scenarios is to lobby for a rewrite. I have another post, here, that covers why this is NOT usually the right option.
Action is a great restorer and builder of confidence. Inaction is not only the result, but the cause, of fear. Perhaps the action you take will be successful; perhaps different action or adjustments will have to follow. But any action is better than no action at all.Norman Vincent Peale – Author of “The Power of Positive Thinking”
Agnostic to the definition, there is a need or desire to change this code (even if you go for the rewrite), so the aim must be to make it easy and safe to change!
If there are already useful tests, excellent, go review and refactor them!
If there are no tests, then go write some!
Even when tests exist, their value requires assessment. There are some checks we can do to evaluate to what level we have the intended behaviours covered.
We could be in one or a combination of the following states
- No Tests
- No valuable assersions
- Implementation details tested directly
- Partial test coverage of required behaviours
- Enough behaviour covered with valuable assersions to support some refactoring
In all cases, except for the last one, there is some work on unit testing required before any work on the production code can safely take place.
Code coverage, at this point, proves useful if you have tools like ReSharper to indicates the lines of code not run by the execution of your unit tests.
Two distinct reasons exist for the un-covered code.
- The code forms part of an un-asserted behaviour
- The code is dead (unreachable) i.e., there is no existing code path that will execute it
The latter case, in my experience, results from some form of impossible state check or just redundant functionality.
In short, if it’s not needed delete it and if it’s not tested test it.
You must make sure any tests you add test code via the exposed API. Avoid the temptation to make implementation details public in order to test them. The inner workings of any specific implementation require a degree of isolation from the required behaviour under test. Testing an implementation’s private functions directly forces unit tests to know too much about that specific implementation increasing the code’s rigidity, making code coverage a less useful indicator of the value of those tests. Always test required public behaviours over the specific implementations of internal actions.