7 ms·

My biggest problem with code reviews isn't so much the phrasing(although it is definitely important), but the fact that they almost never has a "done" criterion

My biggest problem with code reviews isn't so much the phrasing(although it is definitely important), but the fact that they almost never has a "done" criterion. In my opinion, if new code has nothing objectively wrong with it(no security flaws, no unacceptable design patterns, no foreseen bugs, etc.), then the author has the prerogative to merge the code. Reviews shouldn't go on in perpetuity because different reviewers would all have written something differently than the author submitting the code. I've experienced this phenomenon at every company I've worked for at one point or another, and it's basically a way for Waterfall to sneak its way back in to an Agile process. (not that I'm an advocate of Agile) It doesn't happen too often, but I find it more frustrating than just about anything else. Beyond some reasonable adjustments to be made in the first wave or 2 of review comments, any other non-critical changes should be separated into tickets for the next sprint or simply be labeled as DIY or "do it yourself". If you want something done, but there's no actual standard for doing it on your team, then ordering people to write things your way via code review is really just a form of backseat coding. My code is never perfect, but that doesn't mean I want to change variable names, swap out libraries, or use different approaches merely because someone else prefers them that way. You think the variables should have different names? Wonderful! Go do it yourself. I'm not trying to be rude, but I've got other shit to do, and the software already works beautifully.

jedimastert 7y ago
> My biggest problem with code reviews isn't so much the phrasing(although it is definitely important), but the fact that they almost never has a "done" criterion. I've actually been experiencing this myself, as I've recently started a job with code reviews, and it's been driving me crazy. > In my opinion, if new code has nothing objectively wrong with it(no security flaws, no unacceptable design patterns, no foreseen bugs, etc.), then the author has the prerogative to merge the code. One of my biggest personal issues currently is that I have a fear of hitting the GO button, doing one more look-over just in case. I suppose it's just a lack of confidence in my own abilities, or maybe being anxious that my code isn't up to the standards of everyone else and the code base as a whole.
BurningFrog 7y ago
Code reviews can be much better, faster, and friendlier if done in person. It's also harder to ask people to spend half a day doing extra work on a whim in person.
- stevens32 7y ago
  IMO one of the great benefits of working physically near your coworkers is the ability to sit in a room and discuss things. It never made sense to me that PR's would happen mostly over the internet when the person who wrote the code might be sitting right next to you. After in-person discussion, comments could be dropped in the PR as a reminder.
fogetti 7y ago
I completely agree. Many times when someone asks me to change something without pointing to a previously agreed-on coding standard that all team members approved before, I just call it bullshit and let the PR sit there indefinitely while I get to my next task. If someone is interested why the PR is not released yet, I always point out that some people wanted to have changes but they were lazy to make those changes themselves even though the code base was accessible for the criticizer too. And I never fail to mention that I am already working on something else so going back is just the waste of my time and my time is as precious as the criticizer's. Of course I am not this arrogant when there are agreed-on coding standards and the reviewer points out mistakes based on those standards (but that almost never happens). I am more than happy to make changes in that case. So to sum it up: pull requests have become the prefect tool for workplace bullying.
- xpe 7y ago
  First, you have the choice to be arrogant or not arrogant. I would suggest that you’ll be more successful if you choose the latter. Second, not all team norms and guidelines can practically be discussed in advance. It is much better for a reviewer to give a sincere and detailed review than to leave out useful feedback.
  - fogetti 7y ago
    Well I can't even imagine a more arrogant thing than criticizing someone's work without the right to do so. Just because people have a keyboard it doesn't mean that they have all the fundamental skills and experience to give feedback on anything. > not all team norms and guidelines can practically be discussed in advance I would suggest that you’ll be more successful if you grow a spine and try to tackle the fundamental issue first: having norms and guidelines. If that's your attitude to programming your professionalism is very low. You ignored the most important thing in a successful development team's life and now you are suggesting other people how to become more successful? Ridiculous.
    - xpe 7y ago
      I was trying to constructively and civilly offer feedback. You did not reply in the same spirit. Your comment was rude; many people would ignore it. In the past (and occasionally even recently) I’ve been rude, arrogant, or not valued people properly. It didn’t get me very far. This is the reason I’m writing despite your tone —- because I want to help you see more broadly about what is important. If you disagree, fine, but open your mind to the possibility that you should revisit this topic later with more experience and perspective. Defensive, aggressive, and ad-hominem attacks are not useful and simply make you look worse. Being right is not sufficient. Life and work are bigger than that. If not me (someone you don’t know), I sincerely hope you ask several people you trust for feedback on this topic. I think you might be surprised by what you can learn. Your comment also makes unwarranted and illogical assumptions. For example, I never said that having norms and guidelines is a bad thing. I only said that there will inevitably be areas that are not covered in advance. Most importantly, you have a choice in how you act. Aspire to be both correct and kind. This will get you farther. Good luck.
