4 ms·

> I've seen cases where people got hundreds of comments (many of them minor, nitpicky) from more experienced developers and were discouraged by the sheer number

> I've seen cases where people got hundreds of comments (many of them minor, nitpicky) from more experienced developers and were discouraged by the sheer number of them. That most new developers naturally suffer from imposter syndrome is not helped at all by 100% critical code reviews. This, combined with a large portion of developers lacking social empathy, poor communication skills, and (unfortunately) a desire to appear to the the smartest person in the room, can lead to severe demotivation and stress for both experienced and new team members.

hckr1292 7y ago
Social empathy is a really important skill, especially when delivering feedback. However, the best way to improve as a junior developer is to welcome feedback, not panic. I've seen junior developers welcome feedback or run from it, and I think that has as much to do with the reviewer as the author of the code. On the flip side, I've learned almost as much from reviewing more senior engineer's code and just asking dumb questions or looking up functions in documentation that I didn't know about. I think there's huge potential to learn as the code reviewer.
- siquick 7y ago
  I'm sure we've all worked in places where a senior would not like it if a junior reviewed their code, regardless of the juniors intent. But yes, always a tonne to learn. IMO the first few code reviews should be done face to face so some rapport can be built. Receiving feedback from a 'human' is far easier to process than a faceless Github profile picture.
  - dgellow 7y ago
    The face to face is definitely a good approach for first reviews. Also it’s worth starting with lower expectations and less critical feedbacks then increase over time. It takes time for people to feel comfortable receiving direct critics from persons they aren’t familiar to.
vonseel 7y ago
> hundreds of comments (many of them minor, nitpicky) This is one reason I liked Phabricator’s review system, which allowed drafting comments on an entire PR before submitting the comments. This allows you to be as nit-picky as you want when reading the PR, and then delete or modify any of them at the end. Instant submission of line-level comments, on the other hand... I think reviewers should have the awareness to understand not to over-burden the person whose code they are reviewing. For example, if I’m reading some very junior code and finding lots of issues, maybe I should delete all my comments about minor things like style and only leave the comments requesting major fixes like code design or potential bugs? When the developer re-submits the code after fixing the main issues, maybe it will be less frustrating to get a review back with “this looks great, can you just fix these small issues that don’t conform to our style guide and it’s good to go.”?
- pitaj 7y ago
  GitHub features the same draft system you described.
  - vonseel 7y ago
    I thought theirs was immediate, but I was referring mostly to Bitbucket Server's system from previous experience with that.
    - dgellow 7y ago
      GitHub has both. You can add an immediate comment, or add it to a draft that you post as one unit.
- tbodt 7y ago
  Critique and Gerrit (code review systems at Google) also do this.
rajangdavis 7y ago
100's of comments should raise a red flag. I think there are 1 of 3 scenarios at play: 1. The commit/change is too large. 2. The reviewer is nit-picky. 3. The person that wrote the code made a lot of mistakes. I really feel like if a reviewer is making 100 comments on a change, they're doing something wrong even if there are a lot of mistakes. That reviewer should really reach out to the person that wrote the code and talk to them human to human.
- LandR 7y ago
  As someone who did a code review for a colleague and left over 100 comments... I would say that leaving those comments is handy as it's a single place I can check to see what comments I left and the state of the fix, or if it's not fixed yet. If I just ended up talking to the guy, I'd have no record and he would have no record (unless he took notes) of all the places he has to fix. The reason for 100+ comments was a junior developers code and far too large a changeset.
  - rajangdavis 7y ago
    I feel as though leaving 100 comments only addresses the code but not the coder. Telling someone that the changes they made are too much for a commit would probably go a lot farther. Identifying common architectural problems would also reduce what you need to connunicate. Maybe they made a lot of mistakes but I would find it difficult to believe there were 100 unique mistakes.

hckr1292 7y ago

Social empathy is a really important skill, especially when delivering feedback. However, the best way to improve as a junior developer is to welcome feedback, not panic. I've seen junior developers welcome feedback or run from it, and I think that has as much to do with the reviewer as the author of the code. On the flip side, I've learned almost as much from reviewing more senior engineer's code and just asking dumb questions or looking up functions in documentation that I didn't know about. I think there's huge potential to learn as the code reviewer.

siquick 7y ago
I'm sure we've all worked in places where a senior would not like it if a junior reviewed their code, regardless of the juniors intent. But yes, always a tonne to learn. IMO the first few code reviews should be done face to face so some rapport can be built. Receiving feedback from a 'human' is far easier to process than a faceless Github profile picture.
- dgellow 7y ago
  The face to face is definitely a good approach for first reviews. Also it’s worth starting with lower expectations and less critical feedbacks then increase over time. It takes time for people to feel comfortable receiving direct critics from persons they aren’t familiar to.

vonseel 7y ago

> hundreds of comments (many of them minor, nitpicky) This is one reason I liked Phabricator’s review system, which allowed drafting comments on an entire PR before submitting the comments. This allows you to be as nit-picky as you want when reading the PR, and then delete or modify any of them at the end. Instant submission of line-level comments, on the other hand... I think reviewers should have the awareness to understand not to over-burden the person whose code they are reviewing. For example, if I’m reading some very junior code and finding lots of issues, maybe I should delete all my comments about minor things like style and only leave the comments requesting major fixes like code design or potential bugs? When the developer re-submits the code after fixing the main issues, maybe it will be less frustrating to get a review back with “this looks great, can you just fix these small issues that don’t conform to our style guide and it’s good to go.”?

pitaj 7y ago
GitHub features the same draft system you described.
- vonseel 7y ago
  I thought theirs was immediate, but I was referring mostly to Bitbucket Server's system from previous experience with that.
  - dgellow 7y ago
    GitHub has both. You can add an immediate comment, or add it to a draft that you post as one unit.
tbodt 7y ago
Critique and Gerrit (code review systems at Google) also do this.

rajangdavis 7y ago

100's of comments should raise a red flag. I think there are 1 of 3 scenarios at play: 1. The commit/change is too large. 2. The reviewer is nit-picky. 3. The person that wrote the code made a lot of mistakes. I really feel like if a reviewer is making 100 comments on a change, they're doing something wrong even if there are a lot of mistakes. That reviewer should really reach out to the person that wrote the code and talk to them human to human.

LandR 7y ago
As someone who did a code review for a colleague and left over 100 comments... I would say that leaving those comments is handy as it's a single place I can check to see what comments I left and the state of the fix, or if it's not fixed yet. If I just ended up talking to the guy, I'd have no record and he would have no record (unless he took notes) of all the places he has to fix. The reason for 100+ comments was a junior developers code and far too large a changeset.
- rajangdavis 7y ago
  I feel as though leaving 100 comments only addresses the code but not the coder. Telling someone that the changes they made are too much for a commit would probably go a lot farther. Identifying common architectural problems would also reduce what you need to connunicate. Maybe they made a lot of mistakes but I would find it difficult to believe there were 100 unique mistakes.