24 ms·

How to Make Your Code Reviewer Fall in Love with You

472 points by kiyanwang 6y ago

anotherhue 6y ago
This is marvelous
mtlynch 6y ago
Author here. Happy to answer any questions or take any feedback about this post.
- whoisjohnkid 6y ago
  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.
  - mtlynch 6y ago
    Thanks for reading! Yeah, I had the same assumption for years. And then I started to realize that people would submit PRs to me where there's a ton of debug logging code left in or git merge markers they accidentally didn't fix. I thought, "Wait, are some authors not even reading this before they send it out?"
- sixhobbits 6y ago
  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 https://en.wikipedia.org/wiki/Goodhart%27s_law
  - mtlynch 6y ago
    Hey Gareth, good question! It's unusual for me to try to "time" my submissions to Hacker News. When I published this post, I thought it would be interesting to live tweet my publication day process,[0] so I published sooner than I originally planned because it was a slow news day on HN. But it ended up not gaining traction on HN that day anyway, so even that optimization seemed to have no effect. But overall, I think the HN filter works pretty well. I can't think of anything I do to get an edge aside from investing a lot more time into editing and illustrations than I think most bloggers do, but that would be the filter working as intended. I do worry about taking scores and readership metrics too personally. When I published my first code review article back in 2017,[1] it flopped on Hacker News and on reddit, the two places I was hoping it would find readers. I felt awful because I spent more time on it than I'd ever spent on a blog post before. I took a break from watching the metrics and went for a run, and that cleared my head. I decided that I felt good about the post regardless of what response it got, and I need to have that attitude on future posts. When I got back from my run, it had taken off on reddit, which made me excited but also kind of interfered with the lesson I was hoping to learn. But I do try to view my articles mainly in terms of how good I feel about them and maybe 20% what kind of response they get. [0] https://twitter.com/deliberatecoder/status/1334156012132118530 https://twitter.com/deliberatecoder/status/13341560121321185... [1] https://mtlynch.io/human-code-reviews-1/ https://mtlynch.io/human-code-reviews-1/
    - sixhobbits 6y ago
      Thanks for the response! I made that 2017 post compulsory reading for a bunch of code reviewers I helped train if that counts for anything :)
- bbarnett 6y ago
  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.
  - qualityonly 6y ago
    Wear a wedding ring? I had a similar thing in my early 20s despite the wedding ring, now in my 30s everyone looks for it.
- foobarian 6y ago
  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.
  - mtlynch 6y ago
    Oh, that's interesting. I'd really only want to do that for things that are hard to test automatically. I have 100x more confidence in a test where I can see the source for it and watch it run in CI. But I agree that for exceptional cases where it's hard to write test automation, it's a good idea to explain how the author verified the changes.
- alisonkisk 6y ago
  "fall in love with you" is catchy but not the right interpersonal dynamic. Something like "love reviewing your code" is better.
  - JoeAltmaier 6y ago
    Sure but that wouldn't get on the 1st page, right?
  - nullsense 6y ago
    The one weird trick that will make your coworkers fall in love with your Pull Requests.
- mannykannot 6y ago
  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_6th_Edition,_with_Source_Code#%22You_are_not_expected_to_understand_this%22 https://en.wikipedia.org/wiki/Lions%27_Commentary_on_UNIX_6t...
  - mtlynch 6y ago
    Thanks for reading! I absolutely agree with all of the exceptions you listed. I'm totally in support of comments when they're used judiciously. To add a bit of context, a tendency I've noticed when I encourage people to answer questions with the code is that they think that means "add a comment in case anyone else has this question," when really what I want them to think is, "rewrite the logic so nobody else has this question. If that's not possible, add a comment." In other words, a comment should be the last resort for explaining code rather than the first line of defense.
- airstrike 6y ago
  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.
  - mtlynch 6y ago
    Thanks for reading! That's interesting about "work review" in Investment Banking. I've never heard that term and Google isn't helping me out much. Do you have a recommendation on where I could learn more? I have noticed that things like this aren't too far removed from other fields. When I wrote my article about reviewing code from the reviewer's side[0], I got feedback that most of the techniques apply to giving feedback in other domains. And while I was writing my guide to hiring writers,[1] I realized that the process is 80% identical to my process for hiring developers and graphic designers. The thing that's tough about going more abstract is that it becomes hard to convince any particular group that the content is worth reading. Focusing on code review appeals to HN because a large percentage of the audience is developers, but if it was a more general guide to improving review workflows, I'd have a harder time convincing HN readers that it was relevant to them. [0] https://mtlynch.io/human-code-reviews-1/ https://mtlynch.io/human-code-reviews-1/ [1] https://mtlynch.io/hiring-content-writers/ https://mtlynch.io/hiring-content-writers/
- justinclift 6y ago
  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.
  - mtlynch 6y ago
    That was in jest. I assumed readers would understand that nobody's going to fall in love over proper code review technique.
- protastus 6y ago
  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.
