It took me a long time to warm up to pull requests. My early experiences with code review were all a tremendous waste of time: they were a bottleneck to getting things done, a forum for people to conduct their ongoing feuds or both. After years of being staunchly against pull requests, I've come around on them in the last year at Terrible Labs.

What I'm asking when I ask you to review my pull request

1. Does this make sense to you?

Start with cthe big picture and get more detailed from there.

My pull request should be trying to accomplish only one thing and that should be explained in enough detail in the commit message for you to understand, did I succeed? If something is confusing: you're not sure how this system works, or how this new piece of code fits in with other parts of the system, let me know and we can clarify it together.

Do the objects and methods being used make sense? Am I adding too much responsiblity to an existing object or creating a new object where one with similar functionality already exists?

Does the code in each method make sense and do the names of each method describe what it is doing?

2. Did I miss anything?

Sometimes I move a little too fast and I miss things. Could you double check and make sure everything looks in order?

Here are a few things that I tend to miss:

tests

database indexes

checking in the updated database schema

documentation

3. Was there anything that you really liked about this code?

Occasionally someone will comment on a pull request I've made and point out a piece of code, a pattern or something else they really like and its the most amazing feeling in the world. This happens far too infrequently. If you see something you like, call it out! Not only does it make me feel warm and fuzzy inside and like you a lot, it highlights cool code for everyone else on the team.

4. Is this ready to go?

If everything looks good and you don't have anything else to add, throw a :+1 in the comments so I know its ready to do.

A few things that kind of annoy me

1. Telling me that you would have done this differently

I often think this when reviewing other people's pull requests and I don't always remember this piece of my own advice. The thing is that this is really demoralizing to hear someone say to you and its totally unhelpful. If the two approaches are entirely equivalant, you should just go with the one that they have already implemented, tested and have working.

Chances are, however that each approach has some advantages and disadvantages over the other and these are useful to point out. The easy thing to do is just to fire off a comment saying, "I think you should have done it like this instead", but it sucks to be on the receiving end of a comment like that. Instead, take the time to really understand the two approaches: your's and their's and take it upon yourself to figure out what if any advantages your approach would have that can be incorporated into what they've already done. Maybe even go so far as to submit another pull request with some proposed changes.

2. Non-actionable lukewarm feelings

Youre going to come across some code and think, "I don't love this, but I don't know what to do about it." I suggest not saying this. In my experience, it just makes everyone feel bad and leaves you in a weird limbo state that you're not sure how to proceed from. Do you keep trying to rework that code and resubmit the pull request until you stumble upon something that is satisfactory to the reviewer? Do you move on to another task? Is this pull request ready to be merged? No one knows.

3. Pointing out violations of our styleguide

I understand that having a consistent style in your code is valuable and it is important to maintain certain aspects. I'll write another post with my feeling on styleguides, but please understand that this is NOT what provides value in reviewing a pull request.

Do you have anything else that you want or don't want from a pull request / code review? Let me know!

This comment has been minimized.

One thought on "don't tell me that you'd have done this differently": I do expect the reviewers of my PRs to throw out a red flag if they have any serious reservations about the approach I'm taking. There are subtle but important differences between those two.

"I'd have done this differently" shifts the focus onto the author and the reviewer, rather than on the code in question. That starts the discussion off on less than objective footing. Moreover, the objections seem to often be a matter of taste—perhaps you've used a singleton pattern, and while the reviewer doesn't like singletons in general, they aren't elucidating why singletons are harmful in the context of what you're changing right now.

"I have reservations about merging this", instead focuses on the reviewer's actual issues with the code. If I've just written a series of SQL queries that are going to melt down production under load, I expect my reviewer to say something. And if there's actually no way to make the real-time queries efficient enough and a different approach involving denormalization is warranted, I certainly want to hear about it. Same for designs that spread too much knowledge throughout the system or violate SOLID principals in some definitive way.

The caveat here is that pull requests are the worst absolute time for these critiques. Having someone point out (calmly and nonjudgmentally) that, yes, there's a serious flaw in my design and it need to be rethought is valuable—and way way way more valuable if it's pointed out while the design is still a bunch of ink on a whiteboard. Having this discussion after the point where the author has written the code and thinks it's ready to ship is a sign of communication breakdown and is an organizational smell. That said, it's even worse to not point out major issues and let something problematic ship just because the author had already put a lot of work into it.

