Most software engineers practice the ancient rite of Code Review, a humbling experience in which we submit our work to each other’s judgement. Skillful code review not only improves the code, but edifies the participants, both reviewer and reviewed. It’s one of the most effective ways to transform a group of individuals into a team working collaboratively toward shared goals. When done badly, code review instead becomes a recipe for shame, anger, and weakening of the codebase through loss of conceptual integrity.
Don’t train alone. It only embeds your errors.
—Vesemir, The Witcher 3: Wild Hunt
Too many cooks spoil the broth.
—Anonymous
Let’s talk about (how to avoid) bad code reviews.
Authors:
Thou shalt not send huge change sets for review. Yea, verily, ‘tis far better to submit one hundred Pull Requests of ten lines apiece, a few at a time, than to eventually plop out a single PR for a thousand-line code change. If thou canst not figure out how to present thy changes as a series of small, independently reviewable pieces, thou shouldst brainstorm with a colleague, and invest thy faith in the power of factoring.
Thou shalt not send multiple, simultaneous PRs that depend on each other, as any change to one PR may invalidate others. Even if thou dost not mind manually synchronizing several PRs, thou layest a gross burden upon the reviewer, who hath not thy understanding of the interrelated change sets thou proposest, nor desireth to make busy-work for ye. Have a heart, give the reviewer a break, and please, never submit a PR-storm.
Thou shalt not send copy pasta for review. Thou not only irritatest the reviewer on general principles, but obligest them to review the same code again and again, and puttest them in the awkward position of leaving the same comment at each location.
Reviewers:
Thou shalt be responsive. Thou shalt not leave PRs unattended for days or weeks.
Thou shalt not let the perfect be the enemy of the good. Mere room for improvement is not sufficient reason to delay merge. Improvements can be made in later PRs; and if thou feelest strongly enough, thou canst darned well send such improvement PRs thyself, after merging the original. The question thou shouldst ask thyself is not “could this be better?” but “will this make us better?” Will the incoming change, as it stands, be a net positive? If so, thou shalt merge, and that right soon.
Thou shalt not delay merges in the name of bike-shedding. Thou art a lookout, not a gatekeeper. Thou art a teammate, not a fashionista. Thou art not the arbiter of good taste. Support and encouragement are more effective than exclusion. Water the flowers, and let the weeds die on their own.
Thou shalt be gentle. When thou must request changes (or even reject entire PRs!), no matter the reasons, thou shalt remember thy humanity, and that of they teammates. If thou believest that people should learn not to take professional feedback personally, then thou hast yet much to learn thyself. Disrespecting the work disrespects the worker. Thou hast been invested with power, and shalt wield it with compassion.
It would be a missed opportunity to think of code reviews as solely quality control. I’ve learned at least as much from reviewing other people’s work as from their reviews of my own. As a manager, I’ve found code reviews an invaluable way to break down silos, ensuring that the code is not “owned” by any single individual without whom it would be unmaintainable.
A final note is that code reviews are often worth following even if you’re not an active participant. As workplaces have become remote, and communication increasingly asynchronous, reviews have gained importance as a way for engineers to coordinate their activity. If you want to see how your developers actually work together, look no further than their code reviews.
Please use a more modern language. That is 500 years old, and difficult to read by those who learned only modern English. I know that the most common translation of the Bible into English uses that language, but I have never read the Bible in English.
Spot on. As a Senior engineer, I was reading the first list and thinking: "I know, I hate when my colleagues do that!", but then, as I got to the second half, I was thinking: "Oh. I might be guilty of that. I need to make sure I do not do that". I am going to make this the manifesto for our team if you don't mind Jeff!