lllr_finger 6y ago
This is great! Many of these things are slowly learned over time, but explicitly listing them out will be helpful when refreshing or teaching others. The three that have been most helpful on our team are: #1 (review your own code first), #2 (write a changelist description), and #5/#6 (narrowly scope changes/separate functional/non-functional changes).
kowlo 6y ago
Enjoyed the illustrations
- mtlynch 6y ago
  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/ https://www.lolo-ology.com/ [1] https://mtlynch.io/how-to-hire-a-cartoonist/ https://mtlynch.io/how-to-hire-a-cartoonist/
  - kowlo 6y ago
    I didn't recognise your name - it just so happens the only other post of yours I've read is [1] https://mtlynch.io/how-to-hire-a-cartoonist/ https://mtlynch.io/how-to-hire-a-cartoonist/!
itsmemattchung 6y ago
Nice post. I agree with almost all of the author's points. However, I would add that "reviewing one's own code" is more difficult than it may appears. It's like reviewing your own prose, one develops blind spots. That being said, combing through your own CR for trivial mistakes is just due diligence. Also, 100% on the change list description. I mentioned the same thing in a similar post[1] of mine. [1] https://blog.mattchung.me/2020/11/09/3-tips-on-getting-eye-balls-on-your-code-review/ https://blog.mattchung.me/2020/11/09/3-tips-on-getting-eye-b...
- Cthulhu_ 6y ago
  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.
  - mumblemumble 6y ago
    This is what I do. Even that small change in perspective is often enough for me to catch quite a few stylistic or functional problems two which I was completely oblivious when looking at the code in the IDE. Even in the IDE's own diff viewer.
  - syncsynchalt 6y ago
    Hard agree. I spot things in `git add -p` that I had missed when writing the code in my IDE. Then I spot more things in the github "Create PR" page that I missed in the previous step. Then sometimes I spot more things in the created PR that I missed in the steps above. Always skim back over your PRs, especially in the final context that they'll be reviewed in. Something about it puts you into "reviewer" mode and you'll get a better PR out of it.
    - angrais 6y ago
      What you've described indicates that while you may not agree, reviewing your own code helps identify issues.
cmwelsh 6y ago
I thought this was a dupe but actually this received avid discussion on reddit 5 days ago: https://www.reddit.com/r/programming/comments/k5cbln/how_to_make_your_code_reviewer_fall_in_love_with/ https://www.reddit.com/r/programming/comments/k5cbln/how_to_...
PunchTornado 6y ago
this only works when the company actually cares and allocates time to improve code quality. Not workable in companies where you are expected to add 2-3 features daily.
- lllr_finger 6y ago
  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.
  - alisonkisk 6y ago
    Maybe at a tiny startup org that doesn't have many users or sensitive data or money.
- Cthulhu_ 6y ago
  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.
- treespace88 6y ago
  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.
jakevoytko 6y ago
This is an amazing list. I just want to point out that there's an implicit requirement in this list, which is that this is optimized for reviewers who are willing to give you quick feedback. At a previous job, I worked on a team where we iterated towards the list that OP provides. It worked really well, and it worked because we had a culture of providing rapid reviews. The senior people on our team even thought that reviews were important enough to drop their own work. A lot about the team was suboptimal, but I was spoiled by getting such rapid feedback for my work. We had some partner teams that really challenged this. One was halfway around the world, so you were guaranteed to only get one review round per day. If you wanted to make progress while working with them, you needed to either send them huge changes or send them a bunch of changes at once. But structural feedback in an early change would cause you to need to rewrite the later changes. We broke this cycle by having them eventually give local reviewers stronger conditional approval in the integration path, which fixed our particular issue but showed that there were cracks in how we optimized our code review flow. The real trouble came from a local team, which was an overloaded platform team that we were built on. No matter what you did, they would only review your changes about once per week. Their feedback was super insightful, too, and they often caught very subtle misuses of their platform that would cause subtle user-visible bugs. You couldn't just skip their feedback. To try to manage their flow, they wanted teams to review pull requests locally before sending changes to them. So people on my team would write a change for this platform, which we'd review on our team. Our team would apply our rules and break the change up. We'd send the first one out. Then it would take a full month to get the entire thing in, to account for the back-and-forth. If you needed to get something substantial in, it could take months. My manager fought for months for them to acknowledge that this was a problem and fix it, and they did eventually dig out of it by setting SLAs and increasing capacity. I don't want to take away from this list, but I want to point out that some of it is a function of your reviewer's incentives. If your reviewers have different incentives, then you need to find your own list that works within those bounds.
- mumblemumble 6y ago
  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.
jsnell 6y ago
Great list, but I'd argue that point 6 is more often indicative of tooling failure. Why is it hard to review something that changes both formatting and the code? In the example it is because the tooling is visualizing both kinds of changes in the same way. But there is no fundamental reason for why that needs to be the case. The same applies to refactoring. It seems perfectly doable for a code review tool to detect code motion, and have differet treatment of the code that moved unchanged vs. the parts that were modified.
- pimlottc 6y ago
  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.
- piotrkaminski 6y ago
  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.
  - dpcx 6y ago
    The problem I've found with this statement is that then no one ever changes formatting for fear of "messing up blame for the affected lines," and then no reformatting ever gets done. At some point, take the leap and do it (assuming you're not completely changing the formatting of the entire file/project/what-have-you).
  - jsnell 6y ago
    Blame is also a tool. If a reasonable change messes up blame, that's a tooling problem :) For example git-blame can ignore whitespace, detect code movement inside a file, and even code movement across files. There's no reason why that has to be the limit of how smart blame can be.
