I work on a team of software developers that maintains several large codebases — too much code for any one person to easily know what’s going on in every part of it at any particular time. I found myself thinking a lot about how to keep the code healthy and a while ago I set my thoughts down as a list of good practices. Thanks to my coworkers at Endless for input, editing, and debate.
The good practices in this post differ slightly from the ones we adopted at work, which reflect the opinions of the whole team; these are worded to reflect my personal opinions.
Assumptions
I don’t like rules without a rationale. I believe these six assumptions underlie the rules that I set out below. That is, if you don’t agree with these assumptions then you probably won’t agree with the rules… ☺︎
- We can never know that our own code is correct.
- Left unchecked, we will believe our own code to be correct.
- Even small mistakes can lead to catastrophic data loss.
- Non-trivial programs have interconnections too complex to keep entirely in one person’s mind.
- Modifying non-trivial programs will break code unrelated to the modifications.
- The business value of maintainable code is only visible to developers.
Good practices for code health
Use your judgement
As always, rules apply only in the absence of any overriding reason to ignore them. Breaking them should be in mutual agreement between the writer of the code and the reviewer. (This system only works if everyone agrees about what the rules are in the first place, though.)
Example reason to break this rule: If no agreement can be reached, then the default is to follow the coding standards.
Review code
Code gets reviewed by a developer who didn’t write any part of it, because of assumptions #1, #2, and #3 — and to spread familiarity with different parts of the codebase throughout the team. Develop with ease of reading in mind, as if you are writing a letter to an unfamiliar code reviewer. Review code skeptically and with full attention, as if it came from a malicious agent out to erase your hard drive.
Example reason to break this rule: You are committing a trivial fix for a broken build and your continuous integration system acts as the code reviewer.
Observe the style
Code follows the coding style. Coding style is important because when code looks the same it’s quicker to read and errors jump out more easily. Apply automated tools when possible to save the code reviewer from becoming a parenthesis counter.
Example reason to break this rule: The code reviewer agrees with you that deviating from the style is more readable.
Test your code
Code needs automated tests. The rationale for this is assumptions #2 and #5, but could be the subject of an entire blog post itself. Lack of tests can be by itself a reason to fail code review, or at least start a dialogue between developer and reviewer about why tests are not necessary in this particular case.
Example reasons to break this rule: A one-off script. A component that proxies an external resource which can’t easily be mocked out.
Refactor on write
You will always have to deal with legacy code (code on which development has ceased but still must be maintained) and rushed code (code which you were forced by circumstances to check in that didn’t quite work well, works but is difficult to maintain, or is not tested.) By assumption #6, you will probably never set aside time to refactor code for its own sake. Therefore, refactor bit by bit to leave the code in a slightly better state each time it’s touched. In this way, code receives refactoring attention roughly proportional to the benefit you receive from refactoring it. If at all possible, add new code with a unit test even if the rest of the code is not written in a testable way.
Example reasons to break this rule: The code is already in good shape. The feature is critical and cannot be delayed. You are contributing your code to an open source project, in which case it is better to work with the upstream community to refactor.
Refactor only on write
Make your diffs per commit no larger than they have to be, in order to make code review easier. Since diffs go line-by-line, do not fix style errors in lines that that are not already being touched in the same commit. Use separate commits if there is an opportunity to make other style fixes.
Example reason to break this rule: If it makes more sense to fix lines other than the ones being edited in one shot (e.g. large sections with wrong indentation), do so throughout the whole file in a separate commit.
Pay down technical debt
Sometimes it’s not possible to build a feature without doing a large refactor first. Determine this as early as possible and include it in the time estimate for the feature. Do not shy away from paying down this debt; it will only compound if you borrow more on top of it. However, keep the changes incremental, and the functionality unimpeded while making these changes.
Example reason to break this rule: Extreme time constraints force you to take out a second mortgage on the code (even then, do this only with a healthy dose of disgust.)
The Mad Scientist Review 