How to Make Your Code Reviewer Fall in Love with You

I honestly love that Go has a formatter included, and the style is specified. Similarly with black[0] for Python. I used to care terribly about style, until I realized I just care about consistency and readability. Otherwise I don't give a shit, and having a tool available to everyone makes such a difference. This goes for other tools too, like static checkers, linters, even CI. I've been on projects where anyone could run the whole CI pipeline (which included the aforementioned static checkers, unit tests, even a doxygen run to check for undocumented things) on their dev machine before submitting, and not waste anyone's time with things better done by a computer. Wish every project could be like that.

[0] - https://black.readthedocs.io/en/stable/

I have a rule that if stylistic decisions matter enough to reject a PR over, they also have to be enforced by a linter or code formatter. If they are that big a deal, we should enforce them via automation, not review. And if they're not that big a deal, do not put them in a code review.

> Nothing takes my motivation for contributing away as a reviewer that just must find something to complain about the code.

Or the reviewer that always asks for more tests. (Once I got asked to write a test that was unnecessary and almost impossible to write a test for. Think timing issues. My options were to push back or spend a significant amount of time writing a test that was not needed. As a junior that already pushed back more than a typical junior I made the best choice I thought I could make at the time.)

How I managed this on a team:

If there is a style disagreement, documented style rules prevail. If there is a documented style rule, follow it. If there is not a documented style rule, you cannot block this PR with the style objection.

This avoids really annoying issues of "I don't like how this is written [for style reasons]" in PR feedback. I'm not a mind reader, I don't whether you prefer for..in over Array.foreach or whatever, my PR shouldn't be blocked because of that. If you want new rule, go propose one and we can discuss & vote on it later but it doesn't apply to this PR.

This avoids the issue of "style rules are determined by whoever has the greatest capacity for argument & is willing to block PRs over it." It goes to a discussion & vote and everyone can agree once then stop talking about it. :)

Exactly. Nothing I love more than working hard to make sure my PR meets the personal style of the colleague whom I expect will review it, only to have to have it reviewed by another colleague, with a mutually exclusive style, meaning I have to do it all over again, and then that reviewer goes on vacation and I get grilled by the expected reviewer on why I did it "that way". Sigh.

Yeah... I have a lot of that discussion with my team, too.

It seems people (reviewers) need to find something or they think they are not doing good job.

Another irritating practice is treating opinions about style as commands -- basically commanding the author to change the style of the code to conform to the reviewer.

It should be understood that every problem has many potentially correct solutions. The job of the reviewer isn't to assure the best correct solution or the solution they like, it is to verify that the one selected by author is in the set of correct solutions. If this makes sense...

My last boss had a problem with this. Guy literally told me to just accept any change suggested to ease the review process and it's not worth debating any comments. We had very different opinions.

He always claimed he was "not nit-picky", but then he let reviews turn into exactly what you said, while loops versus for loops and what-not, and allowed the rest of the team to comment on low-value things like style rather than answering the important questions.

Why pushback at using a built-in function that does the same thing as some little helper your team wrote 8 years ago, when your helper function doesn't even handle special cases? There's literally no point other than someone has NIH syndrome. Why are the source files all 5,000 lines long and not broken into a logical project structure? Why is the back-end data-gathering and transforming code inter-mangled with the front-end templates (Django) so far as to be written as template filters and template functions?

It did not take me long to realize the development culture there was not aligned with my values. The lead developers came up in a different world and refused to see the positives and benefits of any modern development workflow involving a separate back-end API and front-end application.

I mean, consistent code style is just good practice, so maybe that's not a great example.

I was going to say something similar.

When I'm the reviewer, I try to decided for myself whether something is truly important, or just a style choice. If it's the latter, I will comment on it, but then add "Your call." to make it clear that I expect ties to go to the author.

> agree that person putting work into it should be able to decide their own code.

I take the opposite of this approach. If my reviewer felt it was worth their time to put in the work to provide feedback, then I accept their opinion. 99.99% pr feedback is not worth escalating to a ping-pong or face to face meeting.

This is one spot where I strongly prefer the standard from the Google guidelines[1]. "Reviewers should not require the author to polish every tiny piece of a CL before granting approval. Rather, the reviewer should balance out the need to make forward progress compared to the importance of the changes they are suggesting." Getting caught up in all the little details rarely serves this purpose; more likely it's being done in the service of something anti-productive like a personality conflict.

[1]: https://google.github.io/eng-practices/review/reviewer/stand...

My preferred approach is: the ties are awarded to the individual whose name will show up in `git blame` after this change is merged.