joshuamorton 7y ago
Code working is only a first step to being good software. Software should be maintainable, and readable, clear, conformant code is necessary for future maintinability. Granted most of the style should be written down, but when I'm reviewing code, most of what I say is essentially "if you do this, it will be easier for others to maintain in the future." Code review is very bad at catching bugs (static analysis and tests do that). If all your code review does is attempt to catch bugs, you're just wasting time.
- ravenstine 7y ago
  Sure, but not everyone agrees on what's "readable" or "maintainable", and no organization has hashed out all the design patterns and coding style that it would deem appropriate. Even if there's a "better" way of doing something, not everyone might agree that it's worth the effort. A lot of "elegant" patterns are less readable. (e.g. object-oriented abstractions vs a functional approach) I'm not advocating for flimsy or convoluted code, yet I don't advocate for perfection. If a reasonable person can understand the code in front of them, and there's nothing objectively wrong with the code, then there should be a clear point where the review is considered done, the code is merged, and any further notes from the review are used to generate new tickets that may get handled by anyone on the team.
  - joshuamorton 7y ago
    > Sure, but not everyone agrees on what's "readable" or "maintainable", and no organization has hashed out all the design patterns and coding style that it would deem appropriate. Even if there's a "better" way of doing something, not everyone might agree that it's worth the effort. A lot of "elegant" patterns are less readable. (e.g. object-oriented abstractions vs a functional approach) This doesn't really matter. Yes, there will always be some level of disagreement. But you aren't writing code in isolation, and it is your job to write code that your team can maintain. If I, a reviewer (who is presumably well versed in the language in question) cannot understand what a block of code is doing without expending significant mental energy, that is a red flag. I'm talking about refactorings on the orders of 10s of lines of code, not changes to design patterns of architectures, those should be decided much earlier (and in general PRs should be small enough that you aren't forced to redo a design as part of a code review). > If a reasonable person can understand the code in front of them, and there's nothing objectively wrong with the code, And this is where we disagree. A reasonable person being able to understand the code is only the starting point. Code should only be merged when, to a good approximation, you're comfortable with anyone on your team (or not on your team, consider onboarding) being able to understand the code quickly.[0] One pet peeve of mine in this regard is loops that break or continue, especially those that are nested. They're often easy to write, and sure I can figure out what's going on eventually, but it's not easy to maintain, and there are straightforward, mostly mechanical changes that can make things easier to follow: a loop with a break is almost always an attempt to find something in a container. If you're continuing, it means you've got a function that finds and then does something with the found object. Split it in two. Have a function that loops through and returns the result, and then a second function that processes that result. A loop with a continue can almost always be refactored into a filter (in practice, I am not saying you should be using map/filter everywhere) by reversing the conditional. There are exceptions to this: early breaks/guards (sometimes), "returning" by reference in C, and sometimes building state machines can be done most clearly with continues. But those are fairly context specific (and they aren't the context I work in, for the most part), so can be called out explicitly. That's the kind of thing I look at. [0]: Another way of thinking about this or justifying this: If someone gets paged in the middle of the night because there is some production issue happening, and the trace happens to cross through this chunk of code, will it significantly delay this groggy person, who is neither you nor the author, and who has perhaps never looked at this code before, from quickly isolating the root cause of the issue?
arnvald 7y ago
While I do not know your exact situation, I find such attitude a bit selfish. Yes, you've got other shit to do, but guess what, writing good code is part of the shit to be done. I consider working code being a minimum standard. Code can be working but can still be bad. On top of correctness it should be easy to read and modify. And by telling your team go modify if themselves if they don't like it, you're kind of saying "if you expect me to do more than minimum, do it yourself". Again, it varies from team to team and you might have colleagues who focus on meaningless details instead of the essence. What I try to do in my teams is to encourage people to go together through changes suggested by the reviewer. If the sides agree that one version is objectively better than the other, go with that version. If there's no agreement or if the difference is too marginal, the author of the code makes the call. Also for me the review has easy "done" definition. It ends either in "code approved" or "changes requested" state. The first one leads to merge, the other one to further work or discussion on the code.
- ravenstine 7y ago
  Then we're both in agreement, because my point isn't that code shouldn't be subject to standards, but that a lot of feedback can end up being pedantic and that there needs to be a line drawn to prevent code reviews from turning into an endless cycle. If the difference is marginal, the author of the code should be able to move on to other things.
  - TheHegemon 7y ago
    The problem is that everyone seems to disagree on what pedantry is and how much marginal a difference actually is.
    - brigandish 7y ago
      Isn't that why we have specs, tests, and (explicit) coding standards? Once you met those then, yes, your code should go through.
skybrian 7y ago
Some people who are less in a hurry want this kind of feedback. (See sibling comment.) So you have to expect some negotiation over what kind of review you want. I think the key here is that it should be possible for reviews to mention improvements while not treating them as blocking the commit. When I was reviewing code I would often say this explicitly: "You don't have to do this, but..." Or, I would prefix some comments with "nit: ". If you're always pushing people to say less in code reviews and they listen, it can turn into a rubber-stamp process. Reviewers need positive feedback too!
- tomlagier 7y ago
  This is why I love code review tools with a "task" function (e.g. BitBucket). Adding a task blocks submission until the task is marked completed, but anyone can do that. So, you can approve a CL but add tasks for nits or non-interactive suggestions (things that can be done without needing a response from the reviewer). Then, the author can implement the tasks they find reasonable and ack the rest and not be blocked on submitting by a bunch of back-and-forth over style details.
- allannienhuis 7y ago
  I use the term ‘consider’ often in code reviews. “Consider using x...” conveys the idea of it being optional but worth their time to think about. I leave ’should’ for things worth arguing about. :)
Jach 7y ago
The "do it yourself" mentality needs buy-in at least from your team and a spirit of frequent refactoring otherwise you're being difficult... I think if you can get one reviewer to ok the merge, then merge it, fix followup issues subsequently (as new tickets you can pick up or someone else) but if you can't even get one other person to sign off... that's a problem if you then merge anyway. Ultimately what is merged without signoff can be reverted without signoff, but hey, you might at least get someone to take a look at it then. Just getting to code reviews at all is a huge win for a team, every process after that (including "definition of done" for reviews as well as tickets, merging without signoff (I do it for small trivial things) and the post's "be nicer" guidelines) should be discussed as a team in retros. Something that's worked for me (mostly on remote/distributed) for making sure reviews don't stay in limbo is as an author to be a bug and aggressively ask specific individuals for a review, even going so far as to schedule time to hop on a video screen sharing call if some background context and walkthrough is helpful. But the team needs to be accepting of this, otherwise again you're being difficult... At a team level another successful practice was for everyone to block off a convenient (not identical) chunk of time once a week to dedicate to making sure you have no pending reviews waiting on you, and also letting you say "please wait until [time] or see if someone else is free" when you're busy and the author bugs you. Sometimes a good review of a large change takes more than a focused hour or two -- it's fine to push back for smaller changes, but if you just block off some time, you can plan for it and just do the big review and then a limbo period will just be at most a week.
__turbobrew__ 7y ago
If you have to argue about naming conventions you are already doing it wrong. The style/conventions should be written down in a doc so they are objectively right/wrong -- no discussion.
- gwbas1c 7y ago
  You can't document common sense. Some things, like whitespace, braces, and naming conventions should be obvious. That's why the best rule of thumb is to stick to style, names, and conventions that are already in use. The review is to keep it consistent.
