4 ms·

I love code-reviews. With the right people, they can be useful both to achieve high-standards of code quality and also improve your own engineering process. The

by erwan 8y ago

I love code-reviews. With the right people, they can be useful both to achieve high-standards of code quality and also improve your own engineering process. The requirements are quite high, I think you need some combination of the following: 1. Both reviewer and reviewee are focusing on getting the best outcome possible, in good faith and with generosity. 2. The reviewer concedes that there can be equally valid approaches to a given problem or taste wrt. aesthetics. They do not try to gratuitously force their style upon the reviewee. The reciprocal must hold as well. 3.The reviewer makes practical suggestions and ideally accompany their comments with snippets of code. They involve themselves into being part of the solution. 4. The reviewer focuses on the substance of the pull request. That means putting in the time to understand the real logic, putting themselves in the shoes of the reviewee (with their help), and trying to challenge it so that the limitations of the approach are known and documented. 5. The reviewee makes an effort at slicing their request into readable and compact sub-PRs if needed. Similarly, the reviewer makes a best-effort attempt at starting their review within a reasonable time. Those are central aspects of a productive engineering culture anyway. You only need to find someone else that shares those "values" (i.e focusing on the best outcome possible as a sort of devotion to Engineering - so to speak) to grow this attitude within your company.

Vinnl 8y ago
I think in such a situation, receiving a review is great, even though the downsides mentioned in the article ("what am I going to do while I wait for the review to come in?") are still presents. However, it doesn't do away with how unsatisfying performing a review is. If I spend a day doing code reviews, that is not an enjoyable day to me. This is regardless of how useful I feel it is; it's just not a job I enjoy doing. It'd be nice if there was some change we could make to make it a more pleasant aspect of the job.
- cimmanom 8y ago
  While you wait for code review you work on another ticket. What kind of screwy engineering process assumes that code review is instantaneous and doesn’t give you something else to do in parallel?
  - Vinnl 8y ago
    Nobody expects code review to be instantaneous - that's the problem. Working on another ticket involves context switching, and context switching negatively affects the productivity. Sure, that downside is probably offset by the benefits of code reviews, but that was exactly what I said: receiving a review is beneficial, it's just a bummer that there's the downside that it's not instantaneous.
    - cimmanom 8y ago
      You're going to be context switching at the point you make the pull request anyway. And if you structure your day well, you can do the rest of the context switching at a point when it's not a burden anyway - such as making changes in response to code review first thing in the morning before returning to your other tickets.
      - Vinnl 8y ago
        Well, I guess we'll have to agree to disagree there. It's far easier to continue working on the same thing, even if I have to make a pull request (or simply have to write a commit) in between, as long as that's still related to the same change. If I have to switch between entirely different problems, that has more impact on my productivity than having to switch tasks while still working on the same problem. Again, in practice I still do actually switch to a different problem if that allows me to get my code reviewed. It's just that I wished it didn't come with the productivity hit that I experience.
        cimmanom 8y ago
        This still sounds to me like it might be a process issue around code review rather than code review actually being the problem. What else would you be doing on the same problem post-PR if you didn’t have to wait for code review?
        Vinnl 8y ago
        Otherwise I'd be working on the next step of the problem? Which I can still do but, as OP mentioned, does lead to more work if feedback comes in on the first part while I'm already far underway with the second part.
- bradleyjg 8y ago
  There are definite upsides to having a detailed written record of a code review, but in terms of enjoyment if I have some code to review I read though it, take some notes, grab a meeting room and have a discussion with the person that wrote the code. I find it far less tedious than having to write up my critiques in detail. Then the written review in the tool can just be terse reminders to the things we discussed. I'm sure there are people that feel exactly the opposite, but it's worth trying.
  - Vinnl 8y ago
    Hmm, that does sound like a good idea that would make it a lot more enjoyable, with the added bonus of being able to use tone of voice and facial expressions to prevent remarks from being taken the wrong way. I'm not currently in a position where I share an office with people whose code I review, but when I am again, I definitely want to try this. Thanks for sharing!
    - erwan 8y ago
      I want to second what OP said. A lot of the pain points people have from code review seem to come from limiting themselves to the review tool to understand the aim, scope, and rational behind some decisions. You should definitely use other channels (1-1 discussion, grabbing coffee, a meeting room, a call, whatever works).
falsedan 8y ago
Great points. Regarding point 2: > 2. The reviewer concedes that there can be equally valid approaches to a given problem or taste wrt. aesthetics. They do not try to gratuitously force their style upon the reviewee. The reciprocal must hold as well. If my code is being reviewed, and the reviewer has opinions about how they would do it, I like it when they share their preference but also 1. consider if the code as submitted has the same result 2. share their personal preference for how they would have written it 3. explain why they prefer it over the code I wrote 4. not block the review on me rewriting the code to match their preference I think it’s fine (and valuable) to comment “I hate this code, shipit” or (better) “ship it, if you change foo to bar then <explaination of why they prefer it>” The worst code review comments are “this isn’t performant/idiomatic/maintainable, fix it” or other statements of opinion presented as facts. This kind of faux-objective feedback is terrible, since a fact can be argued against (or could be wrong). A statement of opinion like “I don’t like it” or (best) “I had to think really hard before I understood it” is great, since that’s what someone diving into the code for a maintenance ticket/incident will be thinking but won’t have the original author on hand to walk them through the code. Linking to articles or references that explain why you prefer a particular implementation also gives the reviewer a learning opportunity & lets them save face when they post a follow up CR “thanks, didn’t know that”.
- pytester 8y ago
  >The worst code review comments are “this isn’t performant/idiomatic/maintainable, fix it” or other statements of opinion presented as facts. This behavior is usually symptomatic of an excessively dogmatic outlook. That is, I don't think it's the review style per se that's the problem - it's the person. I find developers like this nearly impossible to work with. They won't just unknowingly write poor code themselves (dogmatism typically leads to bad decisions which leads to really poor code) they will likely try and force you to do the same.
  - danmaz74 8y ago
    Even dogmatic people can learn not to be dogmatic.
