This post was originally posted on devs@work.
After reading Crafting Better Code Reviews, I rethought and revised an old article of mine about our code review process. It's half a statement on how we make code reviews and half a write up of my wishes how they should be.
Code reviews in workflow
We do peer code reviews for nearly all code. This is coupled to github pull requests. That is to say one team member reviews the code, together with its author. We do one to one code reviews cause we are still a small team and we know each other relatively good. Two people team up to find bugs, control plausibility, code cleanness, coding style and maintainability. Author and reviewer should learn from each other how they write and read code on the one hand, on the other hand one learns about the functionality or feature the other has programmed so knowledge about the project is organically spread. Additionally it's a starting point for new team members to get into the code and the team. Truth is, we started relatively naïve with reviews, as we implemented our workflow we said „at pull requests someone should do a peer code review“ and that was it in the first place. A cargo cult to feel more professional but it worked out good so far.
Collective code ownership vs. personal creativity
"This code is our code", should be the first comment in all files. We try to cultivate our collective code ownership all the way with an emphasis on „our“. All pull requests should be respected as an intellectual extension of the code base. Mostly there is no right or wrong but just tastes sometimes the opposite. For often discussed problems (like tabs vs. spaces, camelCase vs. snake_case…) we developed our coding guidelines. Apart from that every coder has his or hers own style (surprise!). To respect these differences in style and approach and on the other hand not to get defensive about your own code is the great difficulty and responsibility of every code review.
Code author and reviewer should meet in middle in the hope to find the best code there. Both should feel to have added something, contributed to the code base. That is less esoteric as it sounds.
The review
Here we have some basic rules and notes how a good review would work. First of all both participants should reserve enough time for the review. And both should be prepared. The author should check his code before…
- he committed small chunks with good commit messages
- added comments to the code where applicable
- removed todo comments and commented out code
- self checked against the coding guidelines
- rechecked variable and class names
The reviewer will read the pull requested code prior to the review the full diff at best plus the corresponding story or ticket. Both parties should get together in a calm atmosphere (coffee anyone?). First the reviewer will ask questions about code chunks he does not understand or find them otherwise questionable. Then he will make change requests if needed. Reviewer and coder should make these changes together if possible. You can do this using line comments and the review tools in github, but talking to each other directly is the better choice, at least by Skype or screenhero. If the reviewer has change requests, he will ask for them and not make the changes himself. The exchange of diffs or patches can be reasonable. The reviewer has to explain change requests, do not frustrate or nitpick people please. On the other hand do not show disrespect not commenting on anything or always fast silent approval.
What are we looking for?
No code in this world is perfect. You can discuss writing style for the smallest hello world program. If you write more than 10 lines, bugs will show up quite frequently. Finding as much as possible of them is one priority. Another one is creating maintainable code.
We look for…
- off by one errors, infinite loops, missing error handling, antipatterns, wrong scope
- browser compatibility
- plausibility: does the code what is advertised, what was ordered
- efficiency: is it fast, memory efficient? Are reflows and repaints or DOM accession minimized?
- code duplication
- testing: are there enough tests, are the tests reasonable?
- naming things: is the hardest part, but are all names useful and in favor of the conventions (like BEM)?
- coding guidelines
- code cleanness
- comments and documentation
Order of discussion: first code that was not understood or understandable, things that do not work, later code style and cleanness. Always communicate first afterwards act together. Mind yourself: if your reviewer does not understand something, it is likely that nobody else will. Learn from each other. Talk about everything.
At any cost?
No. We want the best code, the best code possible. Your own coding style is maybe not the best of the world. The coding style of the author is maybe not the worst of all. Distinguish your preferences from functional needs. If you had long discussions on some points be less strict elsewhere as long as the code works. At the intersection of time and quality the is a tolerance margin that should be fully utilized.
Image by Émile Perron at unsplash.com.
Top comments (0)