lolsal 7y ago
I have been experiencing a lot of what you are mentioning for years at the place I current work. The problem is that any code review feedback can be provided under the guise of 'quality' - once that Q-word gets into the discussion, it's very hard to argue that it doesn't matter or is not worth the time. Not that I don't want to write quality code, but nitpicks in the name of quality are the code review equivalent of a death by a thousand cuts.

jedimastert 7y ago

> My biggest problem with code reviews isn't so much the phrasing(although it is definitely important), but the fact that they almost never has a "done" criterion. I've actually been experiencing this myself, as I've recently started a job with code reviews, and it's been driving me crazy. > In my opinion, if new code has nothing objectively wrong with it(no security flaws, no unacceptable design patterns, no foreseen bugs, etc.), then the author has the prerogative to merge the code. One of my biggest personal issues currently is that I have a fear of hitting the GO button, doing one more look-over just in case. I suppose it's just a lack of confidence in my own abilities, or maybe being anxious that my code isn't up to the standards of everyone else and the code base as a whole.

BurningFrog 7y ago

Code reviews can be much better, faster, and friendlier if done in person. It's also harder to ask people to spend half a day doing extra work on a whim in person.

stevens32 7y ago
IMO one of the great benefits of working physically near your coworkers is the ability to sit in a room and discuss things. It never made sense to me that PR's would happen mostly over the internet when the person who wrote the code might be sitting right next to you. After in-person discussion, comments could be dropped in the PR as a reminder.

