Incremental code quality improvement of a legacy Java/Android codebase
We’ve all seen and touched it. Legacy code. The sound of it has a negative tone and it implies that the code is untested, hard to maintain and should be rewritten from scratch.
It doesn’t have to be however. As opposed to obsolete code, legacy code still functions and runs in production. The code can turn into legacy for multiple reasons, but one of them is lack of regular maintenance. At its extreme, a system that is not maintained and kept up to date can turn into something that no one or very few understand.
Software quality is multifaceted, and to keep it simple this text will focus on ways to improve maintainability — by measuring code quality using static code analysis tools (quantitative analysis), reviewing code (qualitative analysis) and acting on the findings. Verification and automated testing is equally important to guarantee the quality, but that’s a story for another day.
Tools to the rescue
The first goal is to stop the quality from getting any worse, and this is where tools can be of great assistance. Instead of pushing commits directly to the version control system (VCS), push to a code review tool (such as Gerrit) and hook up a CI job (such as Jenkins) to trigger builds on commits (BoC). Once that is set up, you can start putting in the safeguards that will prevent the quality from deteriorating. Pre-commit hooks are also one way of running checks before code is committed.
Reduce static code analysis warnings
Static code analysis tools (such Lint, FindBugs, Coverity, PMD etc.) are great for picking up common coding mistakes and reducing code smell, and by hooking them into the BoC job it’s possible to fail the job if the error count increases. Jenkins allows you to set a threshold, so a first safeguard is to set the current count of warnings as the threshold, and then incrementally fix the issues and reduce the threshold down to zero.
Collecting metrics for a combined view
Static code analysis tools very distinctly point out where the issues are and their severity, but to get a combined view and prioritise correctly we need to take a step back and look broader at the source code. Even if a file is flagged as having a lot of issues, if that code rarely executes or is rarely modified there may be more important files to fix first.
The VCS holds some interesting data that could help us figure out the priority. By going through the full commit log, we can calculate the churn for each file and rank the files by how often they have been changed. This requires some historic knowledge of the source code, as files are changed, deleted, renamed and code gets shifted around. Also, feature development and bug fixing inevitably effect the churn. However, if a file is flagged as having many issues according to the static code analysis tools, and the churn is high, that’s most likely a file that should be at the top of the list.
SonarQube can be used to calculate the technical debt, as well as other metrics, of each file and of the whole project, and then present the results graphically and enable trend tracking.
Tools don’t have guts
Tools are great, but they do at times show false positives that require interpretation, and they don’t show the complete picture. If a team has worked on a codebase for a while, it knows where the warts and the dragons are — which files should not be changed without extensive testing because historically they’ve shown to be roots of regression.
To complete the view from the tools, it’s good to get the team’s view. For example, one option is to query the whole team and ask everyone to list, in priority, the problematic areas and files, as well as what areas they would improve first. Collect the feedback, summarise and follow up with everyone, either individually or sit down as a team and discuss the priority. That’s also a good time to discuss the output from the tools and how it fits with the team’s view. Once the results are combined, you’ll have a list of prioritised areas and files that you can start working on.
Incremental Checkstyle with Gradle
For a codebase that has been around for a while, it’s not uncommon to find different styles and coding standards in different files and even in different methods and fields within the same file. The lack of coherence takes the reader’s focus from what really matters — the logic — and shifts some of it towards understanding, or getting annoyed by, the style. Fixing a project to use a common style, and getting everyone on board can be daunting at first.
“How do we decide on what style to use? And applying a style to the whole project would take time from delivering features and fixes. We don’t have time for that!“
Instead of taking on the whole source code, start small and do incremental improvements. Files that are never touched don’t need to be corrected right away. To apply a style to a project, Checkstyle can be used. The Checkstyle site has a couple of standard styles you can use as a starting point but it’s recommended to look at each available check, take only the ones you need and tailor them to how you want to use them.
If the project is using Gradle, the following Gradle configuration can be used to run Checkstyle on changed files only, instead of the full source.
Treat compiler warnings as errors
Compiler warnings are there for a reason, and the reason is not that they should be ignored and flood the build log. Rather, they should be addressed. Again, this can be done incrementally by setting a threshold to prevent an increase of the warning count and then working your way down to zero.
Code reviews are essential for spotting logical errors, and they can be an effective complement to the analysis done by the static code analysis tools if done properly. A lot has already been written on this topic, so I’ll just add my 2¢ based on a few lessons learned.
Keep changes isolated
As a rule of thumb, keep the number of changes done in a bug fix commit down to one. While doing root cause analysis of issues in legacy codebases, I’ve often found the cause to be an unrelated change where the commit message doesn’t match the particular code change.
Encourage small refactoring work
This may sound contradictory to the first point — but to keep improving the code quality, refactoring has to be done continuously to keep the quality from deteriorating.
It’s a fine line — sometimes the refactoring work can be done as part of the bug fix, sometimes it should be done in a separate commit. While a rewrite may sometimes be the solution, small refactoring commits have several benefits. They can pave the way for a larger improvement — or even make it unnecessary. It also gets easier for the the team to work with the code with every improvement. It can shift the attitude in a positive direction, so that instead of complaining about “that file”, we are now at least doing something to clean up the code.
Make use of the VCS history
Each line of code has a history, and in a legacy codebase that history tend to be quite long. While reviewing a code change, it’s good to visit the VCS and track backwards why the code looks a certain way. That’s especially important if the code has been maintained by others that are no longer working with it, as you can’t rely on their experienced input during the code review.
A common pattern I’ve seen is when a reported bug is a variation of a previous bug, and the attempted fix aims to fix the new bug in a way that, for the old bug, was tried, merged and then reverted due to regressions. By following the VCS history you can prevent that from happening again. Fortunately IDEs like IntelliJ IDEA support tracing line changes within the tool.
While writing the next shiny thing is fun, proper maintenance is equally important for the life span of a product. Without proper maintenance, the codebase can quickly turn in to a legacy mess that forces the team to put more effort into bug fixing than feature development. Therefore it’s important to put in code quality safe guards early.
When taking over a legacy codebase with a lot of code quality issues, it’s easy to throw up the hands and feel that there’s just too much to fix. It doesn’t have to be though — start small and do incremental improvements and you’ll see that the code will start to smell better and better.