- mtlynch 6y ago
qualityonly 6y ago
How can you tell someone is wrong in Programming? Wrong outputs are checked by tests and you could do time studies. However that's after-the-fact. With abstraction there isn't a correct answer from code alone (outside possibly nested loops)
Igelau 6y ago
Coming to Netflix, Christmas 2021
modernerd 6y ago
Also: Add a “How to Test” heading with steps to your PR templates. This is not a replacement for automated tests or intended to shortcut a thorough review — it just saves a lot of time to give reviewers a list of setup and testing steps for anything they need to verify manually.
- mLuby 6y ago
  Before/after screenshots help too, if there's a visual change.
known 6y ago
https://en.wikipedia.org/wiki/Extreme_programming https://en.wikipedia.org/wiki/Extreme_programming helps
q3k 6y ago
Good post. For a longer, more prose-like read that raises the general same points and expand upon them, see Google's 'How to do a code review' [1]. [1] - https://google.github.io/eng-practices/review/reviewer/ https://google.github.io/eng-practices/review/reviewer/
perlgeek 6y ago
A bit of a controversial one: give your reviewer one obvious (but minor) mistake to catch. 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. This is likely more relevant when you present something to a business leader who has the feeling they have to contribute something, but it can also apply to inexperienced code reviewers.
- mtlynch 6y ago
  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."
  - alisonkisk 6y ago
    Just put the mistake in quietly, and if they miss it, notice it yourself later or ask someone else to comment on it
  - Hello71 6y ago
    I think a change of framing might be helpful here. Put in multiple bugs, and call it "training" instead. If you have multiple junior developers, have them all do it and compare results, reinforcing the idea of multiple reviews.
  - guytv 6y ago
    This is a very interesting idea to me. Over the years I noticed that reviewing the same code knowing a bug is there will increase my chances of finding it comparing to not knowing a bug exists. It would be interesting to measure if introducing a "catch" in every PR would increase the number of times the reviewer will find additional issues.
hashkb 6y ago
Just complain to management that your reviewer is being mean and that you need to get this feature deployed right away for growth. Promise to monitor closely in production and go home.
stiray 6y ago
Well, as much I have seen code reviews in my life (ignoring a few people that have OCD), I have noticed that the trend is to: - add whatever comment that comes to your mind and you know wont be argued about (like using spaces instead of tabs) to have your persona look better on management statistics - not deal with actually understanding the code but rather argue about syntax (dont malloc, use vector.reserve(),...) - dont press accept until few lines are exchanged... again, boosting the statistics instead of giving something meaningful - ... whatever boosts the reviewer visibility, even going ballistic on irrelevant parts of code (while no one reviews 3rd party dependencies where most of the evil is) My proposal for code quality is different. Put together a team that communicates (a rare skill nowdays) and LET THEM communicate, offer them free beer afterwork, whatever interests them. Dont use enforced meetings like scrum etc., they just show that you have failed putting together a functional team. Make developers friends instead of rivals. Then avoid JIRA overhead, avoid management visibility,... just listen what people are saying (and preferably become their friend too), surpres any outside world influeces from your team and fire anyone trying to ladder over anyone else. Then you wont need code reviews. And you will always have a long list of improvements at any moment. Just send an email and ask. Be capable of coding. Be really good at coding. Everyone will follow you after "shit hit the fan" and team had to work over weekend and you have stayed with them and help them fix the bugs, analyze the code. DONT be a manager, be helpful part of the team (if this would be industry standard we can just fire 99.999% of managers).
- mumblemumble 6y ago
  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.
- matthewmacleod 6y ago
  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.
nullandvoid 6y ago
A couple from my pet peeve list - 1) ATOMIC COMMITS Nothing worse than a single commit, to address 10 separate comments. I don't want to have to go back, and cross reference my comments with your wall of changes 2) DO NOT SELF RESOLVE A COMMENT Wait for the reviewer to checkout your solution, I want to learn through your fix, aswell as make sure it's what I expected.
- mtlynch 6y ago
  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.
  - nullandvoid 6y ago
    No problem I understand, it took me a while before I ended up sticking with (1). I still think it's much easier for the author to make those changes, and attach a commit ID to the relevant comment, than it is the other way around. As the article states, respect the code reviewers time. If i've put in some effort to point out issues, and you don't even spend the effort to address each comment individually, then you are not respecting the reviewers time. Maybe there are some cases you are right, but as a general rule i'll just always follow (1) as an author, as it honestly doesn't cost me much time, and makes my reviewers life easier.
- colonwqbang 6y ago
  Do you make an additional commit for each review comment? As opposed to just amending the old commit. Why?
  - nullandvoid 6y ago
    You can squash into a single commit at the end Creating a fresh commit for each comment, provides me an easily clickable diff link