fogetti 7y ago

I completely agree. Many times when someone asks me to change something without pointing to a previously agreed-on coding standard that all team members approved before, I just call it bullshit and let the PR sit there indefinitely while I get to my next task. If someone is interested why the PR is not released yet, I always point out that some people wanted to have changes but they were lazy to make those changes themselves even though the code base was accessible for the criticizer too. And I never fail to mention that I am already working on something else so going back is just the waste of my time and my time is as precious as the criticizer's. Of course I am not this arrogant when there are agreed-on coding standards and the reviewer points out mistakes based on those standards (but that almost never happens). I am more than happy to make changes in that case. So to sum it up: pull requests have become the prefect tool for workplace bullying.

xpe 7y ago
First, you have the choice to be arrogant or not arrogant. I would suggest that you’ll be more successful if you choose the latter. Second, not all team norms and guidelines can practically be discussed in advance. It is much better for a reviewer to give a sincere and detailed review than to leave out useful feedback.
- fogetti 7y ago
  Well I can't even imagine a more arrogant thing than criticizing someone's work without the right to do so. Just because people have a keyboard it doesn't mean that they have all the fundamental skills and experience to give feedback on anything. > not all team norms and guidelines can practically be discussed in advance I would suggest that you’ll be more successful if you grow a spine and try to tackle the fundamental issue first: having norms and guidelines. If that's your attitude to programming your professionalism is very low. You ignored the most important thing in a successful development team's life and now you are suggesting other people how to become more successful? Ridiculous.
  - xpe 7y ago
    I was trying to constructively and civilly offer feedback. You did not reply in the same spirit. Your comment was rude; many people would ignore it. In the past (and occasionally even recently) I’ve been rude, arrogant, or not valued people properly. It didn’t get me very far. This is the reason I’m writing despite your tone —- because I want to help you see more broadly about what is important. If you disagree, fine, but open your mind to the possibility that you should revisit this topic later with more experience and perspective. Defensive, aggressive, and ad-hominem attacks are not useful and simply make you look worse. Being right is not sufficient. Life and work are bigger than that. If not me (someone you don’t know), I sincerely hope you ask several people you trust for feedback on this topic. I think you might be surprised by what you can learn. Your comment also makes unwarranted and illogical assumptions. For example, I never said that having norms and guidelines is a bad thing. I only said that there will inevitably be areas that are not covered in advance. Most importantly, you have a choice in how you act. Aspire to be both correct and kind. This will get you farther. Good luck.