Agree; rule #1 of reviewing in my opinion is "trust your colleague whose code you are reviewing".

I'm curious if a separate "design review" phase could have helped with your challenges with the team halfway around the world?

I worked at one company where, due to the nature of the business, you really couldn't reasonably expect other teams to do code review in a timely manner. But most inter-team code review challenges had to do with high-level design, interface protocols, stuff like that. And they had to do with tasks that we could usually see coming down the pipe weeks ahead of time. So we developed a culture of just finding a time that worked for everyone to call a meeting with one or two stakeholders from each affected team, in order to hash out those design decisions before starting on the actual implementation work. It would stay pretty high-level - if any concrete work product came out of it, it would be an ADR or two.

We did code review, too, of course. But agreeing on a plan ahead of time greatly reduced the risk of code review turning up problems that led to costly rework.

those are fascinating, occasionally horrifying cross team examples you give. i suppose i’m spoiled to work in a small company where reviews are always local.

Before/after screenshots help too, if there's a visual change.

While true, sitting down and reading your code in the review tool as opposed to your editor or git client does help; shifting your mindset from author to reviewer also helps.

Back when I was still in a project that did code reviews, it really helped for me.

Similarly / a tip: `git add -p`, to view and review your changes one at a time. You can split up changes into separate commits as well using that technique.

i will often read through my changes in the browser UI in a similar manner as my reviewers. more often than not, that change of perspective will highlight something silly i missed.

i realize now that i could help my reviewers by first submitting a PR to myself into the branch to be merged.

Naturally; if self-review were as effective as peer-review, we need not do the latter!

The implication here seems to be that one is only likely to be able to see 'trivial' mistakes in one's own work; that, unfortunately, tends to be a self-fulfilling prophesy, as, if you are focusing on issues like syntax, you are probably not giving so much attention to more analytic issues such as what implicit assumptions need to hold for the code to work. One thing, that is not uncommon in my experience, is for the early part of your work to be predicated on assumptions about what the as-yet unwritten parts will need or do, and some of these assumptions turn out to be wrong. There is some hope that, on revisiting the early work with an analytic eye, after completion if the remainder, you will see this to be so.

There are a couple of things that helped me improve how well I review my own work, both mentioned in the article: take a break between writing and reviewing, and don't make excuses (to yourself or publicly) or otherwise minimize problems found by the reviewer (the same goes for anything you found in testing.)

I've developed the habit of trying to look at my code (or UI) as if I had never seen it before. You can think of it as an empathy exercise.

One way is imagining you want to explain it to a coworker or new hire.

I find it enormously useful, and it has become second nature for me.

Ideally you want tooling that allows you to toggle between the diff between 2 and 3 and HEAD and 3, and to specify that 2 is the parent of 3.

Some folks use "stacked diffs" or "stacked commits" to do this. Others use feature branches, or a patch series.

To add to the sibling comments, you should avoid this scenario in the first place by hiding things behind feature flags that only apply in dev.

Request 1 can go in fine on its own and reviewed separately

Request 2 can be added feature flagged.

Requests 3 and 4 would remove the respective flags for the scaffolding.

This is a hard problem and I usually try to avoid it, but when I have to do it I usually have them branch off each other so that I can merge the PRs in reverse order once they're approved (4 -> 3 -> 2 -> 1 -> main). The diffs are all based off the previous branch in the "stack" so they aren't duplicated.

It's really easy to mess up and merge the wrong PR first, and it's a pain in the ass to rebase all the way back up if you make a change in Request 1. I'd love to know how other people are handling this.

Create request-1 targeting the default branch, request-2 targeting request-1, etc. Once request-1 is merged, I'd change the target of request-2 to the default branch and merge the branch back. This is under the assumption that e.g. you can feature flag the feature away, so that e.g. the scaffolded app doesn't show in production yet. An alternative would be to have a "temporary" feature branch that you target until you feel it's ready to be merged into the default branch.

By (temporarly) targeting the prev branch problem (3) is solved in most UIs.

The communication problem (2) is simply done by: This adds X and is scoped to X, look at request-Y to see how feature Y builds on it.

> Do you add all 4 Request reviews at once, or do you wait until the previous one has been approved before adding the next one?

This is the tricky bit, in your example I wouldn't go so far and create all MRs. I'd create one MR ahead at maximum, because if you need to change something based on the review that influences a later feature, you might end up doing work twice.

So maybe a timeline would look like this:

- (Skipping a dependency MR, because I feel one should introduce dependency when it is used and not beforehand)

- request-2: Scaffold application / feature, create an MR targeting default branch (request-2)