deleted 6y ago
[deleted]
falcor84 6y ago
Great post. This is the part I liked best: >When your reviewer makes a suggestion, and you each have roughly equal evidence to support your position, defer to your reviewer. Between the two of you, they have a better perspective on what it’s like to read this code fresh.
ivanstojic 6y ago
The title of the article makes it seem like having someone fall in love with you is a desirable outcome. > your reviewer will literally fall in love with you No, author, no they will not. They might appreciate my professionalism and focus on correctness if I follow this advice though.
aequitas 6y ago
> The golden rule: value your reviewer’s time This goes the other way around too. I've had some PR's that put copious amounts of time in to make them as complete and perfect as I could for the reviewer. Only to get them turned down for style reasons. If style is that important, make sure you document the rules or automate it (3. Automate the easy stuff). So contributors have a chance at figuring this out for themselves, without having to go back and forth. If the style comes down to personal preference you can apply rule 12. Award all ties to your reviewer, in reverse too. Nothing takes my motivation for contributing away as a reviewer that just must find something to complain about the code.
- npsimons 6y ago
  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/ https://black.readthedocs.io/en/stable/
  - aequitas 6y ago
    > 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. I had the same thing. Also consistency and automation is what's really important, as it extinguishes every need for style discussions. Because you can trivially convert from the project style to personal styles and vice versa using things like Git attributes[0], as long as it is consistent. [0] https://git-scm.com/docs/gitattributes https://git-scm.com/docs/gitattributes
    - 6y ago
radihuq 6y ago
How do you guys handle managing multiple merge/pull requests that depend on the previous one? For example: Request 1: install dependencies Request 2: add scaffolding to application like adding a menu item, page layout, etc Request 3: add feature A Request 4: add feature B My questions: 1. 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? 2. How do you communicate Request 2 is dependent on Request 1? 3. How do you enable the reviewer to review Request 4 without reviewing duplicate code (since Request 4 has merged Request 1, 2, and 3)?
- cbhl 6y ago
  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.
- onorton 6y ago
  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.
  - nullsense 6y ago
    What about structural stuff that just can't be hidden behind a feature flag? What do people do in those scenarios?
- eat_veggies 6y ago
  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.
ensiferum 6y ago
- Anyone focusing on style issues in a code review is doing it wrong and really just wasting each others time. - self documenting code isn't possible because the code cannot possibly document what isn't there. In other words sometimes the issue important details are in what the code isn't doing (such as not setting some state or not making that function call under some circumstances etc.
pwinnski 6y ago
I find myself nodding in agreement with points 6 and 7 (about mixed and large changelists), but unsure of how to implement the suggested fix. Given that I don't control the tools used, how do I ensure that functional changes aren't lost amidst formatting changes? I would love for BitBucket to ignore formatting changes in my PRs, but alas, no.
- Twisol 6y ago
  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.
  - pwinnski 6y ago
    Sadly, it doesn't appear BitBbucket has similar options, but good notes anyway, thank you!
    - Twisol 6y ago
      BitBucket's documentation shows similar tabs -- is this not what you see? https://confluence.atlassian.com/bitbucketserver/review-and-discuss-a-pull-request-808488540.html https://confluence.atlassian.com/bitbucketserver/review-and-... The section on "View a single commit within a pull request" shows something very similar to GitHub's "Commits" tab.
terrortrain 6y ago
This list is awesome, i love everything except: 12. Award all ties to your reviewer A true "tie" is really rare IMO. If it really is a tie, we should all should agree that person putting work into it should be able to decide their own code. Not only does it cut down on discussion, but it cuts down on work and communication needed to get the PR past the finish line. Allowing ties to go to the reviewer can lead to all kinds of discussions about meaningless stuff, like: ``` We should use forEach(...) instead of for loops, because that is how a most of other code is written. ``` In that example (from a real code review), the code is practically the same, just different style. So now the coder has to go back, re-write, re-test, signal for a re-review, re-discuss etc... All for something that is a tie, and doesn't actually matter either way. Alternatively, letting non-issues through without a duel every time, saves a bunch of time and energy on every ones part.
- vonseel 6y ago
  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.
- thelean12 6y ago
  I mean, consistent code style is just good practice, so maybe that's not a great example.
ktrmnj 6y ago
Clickbait title
luord 6y ago
This is some great advice, although I'm not completely sold on number 8 and number 9. For number eight: If your reviewer is being a douche, I don't think there's harm in pointing it out, respectfully. Nothing says that both sides can't learn from the review. As for number nine. I'm not sure if adding comments is going to completely help; I'd rather refactor to make the code as explicit as possible, including the tests[1]. But more importantly, I don't think that sacrificing possibly more idiomatic code for the sake of beginners is a great trade-off. I think it's better to encourage people to get more familiar with the language and technology being used. [1]: Which is usually my first stop for when I might not understand something in the code itself.
artemave 6y ago
How to make code reviewee fall in love with you: 1. Make a list of your points 2. Throw away half of them 3. Publish the rest
BerislavLopac 6y ago
Number 13 inspires a question: I wonder if there is any code review tool which will tell how many other (open) reviews a potential reviewer is on when you're about to add them to yours. I haven't seen it in any tools, and it would be a nice addition...
jiveturkey 6y ago
assumes your reviewer is competent
stonecharioteer 6y ago
This is good, and I would like to send it to a few junior engineers that I mentor. However some of them are girls and the title, unfortunately, makes it a bit unprofessional to send out. I am really nitpicking here, but please don't title your blog posts like this. How to Impress your Code Reviewers might have been a better, safer title.
birdyrooster 6y ago
Stop giving me 5000 line PRs. I get you are doing the jobs of five people, and maybe you haven’t mellowed on the Adderall yet, but I hate these reviews and I’m not doing as good a job as I otherwise could.