joshuamorton 7y ago

Code working is only a first step to being good software. Software should be maintainable, and readable, clear, conformant code is necessary for future maintinability. Granted most of the style should be written down, but when I'm reviewing code, most of what I say is essentially "if you do this, it will be easier for others to maintain in the future." Code review is very bad at catching bugs (static analysis and tests do that). If all your code review does is attempt to catch bugs, you're just wasting time.

ravenstine 7y ago
Sure, but not everyone agrees on what's "readable" or "maintainable", and no organization has hashed out all the design patterns and coding style that it would deem appropriate. Even if there's a "better" way of doing something, not everyone might agree that it's worth the effort. A lot of "elegant" patterns are less readable. (e.g. object-oriented abstractions vs a functional approach) I'm not advocating for flimsy or convoluted code, yet I don't advocate for perfection. If a reasonable person can understand the code in front of them, and there's nothing objectively wrong with the code, then there should be a clear point where the review is considered done, the code is merged, and any further notes from the review are used to generate new tickets that may get handled by anyone on the team.
- joshuamorton 7y ago
  > Sure, but not everyone agrees on what's "readable" or "maintainable", and no organization has hashed out all the design patterns and coding style that it would deem appropriate. Even if there's a "better" way of doing something, not everyone might agree that it's worth the effort. A lot of "elegant" patterns are less readable. (e.g. object-oriented abstractions vs a functional approach) This doesn't really matter. Yes, there will always be some level of disagreement. But you aren't writing code in isolation, and it is your job to write code that your team can maintain. If I, a reviewer (who is presumably well versed in the language in question) cannot understand what a block of code is doing without expending significant mental energy, that is a red flag. I'm talking about refactorings on the orders of 10s of lines of code, not changes to design patterns of architectures, those should be decided much earlier (and in general PRs should be small enough that you aren't forced to redo a design as part of a code review). > If a reasonable person can understand the code in front of them, and there's nothing objectively wrong with the code, And this is where we disagree. A reasonable person being able to understand the code is only the starting point. Code should only be merged when, to a good approximation, you're comfortable with anyone on your team (or not on your team, consider onboarding) being able to understand the code quickly.[0] One pet peeve of mine in this regard is loops that break or continue, especially those that are nested. They're often easy to write, and sure I can figure out what's going on eventually, but it's not easy to maintain, and there are straightforward, mostly mechanical changes that can make things easier to follow: a loop with a break is almost always an attempt to find something in a container. If you're continuing, it means you've got a function that finds and then does something with the found object. Split it in two. Have a function that loops through and returns the result, and then a second function that processes that result. A loop with a continue can almost always be refactored into a filter (in practice, I am not saying you should be using map/filter everywhere) by reversing the conditional. There are exceptions to this: early breaks/guards (sometimes), "returning" by reference in C, and sometimes building state machines can be done most clearly with continues. But those are fairly context specific (and they aren't the context I work in, for the most part), so can be called out explicitly. That's the kind of thing I look at. [0]: Another way of thinking about this or justifying this: If someone gets paged in the middle of the night because there is some production issue happening, and the trace happens to cross through this chunk of code, will it significantly delay this groggy person, who is neither you nor the author, and who has perhaps never looked at this code before, from quickly isolating the root cause of the issue?

arnvald 7y ago

While I do not know your exact situation, I find such attitude a bit selfish. Yes, you've got other shit to do, but guess what, writing good code is part of the shit to be done. I consider working code being a minimum standard. Code can be working but can still be bad. On top of correctness it should be easy to read and modify. And by telling your team go modify if themselves if they don't like it, you're kind of saying "if you expect me to do more than minimum, do it yourself". Again, it varies from team to team and you might have colleagues who focus on meaningless details instead of the essence. What I try to do in my teams is to encourage people to go together through changes suggested by the reviewer. If the sides agree that one version is objectively better than the other, go with that version. If there's no agreement or if the difference is too marginal, the author of the code makes the call. Also for me the review has easy "done" definition. It ends either in "code approved" or "changes requested" state. The first one leads to merge, the other one to further work or discussion on the code.

ravenstine 7y ago
Then we're both in agreement, because my point isn't that code shouldn't be subject to standards, but that a lot of feedback can end up being pedantic and that there needs to be a line drawn to prevent code reviews from turning into an endless cycle. If the difference is marginal, the author of the code should be able to move on to other things.
- TheHegemon 7y ago
  The problem is that everyone seems to disagree on what pedantry is and how much marginal a difference actually is.
  - brigandish 7y ago
    Isn't that why we have specs, tests, and (explicit) coding standards? Once you met those then, yes, your code should go through.

skybrian 7y ago

Some people who are less in a hurry want this kind of feedback. (See sibling comment.) So you have to expect some negotiation over what kind of review you want. I think the key here is that it should be possible for reviews to mention improvements while not treating them as blocking the commit. When I was reviewing code I would often say this explicitly: "You don't have to do this, but..." Or, I would prefix some comments with "nit: ". If you're always pushing people to say less in code reviews and they listen, it can turn into a rubber-stamp process. Reviewers need positive feedback too!

tomlagier 7y ago
This is why I love code review tools with a "task" function (e.g. BitBucket). Adding a task blocks submission until the task is marked completed, but anyone can do that. So, you can approve a CL but add tasks for nits or non-interactive suggestions (things that can be done without needing a response from the reviewer). Then, the author can implement the tasks they find reasonable and ack the rest and not be blocked on submitting by a bunch of back-and-forth over style details.
allannienhuis 7y ago
I use the term ‘consider’ often in code reviews. “Consider using x...” conveys the idea of it being optional but worth their time to think about. I leave ’should’ for things worth arguing about. :)

Jach 7y ago

The "do it yourself" mentality needs buy-in at least from your team and a spirit of frequent refactoring otherwise you're being difficult... I think if you can get one reviewer to ok the merge, then merge it, fix followup issues subsequently (as new tickets you can pick up or someone else) but if you can't even get one other person to sign off... that's a problem if you then merge anyway. Ultimately what is merged without signoff can be reverted without signoff, but hey, you might at least get someone to take a look at it then. Just getting to code reviews at all is a huge win for a team, every process after that (including "definition of done" for reviews as well as tickets, merging without signoff (I do it for small trivial things) and the post's "be nicer" guidelines) should be discussed as a team in retros. Something that's worked for me (mostly on remote/distributed) for making sure reviews don't stay in limbo is as an author to be a bug and aggressively ask specific individuals for a review, even going so far as to schedule time to hop on a video screen sharing call if some background context and walkthrough is helpful. But the team needs to be accepting of this, otherwise again you're being difficult... At a team level another successful practice was for everyone to block off a convenient (not identical) chunk of time once a week to dedicate to making sure you have no pending reviews waiting on you, and also letting you say "please wait until [time] or see if someone else is free" when you're busy and the author bugs you. Sometimes a good review of a large change takes more than a focused hour or two -- it's fine to push back for smaller changes, but if you just block off some time, you can plan for it and just do the big review and then a limbo period will just be at most a week.

__turbobrew__ 7y ago

If you have to argue about naming conventions you are already doing it wrong. The style/conventions should be written down in a doc so they are objectively right/wrong -- no discussion.

gwbas1c 7y ago
You can't document common sense. Some things, like whitespace, braces, and naming conventions should be obvious. That's why the best rule of thumb is to stick to style, names, and conventions that are already in use. The review is to keep it consistent.

lolsal 7y ago

I have been experiencing a lot of what you are mentioning for years at the place I current work. The problem is that any code review feedback can be provided under the guise of 'quality' - once that Q-word gets into the discussion, it's very hard to argue that it doesn't matter or is not worth the time. Not that I don't want to write quality code, but nitpicks in the name of quality are the code review equivalent of a death by a thousand cuts.