Introducing code reviews to a team – Part 1
depthinfinity.com
depthinfinity.com
We've just recently introduced code reviews on our 3-4 person team. We're working in a particular environment, and most people have no production level experience in that environment. These people have gotten used to _just getting it to work_. We've clearly stated that we will be going with Google's style guide, but we can always divert, as long as everyone is in agreement. When pointing out that the changes are not according to the style agreed (frankly, it's not so much about style, as it is about using patterns from other programming languages, that are not applicable), the response is:
- it works
- I don't think you understood what this does (note: no comments or attempt to explain)
- In the future, please do not waste more time with pull requests. Desktop sharing and a call is the most efficient way to work in remote teams.
It's getting more and more hard to keep an open mind and not take things personally. We're at a point now, where there's a intermediary person refactoring the code.
Does anyone have any practical advice on people understand the importance of structure code, and to limit the _personal factor_?
As for the appreciation of code reviews: developers absolutely hate doing things without reason. For what reason does your team do code reviews in the first place? Is that reason unanimously understood? If not, then you'll face a lot of resistance.
In short, get everyone on the same page as to /why/ you want to use this tool (code reviews) and on the same page as to /how/ to use it.
--
also general code review advice:
- Don't make it all about style, and if you do, preface that you're nitpicking. automate everything you can when it comes to style.
- Use unambiguous questions, with detail. Set a high bar of quality for reviews by leading the charge yourself and holding others to that standard. Don't be afraid to preface questions with: "I don't think I understand how this works."
- Don't be afraid to applaud really cool things as well. Code reviews don't have to be negative comments only.
In a lot of companies you cannot submit a code without an approval from the reviewer. That puts the reviewer in a high ground when it comes to asking for explanation.
I'd take a step back. When people "don' t understand the importance of ...." the issue is almost always a breakdown of bi-directional communication. I like to start by assuming the person offering resistance has a valid point. Start by telling them they are right. If their approach is causing a problem then describe the problem in objective terms. Present your analysis to the resistance leader and ask for their help in solving it. When you do this the person offering resistance can often become your biggest advocate.
Recently I have become a convert of the retrospective with a focus on incremental improvements. Start with the process you have and commit to making it better with 1-3 concrete action items per cycle. It can be surprising where you land.
Above all code improvement requirements and comments should be based on industry defined best practices, like citing SOLID principals for example. Or, citing agreed upon team standard for appropriate level of abstraction. In short, there has to be a limit to the pissing matches.
I spent six months in a Big Dumb corp that did none of the above. Every team member reviewed everything and every comment had equal weight. Code review participation was part of the performance review process. Sounded nice in theory, but incentived a truly toxic environment of "my solution is more clever" and "my solution is better use of abstraction". Which resulted in a spaghetti mess of over engineered code that still had bugs, and still didn't meet requirements, even after 100's of hour of reviews.
One would think the first check in any review is "does this code do what it should do?" and stop the review right there if it doesn't. As a professor of mine once said "code that crashes fast and efficient doesn't help anyone."
They also do a bit of cross-training, which is of some value.
If you have people that send you constantly completely broken code and refuse to get better, there's just not much you can do.
As a veteran of a dozen different code review approaches I've identified a few properties of those that succeed and those that fail.
If you want your code review process to fail its easy just...
1. Make it about you and your preferences. Does this code adhere to my particular aesthetic preferences? Are the variables named the way I would name them? Do I consider this code readable? How can I force others to adopt my perspective? Remember the purpose of code reviews it to keep iron-fisted control of the code base such that it never becomes that dreaded "big ball of mud"
2. Demand that the team adhere to a set of rigid rules regardless of how practical their application is.
3. Most important focus mainly on the subjective qualities of the code(format, spacing, naming, etc.) Allow non-critical path items to hold up delivery and use those items to critique others based on an arbitrary measuring stick.
If you want your code review process to succeed. Its a little harder....
1. Be problem focused. What has bitten you in the past? Are you sure you understand the cause? Acknowledge that there is no "right way" and that you will build the perfect code review model through trial and error.
2. Start with the bare minimum and build on it with a regular retrospective process. Allow your team(s) to take ownership of the code review process both as an expression of what they want to accomplish and as a way to improve their daily lives.
3. Accept that you might have been the one doing it wrong all this time.
4. Focus on the objective qualities in the code. Is this the best approach to solving this problem? Does it work? Are there any obvious bugs? Based on our collective experience will this code cause problems later?
5. If you feel like you are nit picking then you are nit picking. Don't nit pick.
Code reviews can either be a tool to write better code or a form of weaponized OCD. Don't be the latter OP, you're better than that.
I think the whole idea of pushing commits to an open pull request is a good one because you can see the discussion and the commits which come after it.
GitHub actually does this nicely by showing it in a timeline where you have a comment and then a commit following it in the discussion thread.
For branches / commit sets? Totally fine.
For individual commits being rewritten (e.g. because you're using perforce and not on a separate branch)? I tend to push back on anything non-trivial or that will make the diff look dirty - e.g. no running mass autoformatting passes on refactored code, even if the rest of the file desperately needs it, and it's guaranteed not to break anything. Keep that in a separate followup commit. Some minor typo fixes - sure, those are fine.
Key phrases - when accepting feedback: "Sure, but can I save that for a followup commit?" - and when giving feedback: "Looks good - can you rename/fix/tweak X in a followup commit?" I'll generally note "Still TODO" in the actual commit when I do this.
> My org has not established a consistent policy around this and often people will push changes after some reviewers have already approved the request, which I find concerning.
Policy process and tools should all generally treat modified commits as unreviewed or partially reviewed, and modified branches as partially reviewed (assuming the earlier changelists/commits were not rewritten.) The additional changes haven't been reviewed.
If your policy doesn't require reviews for e.g. minor typo fixes, whitespace fixes, etc. - then as long as e.g. all the meaty commits have been reviewed, the pull request is still reviewed enough to commit. A quick "Still look good?" "Yep +1" is all that's necessary if your policy is tighter. Sometimes people will encourage you to not even wait for that - "yeah, go ahead and just commit once you've fixed these X minor issues."
Of course, your policy on that may not be established yet either - I do like it being slightly lax here, and to some degree I see what I can get away with, and it seems to work out OK ;). A tech lead's guideline to our team was simply "if I track a bug down to a changelist and find out it wasn't reviewed I'm going to be cross", leaving it up to your personal judgement how much you wanted to annoy him ;). I'm fairly up front about what I don't bother getting reviewed - this may be enforced by requiring such changelists to be marked, or they may send out email warnings - so I can trust in my coworkers (including e.g. my tech lead) to give me feedback if they'd rather I didn't take it quite so far.
> On the other hand, it is nice to see review comments being addressed in the same branch/PR. Thoughts?
In the same branch/patchset/bundle of commits? Sure. No problem. Encouraged, even, assuming it doesn't break your workflow too much. Unless they're getting unwieldy - but that's an orthogonal issue to the specific act of pushing changes. I get irritated when people throw monster refactoring patchsets at me, even if they do them perfectly and I end up having no changes to suggest. Even worse is when it's one commit. I always give a gentle nudge/reminder to break things up and submit in chunks if at all possible when it happens.
we discussed that someone could make major changes after approval which gets around the technical limitation of merging, and the answer we all agreed on was if we cant trust the author to know when to get re-approval then there was a bigger issue at play.