This is great advice and isn't followed often enough, especially when reviewing code written by people new to an organization/team:
> If you see something nice in the CL, tell the developer, especially when they addressed one of your comments in a great way. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well. It’s sometimes even more valuable, in terms of mentoring, to tell a developer what they did right than to tell them what they did wrong.
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.
I once worked on a team that specialized in very long littanies of code review comments... but they were able to bake this into their culture in fundamental ways such that it ended up being one of the most positive experiences in my software engineering career.
The basics of how they accomplished this was:
- The obvious- no personal / destructive attacks or insults, no cussing, no comments on any person's abilities.
- Having your code picked apart is a badge of honor, and you were expected / required to do the same to the most senior of your teammates when they submit code.
- Collective ownership- it's never "your" code, it's the team's code.
- Constant acknowledgement that our system is hard and complex, and it is really important that it works as expected / promised.
That last part was an ingredient I never found anywhere else. The opposite seems to be more common- every other team I've worked on tended to underestimate or even trivialize the difficulty and complexity of the systems they work on. By acknowledging that this work is hard for everyone involved, and that despite this, it must be well-done and function properly, the code review and the ensuing discussions became a very welcome and encouraged part of this process. It also helped defeat newcomers' imposter syndrome because this mentality was effective at making everyone feel like they had an important role to play, and that even the most senior folks often felt like a noob when they screwed up.
Sure, but “Hey I really appreciated how you did this thing here because it’s tidy / does thing X really well / takes into account future whatever” never hurts to throw in either!
I found that really informal comments cut less deep 'hey man this needs double checking, read up on $x and then reconsider this block' (gender aside) is far better received than 'This is missing fundamental concepts around $x read up on them then rewrite this block'
> 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.
> 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.”?
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.
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.
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.
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.
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.
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.
Want to know a great way to handle a subset of these? Good automated code linters. Google has a ton of formatters and linters that either auto correct or suggest changes. To paraphrase a smarter Googler than me: People tend to just do what the bots say and don't care too much about. The best way to impose your code ideas on others is via a linter.
Automated tools here have also drastically reduced the number of comments I get on CLs at Google, as people don't bicker about the little things as much anymore.
Something I've started doing that I picked up was prefixing my nitpicky comments with "Nit: ..." so that it's clear that certain comments are just minor suggestions, not that anything is necessarily wrong.
I'm usually okay with preemptively accepting code with only nit comments too just to signal that those comments are not too big of an issue (if at all).
My favorite way to do that has been to indicate “Not a blocker” to differentiate nits that I’m picking from things that would make me not approve the review.
Agreed. I find this extra important when working remotely or with other people who work remotely. If you’re not getting any face-time, critique has to be balanced with positivity. In my experience it’s true even with very experienced team members, and even with the very serious & stodgy people who avoid chit-chat and claim to not be bothered by feelings & opinions.
Why is it that people feel so discouraged by loads of review, especially early on? I always had a good bit of imposter syndrome early on, but never considered quitting. I always assumed that you have a lot to learn, that it’s expected you’re going to suck at some level.
People have their own internal stories about how good they are, and all that feedback can be very painful.
It doesn't help that many comments are nitpicks and pedantry, made by people without much social empathy.
This is especially true when sending a patch for a high-level review of your proof of concept, and the next thing you know people are complaining about your formatting.
It can be hard to review badly formatted code. The reviewer has to put in effort to understand your change, but they have to put in more effort to parse the change if the formatting is bad.
"Why is this function's return value not being checked? Oh, turns out this person used a yoda conditional even though the rest of the code doesn't do that."
"Why is this block of code running even when the condition is false? Oh, turns out the else branch already ended and this part of the code is just indented wrong."
And so on. Even something as minor as `foo ()` vs `foo()` can stand out and act as a constant stream of mental speedbumps.
Claiming your change is high-level / only looking for feedback to the overall design doesn't change that. You're asking for a code review because you want the reviewer to read your code, but reading it is exactly the part that they're finding hard to do.
And it's not excusable, but the reviewer might be insulted that the reviewee is wasting their time and their feedback may be ruder / snappier as a result. After all, the reviewee could easily have put in the effort to run the auto-formatter, follow the existing code's style, etc.
Prose editors have various levels of edits, edits for content, edits for organization, and copy editing and finally proofreading.
Yes, one should avoid misspelling words when you are about to submit a manuscript for content, but wordsmithing all the the sentences is a mistake.
Likewise, one should run an auto formatter before sending a proof of concept off for review, but a reviewer who is all tied in knots about low-level nits when looking at a general proof of concept is editing at the wrong level.
Otherwise you end up with a fully-baked, carefully written solution that satisfies all the nits, but was a giant waste of time because it takes the wrong overall approach.
The formatting & style could be automated via IDE configurations, so that when the code is submitted for review it is already clean. This will make reviewer to focus on the design/logic instead of formatting.
The best critical code reviews I've had focused on the design and completely ignored things like formatting.
On balance, formatting is an important part of code health. Hopefully, using auto-formating tools (gofmt and friends) should hopefully make code formatting issues not take up time in code review.
Thanks For The Feedback[1] is a pretty decent book on the subject. Highly recommended if you're interested both in the mechanisms and improving how you receive/give feedback.
Everyone is different, and some people have a harder time with critical feedback than others. It could be that they have different past experiences, or that they just have different personalities.
>Why is it that people feel so discouraged by loads of review, especially early on?
Many people often think their skills and actions are them, instead of things they've acquired.
The question you're asking is oddly similar to asking why people get stressed and mentally suffer at all. It's a deep and complex subject. Taking is personally only scratches the surface.
Eg, I know someone who switched her career over it. I asked her why she left and she told me they kept criticizing her, and she took it as if they were attacking her and trying to get her to quit.
I can offer a personal anecdote here on my experience with code reviews, on both sides.
I was the primary architect and reviewer for a complex real-time mathematical application. When reviewing code, I was pretty much a tyrant: the code had to be correct, well tested, conform to the theory, interface with the rest of the system correctly, etc., in order to be allowed in. I remember leaving some pretty brutal reviews when the proposed design was different than what I thought it needed to be. I thought I was doing the right thing, the project lead thought I was doing the right thing, but maybe I was just making my teammate's lives hell.
In a subsequent job, I was on the receiving end: I was again the primary architect and maintainer, but still needed to seek review from a larger team (my component was part of a larger project they owned). The experience was not enjoyable: reviews sometimes took months (I sure wish I was kidding), sometimes were passthrough "LGTM! I don't understand it at all!", sometimes asking questions like "why is this mutex here" and then I have to spend 3 hours writing up an explanation for how threads and locks work in this case. I found that my mental model shifted: instead of committing small improvements here and there like cleaning up comments or renaming something I just... didn't. I didn't want to deal with a multi-day process of bugging someone to review (they were always busy), dealing with the roulette wheel of comments that might come up, the possibility that I might have to justify some minor thing that I don't even remember the reasoning for. It felt like making a PR opened you up to an uncomfortably invasive inspection, one where the reviewers look down their nose at you and ask you to elucidate why you chose to wear the red shirt today instead of the blue one, as if you're supposed to have some grand unified theory of shirt colors when the actual reasoning is "I thought red would work and it did". How are you supposed to justify why you didn't do all the things you didn't do?
I think an issue is that there's always a different approach that could be used and in a perfect world perhaps we'd iterate endlessly until we found the best one. I've seen plenty of systems that have a design very different than what I think I would do, but as it turns out those systems work too.
I've honestly become less convinced that code reviews are the answer. Is there a possibility for learning reviewer <-> submitter? Of course. Do some teams find code reviews to be hugely beneficial? I would assume so. But I don't know if an organization-wide mandatory absolutely-zero-exceptions is the way to go.
I share this sentiment about code reviews. Too often, they are time consuming discussion over trivialities and/or taste. Also, like you said, it's way to easy for someone to ask one-liner questions which require long and time-consuming explanations. I would be fine with doing code reviews with people who I consider reasonable (ex. I hand-picked them for a team). Otherwise, it is often a drag which doesn't improve the product that much.
At work, and especially when you are new you are at the mercy of others in several ways to an extent that usually eclipses almost anything we expose ourselves to in our lives.
Reviewing can thus be experienced as opening up yourself to critique by essentially anonymous "kingmakers", which at any time can make you look like a clown, if they want to. This experience might not be rational, but I believe it's still extremely common, as an experience.
It's made worse by the fact, that if the review culture is anything else but great, it's not at all unlikely to try and take advantage of that - in one of many ways - in order to feel a bit better about themselves.
At our core most of us are social creatures, and thus most of us easily pick up on that danger/risk, including most that are otherwise not socially adept, as they have usually suffered bullying of some kind and have learned to avoid those exposed situations.
I never considered quitting, but it still feels bad. Putting your code out there for the first time feels vulnerable (sort of similar to publishing your writing or public speaking) and it feels bad to get shit on.
When you've been working for a while you can filter out the noise regarding formatting and other smaller issues (or, ideally, get autoformatting set up), and it gets easier to separate your ego from your code. When you're new though, and showing someone else your code for the first time it's hard to see a huge volume of what seems like substantial negative feedback on your code (and by extension yourself).
I realize the "right" solution here is that people shouldn't equate criticism of their code with criticism of themselves, but I think that's a huge thing to expect at first and contributes to making the field unwelcoming.
Your lead should really be setting you up for success on your first PR. I always made sure I paired with my new employees before they hit submit on their first few PRs. It is much easier to be empathetic when you are next to a person. It would also give me time to deep time into certain aspects of our codebase that I felt they could handle in future sprints.
There are often multiple ways to make a make a skinny cat, with no particular way better in all respects than any other method. Programmers end up designing code based on their own mental model, held in their head at that time, and would not really be able to explain that model to someone else. That someone else in turn has a different mental model of what is the correct way. Both ways are correct, just one of them is not what the reviewer would have done.
When the reviewer communicates this to the coder, this often comes across as criticism of code that is in reality perfectly fine.
> 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.
Unfortunately many people stink at communication. The best thing a reviewer can do during a code review is ask questions. A reviewer who comes in and just says 'this is wrong' misses an opportunity to either learn themselves or actually teach the submitter. A much better approach is to ask the submitter why they did something certain way. What were their thoughts, and what did they see.
IME, asking questions leads to much better outcomes. Sometimes the submitter will just admit they were not thinking about anything and see their own errors. Upside, they found the issue themselves. Other times they'll explain some complexity or use case only they could see while deep in the code. Upside, I just learned something.
In general the best teachers ask questions. It doesn't put people's egos on the defensive, and tends to help people learn more effectively (the goal if any review IMO). I've seen this work on me in other scenarios like Jiu-Jitsu. My teacher will ask mid-roll or afterwards why I did something, no matter how stupid it was. Explaining my thought process helps me to better recognize the error, and I'm not immediately on the defensive. I'm then much more receptive when the teacher goes "that wasn't a terrible idea, but if you did this...".
Agreed. You don’t need fake praise but you should know whether something you did is just adequate or really good. You rarely have discussions about why something is good.
At Amazon, all code is reviewed. Comments are constructive and straight to the point. Standard courtesy applies, but anything more than that is left out. Unless there's something extremely interesting, overly cautious phrasing and "positive" comments are mostly noise. Also, people tend to specify when comments are nits.
Why the scare quotes around "positive"? I have lots of positive thoughts when doing code reviews. I don't write them into comments all that often, but sometimes I do. "Oh I didn't know about this API, nice find!" "Ah, nice approach, this is a big improvement." "Thanks for improving the test coverage!" Maybe this seems like unactionable "noise", but it isn't, it encourages future actions of the kind being positively reinforced. Plus it's free, it takes no time to write and no time to read comments like this, all it does is make the author's experience of reading the review a bit more pleasant. This is all upside, no downside. It would be crazy to discourage such a thing.
Full disclosure, I work at Google, and I actually find positive comments mostly rare, but code review culture is very different team to team, so I can hardly speak for the entire company.
Personally I try to make them often, but I also try to be thoughtful about making them as to not come off patronizing like you mentioned.
Though I’ve never felt like a positive comment ever made me feel patronized in any situation, I think a good way to not come off patronizing is to to make the comment more personal than general, ie:
“This is a cool way to use the rest operator (or whatever thing), Ima steal this”.
Obviously it needs to be an authentic comment, but I think it’s a significant difference vs
“Great use of the rest operator!”
When the interpretation on the other end might be “oh so this person is surprised I know these basic language constructs”.
I learn things from doing code reviews all the time, and I like letting my coworkers know that I feel like I’ve gotten better at my job as a result of reviewing their contributions.
I think it’d be a shame if I didn’t let them know.
Agree, I love to point out a nice approach or a TIL, and I like that Google put it down in writing. If Amazon really considers it noise, it is very descriptive of their culture.
> Unless there's something extremely interesting, overly cautious phrasing and "positive" comments are mostly noise.
Not really. Positive comments help teach engineers which things they've done that conform to local best practices (and why) without them having to meticulously dig those up (assuming they're even documented). A lack of positive comments leaves engineers to learn them only by running afoul of them. Effectively it provides direction only when some threshold of badness is crossed, while leaving positive comments on good code (especially for new team members or junior engineers) provides a beacon pointing away from the badness threshold entirely.
Put differently: commenting on good code makes for swifter and less eventful code reviews by steering engineers away from the bad practices that make code challenging to review in the first place.
Also, it's just nicer to spend forty hours a week with people who demonstrably appreciate their peers' good work.
A code review is not the time and place to report positive or convoluted comments. A code review is to find issues and to report them clearly.
The only positive thing to report should thus be that all is fine if the reviewer did not find any issues.
Now, if you are conducting the review in a meeting then obviously you can make oral comments in passing. If you're using a software tool for reviews then the all the comments should be on point. Nothing prevent you from talking to your coworker afterwards to spread some love if you want to.
Because a code review is not to pat each other on the back, it is to inspect and report issues. It is already costly enough without going off topic.
As said, if you want to praise then you are free to do it offline.
This is a professional procedure in a professional setting, not warm words from an encouraging teacher at school... I don't want to have to go through comments that do not add any value to the exercise of finding issues, and I have never seen people leave such comments in 20 years.
> This is a professional procedure in a professional setting, not warm words from an encouraging teacher at school...
Citing a good practice in someone's code as "yes, please do more of this" alongside "don't do this please" is not, in my opinion, fluff.
> I don't want to have to go through comments that do not add any value to the exercise of finding issues, and I have never seen people leave such comments in 20 years.
> Citing a good practice in someone's code as "yes, please do more of this"
That simply isn't the purpose of a code review.
Good practices should be documented externally, so you can check them consistently during code review ;) It's also quite useful to have a checklist when doing a code review.
I disagree, with a caveat. Assuming your review tool supports "Resolved" or "No Action Required" comments (ours does), it's rather easy to distinguish something that is informative from something that is actionable. I am now recalling that some review tools don't distinguish open/unresolved comments from resolved or informative comments, which would make this more of a trade-off than an obvious win.
Depends on level of seniority and amount of time the person has been on the team. Positive comments to a senior engineer about using a hash map instead of doing a huge linear scan are just noise.
Nurses don’t compliment each other for using a new pair of gloves on each patient.
If this is representative of the Amazon engineering culture, I'm gonna add this to the (already overflowing) pile of reasons I would never work there.
Feeling valued is linked to job performance. And if I notice any of a) a generally interesting piece of code b) the engineer being a boy scout and fixing something in that particular module to make his changelist better c) a novel/comprehensive way of testing the code automatically d) elbow grease to just go the extra mile in terms of doing a great job (without adding unnecessary complexity) I will call it out in the code review along with the regular 'fix this' feedback. As a principal engineer my word carries some amount of weight and I truly want the person to feel good about their work when they deserve it.
Another thing I often do is add a 'thank you for taking the time to do this' whenever someone slightly decreases the amount of tech debt (by refactoring, or removing code/complexity) when it was obvious it wasn't absolutely necessary to get their work item completed. Basically whenever someone shows they're thinking strategically rather than tactical about their work, I want to make sure that person knows that I noticed and that I value that.
People aren't robots, we're all professionals and we all like to feel good about our work.
I did a pretty reasonable chunk of C++ code once years ago. I was junior level. Wanted a good review.
I got 21 comments to remove blank lines and none about the code. Thanks. Really useful. This was the team of "experts" on the code base.
Then a principle engineer reviewed some other code and oh look, actual useful comments.
Nitpicking can be worse than useless. Obfuscates or ignore real issues in the code. Seems to usually be a sypmtom of the reviewing not being competent enough to actually review the code properly.
> If you see something nice in the CL, tell the developer, especially when they addressed one of your comments in a great way. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well. It’s sometimes even more valuable, in terms of mentoring, to tell a developer what they did right than to tell them what they did wrong.
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.