472 points by kiyanwang 6y ago

anotherhue 6y ago

This is marvelous

mtlynch 6y ago

Author here. Happy to answer any questions or take any feedback about this post.

whoisjohnkid 6y ago
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.
- mtlynch 6y ago
  Thanks for reading! Yeah, I had the same assumption for years. And then I started to realize that people would submit PRs to me where there's a ton of debug logging code left in or git merge markers they accidentally didn't fix. I thought, "Wait, are some authors not even reading this before they send it out?"
sixhobbits 6y ago
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 https://en.wikipedia.org/wiki/Goodhart%27s_law
- mtlynch 6y ago
  Hey Gareth, good question! It's unusual for me to try to "time" my submissions to Hacker News. When I published this post, I thought it would be interesting to live tweet my publication day process,[0] so I published sooner than I originally planned because it was a slow news day on HN. But it ended up not gaining traction on HN that day anyway, so even that optimization seemed to have no effect. But overall, I think the HN filter works pretty well. I can't think of anything I do to get an edge aside from investing a lot more time into editing and illustrations than I think most bloggers do, but that would be the filter working as intended. I do worry about taking scores and readership metrics too personally. When I published my first code review article back in 2017,[1] it flopped on Hacker News and on reddit, the two places I was hoping it would find readers. I felt awful because I spent more time on it than I'd ever spent on a blog post before. I took a break from watching the metrics and went for a run, and that cleared my head. I decided that I felt good about the post regardless of what response it got, and I need to have that attitude on future posts. When I got back from my run, it had taken off on reddit, which made me excited but also kind of interfered with the lesson I was hoping to learn. But I do try to view my articles mainly in terms of how good I feel about them and maybe 20% what kind of response they get. [0] https://twitter.com/deliberatecoder/status/1334156012132118530 https://twitter.com/deliberatecoder/status/13341560121321185... [1] https://mtlynch.io/human-code-reviews-1/ https://mtlynch.io/human-code-reviews-1/
  - sixhobbits 6y ago
    Thanks for the response! I made that 2017 post compulsory reading for a bunch of code reviewers I helped train if that counts for anything :)
bbarnett 6y ago
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.
- qualityonly 6y ago
  Wear a wedding ring? I had a similar thing in my early 20s despite the wedding ring, now in my 30s everyone looks for it.
foobarian 6y ago
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.
- mtlynch 6y ago
  Oh, that's interesting. I'd really only want to do that for things that are hard to test automatically. I have 100x more confidence in a test where I can see the source for it and watch it run in CI. But I agree that for exceptional cases where it's hard to write test automation, it's a good idea to explain how the author verified the changes.
alisonkisk 6y ago
"fall in love with you" is catchy but not the right interpersonal dynamic. Something like "love reviewing your code" is better.
- JoeAltmaier 6y ago
  Sure but that wouldn't get on the 1st page, right?
- nullsense 6y ago
  The one weird trick that will make your coworkers fall in love with your Pull Requests.
mannykannot 6y ago
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_6th_Edition,_with_Source_Code#%22You_are_not_expected_to_understand_this%22 https://en.wikipedia.org/wiki/Lions%27_Commentary_on_UNIX_6t...
- mtlynch 6y ago
  Thanks for reading! I absolutely agree with all of the exceptions you listed. I'm totally in support of comments when they're used judiciously. To add a bit of context, a tendency I've noticed when I encourage people to answer questions with the code is that they think that means "add a comment in case anyone else has this question," when really what I want them to think is, "rewrite the logic so nobody else has this question. If that's not possible, add a comment." In other words, a comment should be the last resort for explaining code rather than the first line of defense.
airstrike 6y ago
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.
- mtlynch 6y ago
  Thanks for reading! That's interesting about "work review" in Investment Banking. I've never heard that term and Google isn't helping me out much. Do you have a recommendation on where I could learn more? I have noticed that things like this aren't too far removed from other fields. When I wrote my article about reviewing code from the reviewer's side[0], I got feedback that most of the techniques apply to giving feedback in other domains. And while I was writing my guide to hiring writers,[1] I realized that the process is 80% identical to my process for hiring developers and graphic designers. The thing that's tough about going more abstract is that it becomes hard to convince any particular group that the content is worth reading. Focusing on code review appeals to HN because a large percentage of the audience is developers, but if it was a more general guide to improving review workflows, I'd have a harder time convincing HN readers that it was relevant to them. [0] https://mtlynch.io/human-code-reviews-1/ https://mtlynch.io/human-code-reviews-1/ [1] https://mtlynch.io/hiring-content-writers/ https://mtlynch.io/hiring-content-writers/
justinclift 6y ago
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.
- mtlynch 6y ago
  That was in jest. I assumed readers would understand that nobody's going to fall in love over proper code review technique.
protastus 6y ago
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.

lllr_finger 6y ago

This is great! Many of these things are slowly learned over time, but explicitly listing them out will be helpful when refreshing or teaching others. The three that have been most helpful on our team are: #1 (review your own code first), #2 (write a changelist description), and #5/#6 (narrowly scope changes/separate functional/non-functional changes).

