Code Reviews

One of the most pernicious lines I have seen in chat in a professional coding context is:

"I need someone else to approve my code before it can be promoted".

No, you need someone to review your code.

Approval is not to be assumed. Indeed, on general principles, it would probably be better to assume that on the first try there will be changes requested.

Code reviews are not there to get code checked in. They are not even there (only) to catch potential bugs. They are there to ensure that code is not only algorithmically correct but maintainable, tested, and has no other issues.

("No other issues" includes things like: "That function you extracted is applicable in these other areas of the code base. It should be moved to a more general library and applied to the other places" or, more seriously: "You have introduced a cyclic dependency between packages.")

I have come to view the code reviewing model implicitly promoted by the Atlassian tools and github as problematic. github defaults to having one developer approve code; it can be extended to more. github can require reviews by a "Code Owner", a senior developer. But where I've seen it used, the tendency is for the minimum number of reviews to be made: because the model assumes that the reason, the only reason, to review code is to give it a check up to some given level.

The other major reason for reviews is to familiarize the developers with the code. If people ignore requests for review once the minimum number of approvals has been met, then the reviews are really not doing their job.

Reviews should be done by everyone, all the time. They should be careful and detailed. Not all issues flagged necessarily lead to changes: in some cases there may be tradeoffs that have to be made, or the developers may have differences over what is best; not everything raised in a review has to be resolved in the way the raiser wanted it to. I have known developers who disagreed, generally, on whether classes local to functions were bad or good; on whether all pure functions should be declared static; on whether possible but not very likely errors should be handled by exceptions or by return values; and so forth. A review process should be a conversation with the end of making the code as good as possible.

(Seeing code reviews as a road bump on the way to approval is not actually the worst attitude towards code reviews. Some decades ago I had written an application that scanned a document for certain patterns, which it then looked up in a database, and then used the DB information to update and regularize the locations scanned. The document sets in questions were large, and the application was taking extensive time. There was much talk from management about code reviews. Then a DBA identified that the typical compound key used in the lookups was not indexed; adding an index dropped the application's time really dramatically.

Suddenly all talk of code reviews dried up.

Code reviews are not a way of making presumptively bad code better, except insofar as they make all code better. Once the idea of a code review had been raised, it shouldn't have been dropped because the immediate pain caused by poor indexing had gone away.)

It's not just "many eyes" as far as number: it's that other developers bring different perspectives. That's particularly important when considering maintainability.

However, there is another, larger issue.

Organizations which make even a token attempt at Agile methodologies and especially continuous integration pretty well have to support mandatory code reviews at check-in. (Although sometimes management puts visible pressure on to get code checked in and into QA as fast as possible, which leads to the attitude I started out citing.) But they rarely - in my experience, next to never - provide an explicit place for group code reviews of existing code. The benefits of such exercises are manifold:

• They familiarize a team with the existing codebase. It's an opportunity for newer members of a team to learn from longer-term or more experienced ones.
• They catch latent bugs; in my experience I see bugs (usually in edge cases, logging, or error handling) about a third of the time in examining "stable" production code.
• They can identify commonalities leading to refactoring which assists maintainability. As such refactoring should always be accompanied by unit tests, this also means extending test coverage over the codebase.
• Frequently the code is the product of people who left the organization long ago. This allows a more no-holds-barred critique of code quality.
• They improve the familiarity with the codebase needed to get good estimates for a new project.
• Generally, it's a team-building exercise.

The only drawback is that they do not directly contribute immediately to getting out new features (they indubitably assist on catching bugs, but perhaps not reported bugs).

The answer to that is that they pay down technical debt.

Theoretically, technical debt is undertaken with the explicit awareness that it's a short-term fix driven (normally) by an imminent deadline; the implicit corollary is that it should be paid down as soon as possible after the deadline is past, while the issue is fresh in the developers' minds. (If you are creating technical debt with an expected pay-down date of more than, say, a couple of months there is something wrong with your scheduling. If the pay-down date is "after the end of the project" you are on a death march.)

Of course, in practice most poorly maintainable and/or buggy code is usually not explicit technical debt. It's code that was written by developers who didn't care about anything except getting the specified deliverables done with as little time or effort as possible, and with a lot of skimping on edge cases. It's also usually "legacy code" by Michael Feathers' definition (Michael Feathers, Working Effectively with Legacy Code (Prentice Hall PTR 2004)) : code without unit tests (and usually with only business acceptance levels of testing at the QA level, which also tends to skimp on edge cases until they pop up in production).

If you manage on meeting deadlines to the exception of all else, unless you were very good about doing the estimates[1] which went into the deadlines, there will be a steady deterioration in the maintainability of the code base. Bugfixes and new features gradually become more difficult and hence longer to do, and more likely to generate bugs which pop up in acceptance testing (pushing your actual development/test time further away from your estimated development/test time). Managing on time makes timelines worse in anything but the very immediate short run. (And I mean very immediate. In most contexts I've worked in, any location that gets touched in the course of a project is likely to be touched several times. If the first time it's touched makes it cleaner, the subsequent changes are simpler.)

Managing on quality, though, creates a positive feedback not only on quality and maintainability but also on time. As the code becomes cleaner and better-tested it becomes quicker and easier to add new features or find and fix bugs.

The sort of review I've been conducting here is not a true code review: I bring only my own perspectives to bear at a later time. But even then, reviewing with an eye to presentation, I've discovered a number of bugs as well as a fair number of places where the code could be made clearer and more comprehensible. (Comprehensibility in this case is dependent on familiarity with the new functionality I'm exercising.)

[1] The only context in which you can expect deadlines set at the outset of a project not to move is when the following conditions are met:

1. You know the extent of the project exactly and can guarantee no scope creep.
2. You know the quality of the codebase you are working on exactly and know exactly how much overhead to put in based on that quality,
3. You know the capabilities of the team you have and know how those capabilities are matched to both the scope of the tasks and the relative ease or difficulty of working with the codebase.
4. The work to be done falls firmly into the category of "known art" as far as the team goes. (This tends to mean that consultants, who do similar work repeatedly for different clients, are frequently, in principle, in a better position to do accurate estimates).

All this includes acceptance testing as well as coding. An insufficient set of tests means discovering bugs in production.

Even then, best practices require providing error bars around your estimate. One of the implications of this is that if you do think that all of the above are true, it's still best to use a real project management tool -- MS Project is a baseline -- and associated project management disciplines, to manage the time aspect of the project, and not the partial support provided by the Atlassian tools.

In my experience, frequently none of these are true, especially if you have new hires, old code, or a heavily involved business. If you set up deadlines based on rough estimates from the developers and QA staff, you might be lucky and come in under the deadline, but most people underestimate. Even experienced developers tend to underestimate, partly because they will tend to do more work if they run into code that needs cleaning up -- they may do it faster, but there's more to do.

Agile is an attempt to deal with this dilemma by having really tight feedback loops, optimal communication, and a short time horizon for each bit of a process. I don't think it's the only possible working methodology, but I do think that the only alternatives involve a real need for project management expertise, and also require that the second condition above is met -- because misestimation of code quality (leading to cascading sets of bugs that need fixing once the application is touched) will prevent estimates from having any contact with reality.