If that's happening more than once in a blue moon, it's time to take a serious look at what's going on in your organization and how to get engineers talking to each other earlier.

One thought on "don't tell me that you'd have done this differently": I do expect the reviewers of my PRs to throw out a red flag if they have any serious reservations about the approach I'm taking. There are subtle but important differences between those two.

"I'd have done this differently" shifts the focus onto the author and the reviewer, rather than on the code in question. That starts the discussion off on less than objective footing. Moreover, the objections seem to often be a matter of taste—perhaps you've used a singleton pattern, and while the reviewer doesn't like singletons in general, they aren't elucidating why singletons are harmful in the context of what you're changing right now.

"I have reservations about merging this", instead focuses on the reviewer's actual issues with the code. If I've just written a series of SQL queries that are going to melt down production under load, I expect my reviewer to say something. And if there's actually no way to make the real-time queries efficient enough and a different approach involving denormalization is warranted, I certainly want to hear about it. Same for designs that spread too much knowledge throughout the system or violate SOLID principals in some definitive way.

The caveat here is that pull requests are the worst absolute time for these critiques. Having someone point out (calmly and nonjudgmentally) that, yes, there's a serious flaw in my design and it need to be rethought is valuable—and way way way more valuable if it's pointed out while the design is still a bunch of ink on a whiteboard. Having this discussion after the point where the author has written the code and thinks it's ready to ship is a sign of communication breakdown and is an organizational smell. That said, it's even worse to not point out major issues and let something problematic ship just because the author had already put a lot of work into it.

If that's happening more than once in a blue moon, it's time to take a serious look at what's going on in your organization and how to get engineers talking to each other earlier.

This comment has been minimized.

Love this statement, "My pull request should be trying to accomplish only one thing and that should be explained in enough detail in the commit message for you to understand". So many times we miss these simple points and starting here is a great reminder for me to keep code small and remember that the reviewers may not have as much knowledge about the what/why as I do. Don't force them to go to a JIRA/Trello card to figure it out. And avoid feature creep by combining lots of loosely related changes into the same PR. Keeping them small and focused, makes it easier to review and make changes.

I share Thomas' point about how someone may have done something differently, but mostly this is if I've missed an established pattern or a way to do something in less lines of code, more expressive, or taking advantage of a gem/lib that I'm not aware of. But I also agree that if can be annoying if it is just I would have done it this way. Unless you can tell me why it's better, it doesn't add value.

Thanks for sharing this, it is a great read and something I will refer back to. Although I would have ordered the sections differently ;)

Love this statement, "My pull request should be trying to accomplish only one thing and that should be explained in enough detail in the commit message for you to understand". So many times we miss these simple points and starting here is a great reminder for me to keep code small and remember that the reviewers may not have as much knowledge about the what/why as I do. Don't force them to go to a JIRA/Trello card to figure it out. And avoid feature creep by combining lots of loosely related changes into the same PR. Keeping them small and focused, makes it easier to review and make changes.

I share Thomas' point about how someone may have done something differently, but mostly this is if I've missed an established pattern or a way to do something in less lines of code, more expressive, or taking advantage of a gem/lib that I'm not aware of. But I also agree that if can be annoying if it is just I would have done it this way. Unless you can tell me why it's better, it doesn't add value.

Thanks for sharing this, it is a great read and something I will refer back to. Although I would have ordered the sections differently ;)

This comment has been minimized.

Totally, 100% agree with everything you've written, Thomas. Thank you for clarifying that point a little better. I pointed it out because on many occasions I have seen this exact comment or something like it on a pull request: "I would have done this differently." This always leads to several rounds of back and forth trying to tease from the reviewer what exactly they would have done differently and what, if anything that says about the code being reviewed.

The examples that you provided are exactly the sort of specific, actionable feedback that I love to see on my pull requests.

Totally, 100% agree with everything you've written, Thomas. Thank you for clarifying that point a little better. I pointed it out because on many occasions I have seen this exact comment or something like it on a pull request: "I would have done this differently." This always leads to several rounds of back and forth trying to tease from the reviewer what exactly they would have done differently and what, if anything that says about the code being reviewed.

The examples that you provided are exactly the sort of specific, actionable feedback that I love to see on my pull requests.