- 8y ago
Sharlin 8y ago
Definitely. Those points seem fairly obvious to me, but maybe I've just worked with very professional people.
JoeAltmaier 8y ago
...and if any of those four are not in place, it can be a nightmare. At a rough guess, less than a 1-in-16 (2^4) chance of a non-aggravating code-review process at any given company.

by erwan 8y ago

Vinnl 8y ago

I think in such a situation, receiving a review is great, even though the downsides mentioned in the article ("what am I going to do while I wait for the review to come in?") are still presents. However, it doesn't do away with how unsatisfying performing a review is. If I spend a day doing code reviews, that is not an enjoyable day to me. This is regardless of how useful I feel it is; it's just not a job I enjoy doing. It'd be nice if there was some change we could make to make it a more pleasant aspect of the job.

cimmanom 8y ago
While you wait for code review you work on another ticket. What kind of screwy engineering process assumes that code review is instantaneous and doesn’t give you something else to do in parallel?
- Vinnl 8y ago
  Nobody expects code review to be instantaneous - that's the problem. Working on another ticket involves context switching, and context switching negatively affects the productivity. Sure, that downside is probably offset by the benefits of code reviews, but that was exactly what I said: receiving a review is beneficial, it's just a bummer that there's the downside that it's not instantaneous.
  - cimmanom 8y ago
    You're going to be context switching at the point you make the pull request anyway. And if you structure your day well, you can do the rest of the context switching at a point when it's not a burden anyway - such as making changes in response to code review first thing in the morning before returning to your other tickets.
    - Vinnl 8y ago
      Well, I guess we'll have to agree to disagree there. It's far easier to continue working on the same thing, even if I have to make a pull request (or simply have to write a commit) in between, as long as that's still related to the same change. If I have to switch between entirely different problems, that has more impact on my productivity than having to switch tasks while still working on the same problem. Again, in practice I still do actually switch to a different problem if that allows me to get my code reviewed. It's just that I wished it didn't come with the productivity hit that I experience.
      - cimmanom 8y ago
        This still sounds to me like it might be a process issue around code review rather than code review actually being the problem. What else would you be doing on the same problem post-PR if you didn’t have to wait for code review?
        Vinnl 8y ago
        Otherwise I'd be working on the next step of the problem? Which I can still do but, as OP mentioned, does lead to more work if feedback comes in on the first part while I'm already far underway with the second part.
bradleyjg 8y ago
There are definite upsides to having a detailed written record of a code review, but in terms of enjoyment if I have some code to review I read though it, take some notes, grab a meeting room and have a discussion with the person that wrote the code. I find it far less tedious than having to write up my critiques in detail. Then the written review in the tool can just be terse reminders to the things we discussed. I'm sure there are people that feel exactly the opposite, but it's worth trying.
- Vinnl 8y ago
  Hmm, that does sound like a good idea that would make it a lot more enjoyable, with the added bonus of being able to use tone of voice and facial expressions to prevent remarks from being taken the wrong way. I'm not currently in a position where I share an office with people whose code I review, but when I am again, I definitely want to try this. Thanks for sharing!
  - erwan 8y ago
    I want to second what OP said. A lot of the pain points people have from code review seem to come from limiting themselves to the review tool to understand the aim, scope, and rational behind some decisions. You should definitely use other channels (1-1 discussion, grabbing coffee, a meeting room, a call, whatever works).

falsedan 8y ago

Great points. Regarding point 2: > 2. The reviewer concedes that there can be equally valid approaches to a given problem or taste wrt. aesthetics. They do not try to gratuitously force their style upon the reviewee. The reciprocal must hold as well. If my code is being reviewed, and the reviewer has opinions about how they would do it, I like it when they share their preference but also 1. consider if the code as submitted has the same result 2. share their personal preference for how they would have written it 3. explain why they prefer it over the code I wrote 4. not block the review on me rewriting the code to match their preference I think it’s fine (and valuable) to comment “I hate this code, shipit” or (better) “ship it, if you change foo to bar then <explaination of why they prefer it>” The worst code review comments are “this isn’t performant/idiomatic/maintainable, fix it” or other statements of opinion presented as facts. This kind of faux-objective feedback is terrible, since a fact can be argued against (or could be wrong). A statement of opinion like “I don’t like it” or (best) “I had to think really hard before I understood it” is great, since that’s what someone diving into the code for a maintenance ticket/incident will be thinking but won’t have the original author on hand to walk them through the code. Linking to articles or references that explain why you prefer a particular implementation also gives the reviewer a learning opportunity & lets them save face when they post a follow up CR “thanks, didn’t know that”.

pytester 8y ago
>The worst code review comments are “this isn’t performant/idiomatic/maintainable, fix it” or other statements of opinion presented as facts. This behavior is usually symptomatic of an excessively dogmatic outlook. That is, I don't think it's the review style per se that's the problem - it's the person. I find developers like this nearly impossible to work with. They won't just unknowingly write poor code themselves (dogmatism typically leads to bad decisions which leads to really poor code) they will likely try and force you to do the same.
- danmaz74 8y ago
  Even dogmatic people can learn not to be dogmatic.
8y ago

Sharlin 8y ago

Definitely. Those points seem fairly obvious to me, but maybe I've just worked with very professional people.

JoeAltmaier 8y ago

...and if any of those four are not in place, it can be a nightmare. At a rough guess, less than a 1-in-16 (2^4) chance of a non-aggravating code-review process at any given company.