kowlo 6y ago

Enjoyed the illustrations

mtlynch 6y ago
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/ https://www.lolo-ology.com/ [1] https://mtlynch.io/how-to-hire-a-cartoonist/ https://mtlynch.io/how-to-hire-a-cartoonist/
- kowlo 6y ago
  I didn't recognise your name - it just so happens the only other post of yours I've read is [1] https://mtlynch.io/how-to-hire-a-cartoonist/ https://mtlynch.io/how-to-hire-a-cartoonist/!

itsmemattchung 6y ago

Nice post. I agree with almost all of the author's points. However, I would add that "reviewing one's own code" is more difficult than it may appears. It's like reviewing your own prose, one develops blind spots. That being said, combing through your own CR for trivial mistakes is just due diligence. Also, 100% on the change list description. I mentioned the same thing in a similar post[1] of mine. [1] https://blog.mattchung.me/2020/11/09/3-tips-on-getting-eye-balls-on-your-code-review/ https://blog.mattchung.me/2020/11/09/3-tips-on-getting-eye-b...

Cthulhu_ 6y ago
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.
- mumblemumble 6y ago
  This is what I do. Even that small change in perspective is often enough for me to catch quite a few stylistic or functional problems two which I was completely oblivious when looking at the code in the IDE. Even in the IDE's own diff viewer.
- syncsynchalt 6y ago
  Hard agree. I spot things in `git add -p` that I had missed when writing the code in my IDE. Then I spot more things in the github "Create PR" page that I missed in the previous step. Then sometimes I spot more things in the created PR that I missed in the steps above. Always skim back over your PRs, especially in the final context that they'll be reviewed in. Something about it puts you into "reviewer" mode and you'll get a better PR out of it.
  - angrais 6y ago
    What you've described indicates that while you may not agree, reviewing your own code helps identify issues.

cmwelsh 6y ago

I thought this was a dupe but actually this received avid discussion on reddit 5 days ago: https://www.reddit.com/r/programming/comments/k5cbln/how_to_make_your_code_reviewer_fall_in_love_with/ https://www.reddit.com/r/programming/comments/k5cbln/how_to_...

PunchTornado 6y ago

this only works when the company actually cares and allocates time to improve code quality. Not workable in companies where you are expected to add 2-3 features daily.

lllr_finger 6y ago
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.
- alisonkisk 6y ago
  Maybe at a tiny startup org that doesn't have many users or sensitive data or money.
Cthulhu_ 6y ago
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.
treespace88 6y ago
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.

jakevoytko 6y ago

This is an amazing list. I just want to point out that there's an implicit requirement in this list, which is that this is optimized for reviewers who are willing to give you quick feedback. At a previous job, I worked on a team where we iterated towards the list that OP provides. It worked really well, and it worked because we had a culture of providing rapid reviews. The senior people on our team even thought that reviews were important enough to drop their own work. A lot about the team was suboptimal, but I was spoiled by getting such rapid feedback for my work. We had some partner teams that really challenged this. One was halfway around the world, so you were guaranteed to only get one review round per day. If you wanted to make progress while working with them, you needed to either send them huge changes or send them a bunch of changes at once. But structural feedback in an early change would cause you to need to rewrite the later changes. We broke this cycle by having them eventually give local reviewers stronger conditional approval in the integration path, which fixed our particular issue but showed that there were cracks in how we optimized our code review flow. The real trouble came from a local team, which was an overloaded platform team that we were built on. No matter what you did, they would only review your changes about once per week. Their feedback was super insightful, too, and they often caught very subtle misuses of their platform that would cause subtle user-visible bugs. You couldn't just skip their feedback. To try to manage their flow, they wanted teams to review pull requests locally before sending changes to them. So people on my team would write a change for this platform, which we'd review on our team. Our team would apply our rules and break the change up. We'd send the first one out. Then it would take a full month to get the entire thing in, to account for the back-and-forth. If you needed to get something substantial in, it could take months. My manager fought for months for them to acknowledge that this was a problem and fix it, and they did eventually dig out of it by setting SLAs and increasing capacity. I don't want to take away from this list, but I want to point out that some of it is a function of your reviewer's incentives. If your reviewers have different incentives, then you need to find your own list that works within those bounds.

mumblemumble 6y ago
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.

jsnell 6y ago

Great list, but I'd argue that point 6 is more often indicative of tooling failure. Why is it hard to review something that changes both formatting and the code? In the example it is because the tooling is visualizing both kinds of changes in the same way. But there is no fundamental reason for why that needs to be the case. The same applies to refactoring. It seems perfectly doable for a code review tool to detect code motion, and have differet treatment of the code that moved unchanged vs. the parts that were modified.

pimlottc 6y ago
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.
piotrkaminski 6y ago
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.
- dpcx 6y ago
  The problem I've found with this statement is that then no one ever changes formatting for fear of "messing up blame for the affected lines," and then no reformatting ever gets done. At some point, take the leap and do it (assuming you're not completely changing the formatting of the entire file/project/what-have-you).
- jsnell 6y ago
  Blame is also a tool. If a reasonable change messes up blame, that's a tooling problem :) For example git-blame can ignore whitespace, detect code movement inside a file, and even code movement across files. There's no reason why that has to be the limit of how smart blame can be.
