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.
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?
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
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.
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.
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.
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.
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!
A+
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.