- request-3: Start working on feature A, once feature A is 80% done => Create an MR targeting request-1

- Ask for review on request-2 with sharing a link to request-3

- Continue working on request-3 while you wait on request-2 review

- Incorporate request-2 review feedback, get it merged

- Rebase request-3 on merged request-2 (or merge default branch back), resolve conflicts and put it into review

- Start working on request-4 based on the current request-2 branch.

- Rinse and repeat.

There are obvious intricacies, e.g. if legal needs to review dependencies, review times of your reviewers. I generally try to work with the same set of reviewers over a sensible time frame, rather than getting new ones for every MR relating to the same feature set. So check beforehand whether they are available for the foreseeable future.

4 CRs stacked on eachother in Gerrit. It's that one killer feature of Gerrit that you cannot live without in other systems once you realize how powerful it is. Phabricator's Differential also does this, but it's comparatively extremely janky.

You literally just push four stacked commits to refs/for/master, they end up as 4 separate Change Requests (equivalent to what others call Pull Requests, each with a separate review process), each one explicitly marked as being dependent on another, and showing a diff only against that particular other base CR as if it were merged onto the target branch already.

Practically this just means you push your current HEAD to refs/for/master, Gerrit will take care of automatically creating all CRs for you and print out their URLs. Done. Ready for review.

Then, to update any of them, just amend them locally (address code reviews, rebase stack on top of current master/main, ...), make sure they're still stacked on eachother in Git (as four commits), and push them all again to the same endpoint. Gerrit will automatically update all affected CRs. It will deduce the intended order and state of CRs based on the local Git history pushed, mapping all commits to existing CRs based on the Change-Id present in each commit (automatically generated by Gerrit's local git hook).

None of the hacks that other siblings comments mention are required. It just works.

Use branches. Branches are cheap in Git, and you can do PRs between branches so that the reviewers are only seeing the new changes in that branch.

In your example, I'd create a branch for R1, and publish a PR for R1 -> mainline when it's ready. Then I create a branch for R2 from the R1 branch, using R1 as the upstream.

When R2 is ready, I publish a PR for R2 -> R1, and create a branch for R3 to start working on it.

When there is feedback for R1, I switch to that branch and make those changes. I'll squash those changes back into the original commit(s) of R1, and update the PR.

Then you rebase R2 with the changes you made from R1, rebase R3 with the changes from R2, etc.

As long as the changes between each branch are not on the same lines of code, the rebases are painless and quick.

I name the branches with a key to support ordering, something like:

1_add_dependencies 2_add_scaffolding 3_feature_A 4_feature_B

This way they show up sorted when you list branches, and it's obvious what their dependency ordering is.

Love the concept of reviewing your own code first. I’ve been doing that for the longest now and usually find something I missed when reviewing my own code. I think I have taken this for granted and assumed everyone does this.

Hey, meta comment since I found this post first through your meta post / tweets about trying to get this to the front page of HN.

It's great content, and exactly the kind of things I want to read here, but wondered if you had thoughts about how to solve the Goodhart's law[0] effect. e.g. in an ideal world for me (and I guess for you too), the best posts would more naturally rise to the top of places like HN that ostensibly act as quality filters.

I really like your posting advice in general as most of it is about 'how to improve quality', but some of it (choosing what time to post at etc) falls into 'how to improve my chances at this gambling like system'.

I have mixed feelings about this. Having a higher barrier to entry does lead to higher quality posts (and in one possible future, only people who have read the 'how to get to front page of HN' guide can get to the front page of HN). On the other hand, I've seen the problem of 'should we invest this dollar into producing a better quality Widget or into telling people more effectively that our Widgets are the best?' kill entire organizations when the see-saw tilts too much to the latter.

Maybe I'm relating some things too broadly and it's actually nothing more than 'hey I post high quality content and do things to help it succeed', but wondering if you've thought about this tradeoff at all / if you're worried about getting deeper into the metrics rabbit hole that you've discussed in some of your other posts.

[0] https://en.wikipedia.org/wiki/Goodhart%27s_law

Can you write a counter article?

Unfortunately, I'm incredibly handsome, charismatic, and just generally awesome, and as a result this keeps happening to me.

There's always lots of drama when I just want to remain co-workers, and I've had to move on often.

Help appreciated.

Wanted to add one thing I noticed after I've been on the reviewing side for a while. Add a section in the description of the MR describing how the changes were tested. If this is detailed enough and covers all the edge cases I worry about, sometimes I don't even need to look at the code to be comfortable with approving the change. Conversely, it's frustrating when I get a MR with a blank or generic description without any notes on purpose, approach, how it was tested, etc.

