What do you all think about the practice of pushing changes to an open pull request based on code review feedback? 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. On the other hand, it is nice to see review comments being addressed in the same branch/PR. Thoughts?
We use Bitbucket at work and were able to configure it such that when this happens Bitbucket automatically unapproves the pull request. This seems to work pretty well and allows code reviews changes to be seamless, e.g. the extra commits can be squashed. I would think github/gitlab etc probably have similar features.
I think if you make the developers life harder and ask them to abandon the pull request and submit another you'll run in to objections.
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.
You say it's concerning, but why? Have you asked what your team thinks? If they are responsible for the code they're maintaining, then they ought to be able to determine their own process for review. From my experience, I have seen issues arise during last-minute code clean-ups that were not even in response to review comments, but I don't think it's worth the overhead of 'yet another process' for everyone to remember unless you can automate it.
> What do you all think about the practice of pushing changes to an open pull request based on code review feedback?
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.
if theyre review comments that need action then it sounds strange that it was approved? I guess for minor things that happens, most often we'd just ping the person that made the comment even if they already gave a lgtm.
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.
These experiences mirror my own in an uncanny way (recently made team lead, 3-4 devs, just started code reviews) and found all of this information extremely useful. I would add that discussing how much mileage can be estimated on the current stack is important as well. Good code can still be dangerous if someone in the team knows or feels they are only going to get 3 months out of it before dependencies will change and will need to update it again. I've found discussing sustainability from time to time helps to figure out who is checking in their code just to make it to the next day and who has an eye towards creating a low maintenance product. Life pressures can cause the best devs to live one day at a time they just need the proper forum to express it to the whole team. Sometimes it just simply gets forgotten or lost in the shuffle- the fact that library or API "X" will no longer be available.
I've been code reviewing everything for my entire team for about 6 weeks.
The idea being that we have some kind of quality control on the stuff being developed.
We're using TFS to ask for reviews, which actually works really well.
...what doesn't work is the junk I keep being sent....I bounced the same review 3 times yesterday because it didn't even compile...
When the stuff you get is decent reviews are ok....when it's junk, it's soul destroying
That's typically the case for us, although there's an occasional "before I finish it, what do you think of this direction?". Not a problem when rare and called out special.
I've experienced some really toxic cultures around code reviews. I believe the bottom line is teams need to set standards; linters and findbugs should be incoporated in the build and fail the build if formatting standards aren't met. Standards for level of abstraction should at least be discussed. Is using exceptions for control flow ever allowed? Requirements should not be defined in code reviews. I go so far to say that code beautification comments should be handled in separate tasks.
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.
> 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."
Your last paragraph resonates. Having some experience with this process, I'm convinced that a few years from now, mandatory code reviews will be the silver bullet of yesteryear. Ultimately, it's just another attempt at solving the unsolvable problem of "I hired lazy shitty engineers and I need a way to quality-proof their code." If you don't have this problem, don't bother with code reviews.
I was really hoping for more advice on letting the team members appreciate code reviews, and not take it too personally.
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 people taking things too personally (you or otherwise): talk to your team members. Let them know that you perceive their comments as harsh. If you can't come to a consensus about tone, content and voice, then code reviews will always be a friction point between you and your team. Work it out with them, they're all just people with emotions too, after all.
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.
>Does anyone have any practical advice on people understand the importance of structure code, and to limit the _personal factor_?
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.
>when it was time to review code, what we really looked out for were obvious bugs, problems with the design or architecture and where code should be located. Variable naming was inconsistent but no one really bothered to enforce them. I don’t blame them, making code review a nit-picky task really puts a damper on the mood.
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.
Nice summary! I had the problem of one team member who was underperforming and felt the need to use your (1)-(3) as a way to slow down and put down the work of others. Sure, this is a "people" problem and not a "code review" problem, but it seems that mandatory code reviews can give a lot of power to a few bad apples and can actually magnify people problems.
Good start and my best wishes. We follow these on stash using fork & pull request model, for a large project with 10 teams, each around 6-8 devs.
- everyone has a fork
- no one commits outside their fork
- each module has 3-4 reviewers
- good practices are constantly shared via sessions
- wiki drafted and updated
- comments are given reasoning with concepts, discussion threads and as well to other modules
- object calisthenics, if required
26 comments
[ 2.6 ms ] story [ 74.9 ms ] threadI 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.
If you have people that send you constantly completely broken code and refuse to get better, there's just not much you can do.
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.
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.
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.