mtlynch 6y ago

qualityonly 6y ago

How can you tell someone is wrong in Programming? Wrong outputs are checked by tests and you could do time studies. However that's after-the-fact. With abstraction there isn't a correct answer from code alone (outside possibly nested loops)

Igelau 6y ago

Coming to Netflix, Christmas 2021

modernerd 6y ago

Also: Add a “How to Test” heading with steps to your PR templates. This is not a replacement for automated tests or intended to shortcut a thorough review — it just saves a lot of time to give reviewers a list of setup and testing steps for anything they need to verify manually.

mLuby 6y ago
Before/after screenshots help too, if there's a visual change.

known 6y ago

https://en.wikipedia.org/wiki/Extreme_programming https://en.wikipedia.org/wiki/Extreme_programming helps

q3k 6y ago

Good post. For a longer, more prose-like read that raises the general same points and expand upon them, see Google's 'How to do a code review' [1]. [1] - https://google.github.io/eng-practices/review/reviewer/ https://google.github.io/eng-practices/review/reviewer/

perlgeek 6y ago

A bit of a controversial one: give your reviewer one obvious (but minor) mistake to catch. 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. This is likely more relevant when you present something to a business leader who has the feeling they have to contribute something, but it can also apply to inexperienced code reviewers.

mtlynch 6y ago
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."
- alisonkisk 6y ago
  Just put the mistake in quietly, and if they miss it, notice it yourself later or ask someone else to comment on it
- Hello71 6y ago
  I think a change of framing might be helpful here. Put in multiple bugs, and call it "training" instead. If you have multiple junior developers, have them all do it and compare results, reinforcing the idea of multiple reviews.
- guytv 6y ago
  This is a very interesting idea to me. Over the years I noticed that reviewing the same code knowing a bug is there will increase my chances of finding it comparing to not knowing a bug exists. It would be interesting to measure if introducing a "catch" in every PR would increase the number of times the reviewer will find additional issues.

hashkb 6y ago

Just complain to management that your reviewer is being mean and that you need to get this feature deployed right away for growth. Promise to monitor closely in production and go home.

stiray 6y ago

Well, as much I have seen code reviews in my life (ignoring a few people that have OCD), I have noticed that the trend is to: - add whatever comment that comes to your mind and you know wont be argued about (like using spaces instead of tabs) to have your persona look better on management statistics - not deal with actually understanding the code but rather argue about syntax (dont malloc, use vector.reserve(),...) - dont press accept until few lines are exchanged... again, boosting the statistics instead of giving something meaningful - ... whatever boosts the reviewer visibility, even going ballistic on irrelevant parts of code (while no one reviews 3rd party dependencies where most of the evil is) My proposal for code quality is different. Put together a team that communicates (a rare skill nowdays) and LET THEM communicate, offer them free beer afterwork, whatever interests them. Dont use enforced meetings like scrum etc., they just show that you have failed putting together a functional team. Make developers friends instead of rivals. Then avoid JIRA overhead, avoid management visibility,... just listen what people are saying (and preferably become their friend too), surpres any outside world influeces from your team and fire anyone trying to ladder over anyone else. Then you wont need code reviews. And you will always have a long list of improvements at any moment. Just send an email and ask. Be capable of coding. Be really good at coding. Everyone will follow you after "shit hit the fan" and team had to work over weekend and you have stayed with them and help them fix the bugs, analyze the code. DONT be a manager, be helpful part of the team (if this would be industry standard we can just fire 99.999% of managers).

mumblemumble 6y ago
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.
matthewmacleod 6y ago
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.

nullandvoid 6y ago

A couple from my pet peeve list - 1) ATOMIC COMMITS Nothing worse than a single commit, to address 10 separate comments. I don't want to have to go back, and cross reference my comments with your wall of changes 2) DO NOT SELF RESOLVE A COMMENT Wait for the reviewer to checkout your solution, I want to learn through your fix, aswell as make sure it's what I expected.

mtlynch 6y ago
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.
- nullandvoid 6y ago
  No problem I understand, it took me a while before I ended up sticking with (1). I still think it's much easier for the author to make those changes, and attach a commit ID to the relevant comment, than it is the other way around. As the article states, respect the code reviewers time. If i've put in some effort to point out issues, and you don't even spend the effort to address each comment individually, then you are not respecting the reviewers time. Maybe there are some cases you are right, but as a general rule i'll just always follow (1) as an author, as it honestly doesn't cost me much time, and makes my reviewers life easier.
colonwqbang 6y ago
Do you make an additional commit for each review comment? As opposed to just amending the old commit. Why?
- nullandvoid 6y ago
  You can squash into a single commit at the end Creating a fresh commit for each comment, provides me an easily clickable diff link

deleted 6y ago

[deleted]

falcor84 6y ago

Great post. This is the part I liked best: >When your reviewer makes a suggestion, and you each have roughly equal evidence to support your position, defer to your reviewer. Between the two of you, they have a better perspective on what it’s like to read this code fresh.

ivanstojic 6y ago

The title of the article makes it seem like having someone fall in love with you is a desirable outcome. > your reviewer will literally fall in love with you No, author, no they will not. They might appreciate my professionalism and focus on correctness if I follow this advice though.