"fall in love with you" is catchy but not the right interpersonal dynamic. Something like "love reviewing your code" is better.

This is an excellent article, making several points that I would not have thought of if I were to write on the same topic (point 11, for instance.)

I would, however, like to make a point about commenting, as there are at least three related cases where renaming identifiers and restructuring logic is not sufficient. The first thing is where one is doing something non-obvious on account of an unintuitive (and possibly undocumented) fact about either the problem domain or some remote part of the the system that it is infeasible to change; the second is some non-obvious corner-case (a subtle issue of thread- or exception-safety, for example); and the third is where the algorithm itself is non-obvious (Jon Bentley wrote of a colleage who found that one of his algorithms had beem 'fixed' and 'improved' to the point where it was no longer efficient; a suitable comment might have avoided that (and there's also the famous /* You are not expected to understand this */, for which no mere restructuring or renaming seems to offer an effective alternative.))

I am sure you are aware of these exceptions, but not everyone on the comment-deprecating bandwagon seem to do so.

https://en.wikipedia.org/wiki/Lions%27_Commentary_on_UNIX_6t...

First I just wanted to say I loved the doggo and the general tone of the post.

Also wanted to say that there's a lot of parallel between code reviews and "work review" in Investment Banking. Tooling aside, the underlying concept behind your tips is likely true for a variety of sectors, so I do wonder if one could dig a level deeper and find that essential truth in how to properly manage write-review workflows. Just some food for thought in case you want to jump into that rabbit hole.

Ugh. Stopped reading after this bit:

  They’re going to fall in love with you. Deal with it.
  Nobody ever complained on their deathbed that too many
  people fell in love with them.

That's just so outright dumb + wrong. Assumed the rest of the article would be of similar low quality, so left.

This is good guidance to junior devs. I see these issues frequently from the reviewer side.

The title is super creepy though, so is the claim that "your reviewer will literally fall in love with you". I review code from my team every day, including code written by young engineers, male and female. I can't point them to this.

This is a big part of why I've started pushing for aggressive autoformatting and linting. The automated tools and checks are sometimes annoying, but automating away checks for the low-value stuff like this helps encourage the human reviewers to review for actual bugs and design problems rather than just taking potshots at the easy stuff.

This is possibly the worst advice I've ever encountered about how to run a functional software team. It sounds like you have worked with a succession of terrible people if this is your impression of code review or team communication, so I can certainly understand that would have an effect on your ideas.

I've encountered this sort of team before – the high-functioning jealously-protected team that's isolated from outside interaction and visibility. They usually produce absolutely wonderful high-quality software that is useless and meets none of the requirements that other teams have of them.

In fact, I'd say that well-functioning teams will usually themselves develop a system of code reviews or similar, along with using task-tracking software, regularly seeking influences from the outside stakeholders, and making sure work is visible to other teams. A good manager supports those interactions and helps to make sure everyone has the resources they need to work effectively.

Agree on (2), but I respectfully disagree with (1).

It adds a lot of friction for the author to have to make 10 separate commits to address 10 comments. I also don't want to review 10 separate commits.

The majority of my comments are on separate chunks of code, so it's not that hard for me to see the resolutions to all of them in one big diff.

Edit: I'm assuming the commits will be squashed at the end of the review. If you're preserving the full commit history, then I can understand why you'd want the atomic commits for each note addressed.

Do you make an additional commit for each review comment? As opposed to just amending the old commit. Why?

tbh sounds to me as if you review way to large changes if there's 10 comments requesting changes that are not clearly attached to a small code location (and thus can be reviewed through a line/section specific comment, and thus easy to see the individual change)

I agree tooling should be improved, but the change should be legible in more than one tool. Chances are good that someone will end up looking at the diff in the future in their favorite IDE, on the command line, or in who knows what else tool.

The simplest and clearest solution is to split out the changes in separate commits.

Better tooling is definitely feasible, but confounding formatting and semantic changes also messes up blame for the affected lines and often makes a line's history misleading.

It depends how aggressively your reformatter changes your code. For example, Prettier will take this JavaScript line:

  document.addEventListener('keydown', e => { console.log(e) });

and convert it to this, which changes both whitespace and syntax:

  document.addEventListener("keydown", (e) => {
    console.log(e);
  });

I don't think there are any diff tools that can hide the differences there, even though they're superficial.

[0] https://prettier.io/

This can apply beyond a white space change, but rather larger non-functional changes as well. Something like moving a function up a layer to make it available elsewhere before using it elsewhere. It this point, the "automatic detection tool" is whether or not the current tests fail without having to change them.

> Why is it hard to review something that changes both formatting and the code?

The opposite problem is also a tooling problem: Why is it hard to save your changes to code, but not the formatting.

Unnecessary refactoring and whitespace changes are hard fails for my code reviews. There are rare exceptions where refactoring is essentially intertwined with the change being performed, but they are rare.

I need to be able to see the intent and minimal definition of a change. Anything that obscures that is obnoxious not just to the reviewer but everyone after who might have to merge or understand the change. Perhaps it is because I manage code that is "high risk" and hence "I can't understand your change" equates to "it can't go into production" makes a difference.

Apart from that, it also ruins the chance to cherry pick / separately merge parts of the change.

To me this is pure laziness and selfishness and lack of discipline on the developers part.

I've toyed with this idea too, but for a slightly different purpose.

When junior developers join my team, I find that they're often shy about critiquing anyone else's code because they assume they're the least skilled person, so they couldn't possibly find anything to be improved in anyone else's code.

I've considered sending developers like this a code review and including the note, "And just so you feel comfortable suggesting improvements, I've intentionally left in one mistake (not necessarily a functional bug)."

I've never done it because I worry it comes across as condescending. Like, "Obviously I'm perfect, but to give you something to do, I added a mistake, and now it's your job to go digging around for it."

Instead, I try to reassure junior developers that they should feel comfortable making suggestions and asking questions in the code review, even if they're not finding things that are officially "bugs."

I'm not sure this is controversial so much as it is just terrible advice!

Are you really suggesting that developers purposely introduce bugs into a code base just to keep reviewers on their toes?

It may seem like a relatively harmless idea in the small, but in the large, imagine hundreds or thousands of developers taking this advice, and purposely committing bugs. Yikes.

> For some reason, it feels really unsatisfying to a lot of people to conduct an extensive code review, and come up with exactly zero items to change.

You should fix that instead of trying to hack around this. Code review shouldn't be a nitpicking party for people that need to drum up their egos.

Feels condescendingly manipulative ("I know your ego/self-esteem are low, let me throw you a bone"), and would just be more work for everyone; for them to articulate the problem they find, and regardless of whether they catch it or not, you have to now remember to go back and fix it before merging.

I can only speak from using GitHub (which I'm not overall very happy with anyway), but PRs on my team can recommend review either "by file" or "by commit", with a very strong preference for the latter.

"By file" means that the reviewer should look at the diff between the branch base and tip, ignoring the intermediate changes along the way. If the individual commits really don't matter, "by file" suggests that we ought to squash this branch into a single commit; and if they do matter, something prevented us from providing a proper, clean narrative. "By file" is a smell.

"By commit" is what the article assumes. Rather than look at the sum total of changes, you look at what each commit individually changed.

In GitHub, "by file" corresponds to reviewing by the "Files changed" tab, while "by commit" corresponds to reviewing by the "Commits" tab (and going through each commit in order).

At the command-line, "by file" corresponds to `git diff develop..feature`, and "by commit" corresponds to `git show --reverse develop..feature` (which shows all selected commits in order from least to most recent).

> Given that I don't control the tools used, how do I ensure that functional changes aren't lost amidst formatting changes?

Given the above, you can and should isolate those kinds of changes into separate commits, so that the functional changes are the only visible changes in their respective commits.

Thanks! I work with an illustrator named Loraine Yow, who's fantastic.[0] I wrote a blog post a few years ago explaining how I found her and the process we use to create the illustrations together.[1]

[0] https://www.lolo-ology.com/

[1] https://mtlynch.io/how-to-hire-a-cartoonist/

At a more agile org, getting 2-3 stories done in one day isn't too extraordinary. A PR that follows some of the rules from the OP (especially 5/6/7) might only take 5-10 minutes to review completely.

If you can't slow down enough to do that, or your stories are so large they require more attention, that might be a red flag situation? Writing code faster than you can review it will eventually catch up to the team/org.

If you are expected to add 2-3 features daily, first, look for a new job.

If they stick to it though, you're basically forced to reduce the feature's scope to the bare minimum. So instead of a "implement this form", you'd have tasks in the form of "implement this form field".

I don't know what situation you're in, but to me it sounds like they're steering towards giving little commitment and cheating the system to meet some arbitrary counter-productive productivity goal.

Also only works when a team has a network of trust.

Every company I have worked for in my 20 plus years has been a lead/architect/cto/boss and everyone reporting to that single person. Reviews when done take many weeks.