Large maintenance
+ aggressive schedule
+ lots of new developers
+ minimal system documentation
______________________________
Need for highly efficient and
effective QA procedures
Where I've been working for the past few months, we've been under the gun to meet an aggressive deadline. As management is want to do, they've added several new developers to the project. One of the many reasons why adding developers is ultimately a bad thing is that, in addition to the complexity in communication, there is a risk of innocent well-developed code being added to the codebase that is Just Plain Wrong. Our system has been in development for many years and contains a wide variety of coding styles, implementation patterns and idioms. Some of them should Never Be Followed Again, and some are the Correct Way of Doing Things. There's really no easy way for a new developer to know what is what.
Now, outside of going back in time, creating pedantic test cases, gathering requirements and incessantly refactoring, we need an option to make sure bad code doesn't get into the codebase. By "bad" I don't mean poorly written or buggy code, I mean code that does not fit into the system as a whole. For example, a developer was writing some code to generate printable reports. His implementation resulted in a very nice report popping up in a Swing window in our application (our application is a Swing front-end to a JEE back-end). It was very well-implemented and looked great. However, everywhere else in the application, reports are generated by writing a PDF to disk and asking Windows (via JDIC) to "view" the file. This is the code I'm talking about.
Finding bad code
At the start of the project, we went through the arduous process of identifying each bit of work in an MS-Project file and assigning developers to it. New developers were given tasks that didn't touch core components, while experienced developers got tasks involving major refacotrings or database changes. Our project lead suggested that each module undergo a code review. It sounds reasonable, however all of us let out a collective groan at the thought of wrangling together 3-4 developers once a week for an hour or two to go over printouts or screenfulls of code, much of which was simply being modified.
One of the senior developers proposed the solution we ultimately went with: senior developers get emailed the diffs of all commits and make sure to spend some time reading and reviewing those commits. Coupled with our policy of "commit early, commit often", this has worked out great.
Diff-based code review
Here's what you need:
- A concurrent version control system developers trust. Recommed Git or Subversion if you must.
- A simple script to email diffs on every commit. Usually included as an example hook for must version control systems.
- IM clients (Google talk within GMail works in even the most oppressive environment)
- A sane version control policy: committed code must:
- Compile
- Not prevent application deployment/startup
- Not horribly break someone else's code (optional)
Developers should commit as frequently as they want (and preferably frequently). I typically commit code I feel is "done" but that might not add up to an actual feature. This requires accepting that head is not a real verison. Most real version control systems have the ability to tag, branch, etc. These features are for "real working versions". The head of the trunk is not.
- A sane coding style policy: if you must re-indent or change bracing style, do it in its own commit, outside of actual code changes. Better yet, don't do it at all. Formatting changes can obscure the history of a piece of code and should be made minimally, if at all.
The "process" (if you want to even call it that) is:
- Diffs get emailed to the senior developers as soon as they happen
- Senior Developers read the diffs, using IM to discuss any issues
- If code does have issues, the diff is forwarded to the developer who committed, with comments on what to change and why (senior developers decide amongst themselves who will send the feedback, or a "lead developer" can, if one is identified)
Part of this requires some level of diplomacy, however a plain, to-the-point email on what the issues are with a piece of code, why the changes should be made, and a suggestion on how to make them should be digestible by anyone.
I've had great success with this, having caught a wide variety of problems (even in my code, by others) without having to have one meeting or to print out one sheet of code. The fact is, on a maintenance project, you aren't reviewing the codebase, but changes to that codebase. Diffs are the way to understand what changes are being made.