aequitas 6y ago

> The golden rule: value your reviewer’s time This goes the other way around too. I've had some PR's that put copious amounts of time in to make them as complete and perfect as I could for the reviewer. Only to get them turned down for style reasons. If style is that important, make sure you document the rules or automate it (3. Automate the easy stuff). So contributors have a chance at figuring this out for themselves, without having to go back and forth. If the style comes down to personal preference you can apply rule 12. Award all ties to your reviewer, in reverse too. Nothing takes my motivation for contributing away as a reviewer that just must find something to complain about the code.

npsimons 6y ago
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/ https://black.readthedocs.io/en/stable/
- aequitas 6y ago
  > 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. I had the same thing. Also consistency and automation is what's really important, as it extinguishes every need for style discussions. Because you can trivially convert from the project style to personal styles and vice versa using things like Git attributes[0], as long as it is consistent. [0] https://git-scm.com/docs/gitattributes https://git-scm.com/docs/gitattributes
  - 6y ago

radihuq 6y ago

How do you guys handle managing multiple merge/pull requests that depend on the previous one? For example: Request 1: install dependencies Request 2: add scaffolding to application like adding a menu item, page layout, etc Request 3: add feature A Request 4: add feature B My questions: 1. 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? 2. How do you communicate Request 2 is dependent on Request 1? 3. How do you enable the reviewer to review Request 4 without reviewing duplicate code (since Request 4 has merged Request 1, 2, and 3)?

cbhl 6y ago
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.
onorton 6y ago
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.
- nullsense 6y ago
  What about structural stuff that just can't be hidden behind a feature flag? What do people do in those scenarios?
eat_veggies 6y ago
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.

ensiferum 6y ago

- Anyone focusing on style issues in a code review is doing it wrong and really just wasting each others time. - self documenting code isn't possible because the code cannot possibly document what isn't there. In other words sometimes the issue important details are in what the code isn't doing (such as not setting some state or not making that function call under some circumstances etc.

pwinnski 6y ago

I find myself nodding in agreement with points 6 and 7 (about mixed and large changelists), but unsure of how to implement the suggested fix. Given that I don't control the tools used, how do I ensure that functional changes aren't lost amidst formatting changes? I would love for BitBucket to ignore formatting changes in my PRs, but alas, no.

Twisol 6y ago
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.
- pwinnski 6y ago
  Sadly, it doesn't appear BitBbucket has similar options, but good notes anyway, thank you!
  - Twisol 6y ago
    BitBucket's documentation shows similar tabs -- is this not what you see? https://confluence.atlassian.com/bitbucketserver/review-and-discuss-a-pull-request-808488540.html https://confluence.atlassian.com/bitbucketserver/review-and-... The section on "View a single commit within a pull request" shows something very similar to GitHub's "Commits" tab.

terrortrain 6y ago

This list is awesome, i love everything except: 12. Award all ties to your reviewer A true "tie" is really rare IMO. If it really is a tie, we should all should agree that person putting work into it should be able to decide their own code. Not only does it cut down on discussion, but it cuts down on work and communication needed to get the PR past the finish line. Allowing ties to go to the reviewer can lead to all kinds of discussions about meaningless stuff, like: ``` We should use forEach(...) instead of for loops, because that is how a most of other code is written. ``` In that example (from a real code review), the code is practically the same, just different style. So now the coder has to go back, re-write, re-test, signal for a re-review, re-discuss etc... All for something that is a tie, and doesn't actually matter either way. Alternatively, letting non-issues through without a duel every time, saves a bunch of time and energy on every ones part.

vonseel 6y ago
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.
thelean12 6y ago
I mean, consistent code style is just good practice, so maybe that's not a great example.

ktrmnj 6y ago

Clickbait title

luord 6y ago

This is some great advice, although I'm not completely sold on number 8 and number 9. For number eight: If your reviewer is being a douche, I don't think there's harm in pointing it out, respectfully. Nothing says that both sides can't learn from the review. As for number nine. I'm not sure if adding comments is going to completely help; I'd rather refactor to make the code as explicit as possible, including the tests[1]. But more importantly, I don't think that sacrificing possibly more idiomatic code for the sake of beginners is a great trade-off. I think it's better to encourage people to get more familiar with the language and technology being used. [1]: Which is usually my first stop for when I might not understand something in the code itself.

artemave 6y ago

How to make code reviewee fall in love with you: 1. Make a list of your points 2. Throw away half of them 3. Publish the rest

BerislavLopac 6y ago

Number 13 inspires a question: I wonder if there is any code review tool which will tell how many other (open) reviews a potential reviewer is on when you're about to add them to yours. I haven't seen it in any tools, and it would be a nice addition...

jiveturkey 6y ago

assumes your reviewer is competent

stonecharioteer 6y ago

This is good, and I would like to send it to a few junior engineers that I mentor. However some of them are girls and the title, unfortunately, makes it a bit unprofessional to send out. I am really nitpicking here, but please don't title your blog posts like this. How to Impress your Code Reviewers might have been a better, safer title.

birdyrooster 6y ago

Stop giving me 5000 line PRs. I get you are doing the jobs of five people, and maybe you haven’t mellowed on the Adderall yet, but I hate these reviews and I’m not doing as good a job as I otherwise could.