So you are using git? Awesome. But then comes your first pull request (PR). What should you expect? Or maybe it is time to do your first PR review on someone else's code. This too can be daunting. No one likes to be criticized, but when there is something wrong in the code, it needs to be brought up.
I have struggled with this over my near 20 years of programming, first in the form of code reviews, and now in the form of pull requests. I have done the Google searches, read leadership articles about it and most of all struggled through it myself. Surprisingly, this seems to be a set of skills that are not generally well developed in post-secondary computer science type programs (at least in my experience). Anyways, maybe through my pain and experiences you'll find something that helps.
Principles
I like to approach code reviews with some principles in mind. I have found that these principles help the process go smoother no matter if you are the reviewer or the reviewee.
Principle 1: Be flexible
Go into a PR review with an open mind. Coding is as much art as it is science. One thing I have learned over the years is that technical correctness and pedanticness do not necessarily equate to good code. For example, does it really matter that your variable names all adhere to a pattern if the code is too slow and frustrates a user? Further, I remember a time where a developer was so focussed on writing good code with an ORM, that they thought it was awesome. The problem was that this was a batch process that had to copy gigabytes of data from one database to another. Using a transactional ORM was basically an approach fraught with issues, as it needed to load each record into an object to transfer it. An alternative approach would have used a bulk transfer tool to stream the data between databases and minimize the memory overhead. However, this developer was so convinced that because his elegant code was perfect, that when the issue was brought up, he couldn't see past the technical correctness of his code for the real problem, so it went to production. Shortly, we started to see memory issues as the datasets grew and the process started to fail...
On that note, also remember that we all have different ways of approaching problems and different styles of coding. In the end if there are multiple styles in the code base, does it really matter if the maintainers can understand the code? For example, does it really matter that you missed a leading underscore on an instance variable if the rest of the name is intuitive to the team? Or does it really matter if your opening chicken-lips (AKA curly brace or { ) is on the same line as your method declaration or on the line following it? The code still compiles and executes the same regardless...
Principle 2: Be curious
When under going a PR review, approach it as a learning opportunity. Both sides of the PR have an opportunity to learn. Instead of being critical and saying something like "This is wrong because X", instead ask a question like, "This is an interesting approach, what led you to go this route?" You can also go one step further and add a suggestion if you truly do think that something is broken. For example, "Your logic for processing this collection is nice and simple and solid. I wonder, though, could we improve it however by using the new .ForEach() collection method so the runtime engine can optimise and process the collection in parallel or multi-threaded?" Doing things this way gives credence to what the person did, but asks open ended questions to start the dialog about how things can might be improved.
Principle 3: It ain't personal
When working in a team, it is important that everyone realize that code is just code. Try not to treat it as an extension of the author (yourself or others). While it is logical to feel this way (someone wrote it after all, using their own brains and ideas, so there is an amount of reflection of self in there). However, those feelings do need to come second to the goal of the team and the purpose of the code. For example, I may pride myself on a clever trick in my code, or what I think is a particularly elegant way of writing something. However, if that code is hard to maintain for others, or is super slow for an end-user, it probably needs to change. I have encountered far too often developers that measure self-worth by what others think of their code (and I will openly admit that I have been one of these myself in my mid-career, but I think I am mostly past that now). Thinking this way can be truly devastating to all involved. Do yourself a favour and figure out how to move past the code being "yours". My work life has been much happier since I figured this out. Also, I find that people approach me more easily and freely now.
Principle 4: Don't forget the human element
I have found that in this day in age, we often put technology between us and others. Texting instead of chatting in person or making a phone call is one example of this. Don't get me wrong, text messages and chat systems like Slack are amazing and bring huge benefits, but we often forget their limitations and downsides. People are inherently social animals and inherently behave differently in near-to-face situations.
Another good example, in the context of PRs and code reviews, is that many git systems (Github, BitBucket, Gitlab) allow comments to be placed on and throughout PRs. This, however, can cause problems because these systems remove the face-to-face and PR reviews are inherently about "checking up on someone's work". So much non-verbal communication and other social cues are lost when you only use these systems. Don't get me wrong, things like this are useful and have their purpose, we can't forget that we are social beings. I suggest knowing your audience and adjusting (see Principle 1: Be flexible). I have worked with people who are fine with only using this system of communication as they don't take offence if something could be taken not as intended. Yet, on the same team, I have worked with people for whom this doesn't work very well (they always seem to feel attacked). What I have found is it is important to know what works for different teammates and adjust. When I am not certain how a teammate is likely to react when I review a PR, I generally do the following:
I use the comments system as a way to mark issues, including a brief description of what the issue might be (I heavily apply Principle 2 when writing these descriptions). This is simply so that nothing gets forgotten and so that we both can find the affected code quickly. Nothing more.
When I have done my review, I sit down with the person, in person or on the phone, and step through where my concerns are in the PR (I am also careful to not just read the comment to the person...something about having the person right there listening to me, autocorrects anything potentially offensive in my written words). Again heavy application of Principle 2 has proven to be useful here.
In Summary
So there you go...my approach to PR reviews both as a reviewee and reviewer. Basically, it boils down to being humble and treating it as a learning opportunity.
In the immortal words of Wil Wheaton, "Don't be a dick."
Top comments (14)
Nice post Chris, I agree with most of the things you were describing and the conclusion fits perfectly. Still, I have to disagree with one thing you mentioned which is:
I mean sure code styling isn't the most important thing in a PR but still most IDEs and dev tools today offer code styling automation, which just eliminates those things from PRs.
So I would conclude: Yes don't be a dick, but also don't make your life unnecessarily hard.
I agree with Sir Mo. It's not really hard to follow a standard style and all it takes a couple of key strokes. When the code is not formatted properly, it's hard to follow through the code and understand the intent of the developer. At my work, we don't really have PR review meetings as such. We work on agile development and I receive multiple PRs in a day or sometimes a huge PR with 10-20 files modified. If it's not formatted properly, it did took some time to go through and do review on the PR.
BTW it's a great article. I have some challenges on the "Human element" principle, I have been working on it.
Thanks Chris.
Great points guys! Styling is important for all of the reason you guys state. How important it is though, is really something, I have found that needs to come out of the dynamics of the team. For my current team, this means that we don't get upset and block PRs for minor style violations. Instead we may just let the submitter know and merge anyways. We have found for us, this works best. For your guys' teams, it sounds like it is different, and that is also awesome.
I think Sir Mo actually hit the nail on the head as to the point I was trying to make:
There is always a balance to be had, and it is different between teams. In my current team, because we are mostly laid back about style, one person becoming a style "enforcer" (and this has happened) leads to life becoming unecessarily hard. I have worked on other teams where being the lone wolf who does not follow style guidelines and standards would lead to the same result.
Thanks again for the feedback!
I fully agree with this. In the grand scheme, indentation and brace styles do not matter much and, in the end, we will be able to understand the code (or not) regardless of which style was chosen.
But that's exactly why anyone submitting a PR should not be too lazy to stick to the existing style. Having different styles mixed together is just an unnecessary annoyance and distraction for any reader. Consistency is key.
Well said!
Thanks for your article !
I'm a developer from 2 years now and I still need to deal with the "Your code is bad but it ain't personal" and I'm ready to learn from my mistakes and from my team mates though.
But when the manager say "I need something which works in four hours", I'd love that this information would be taken in consideration when the lead developer perform the PR review...
How do you deal with this kind of case ?
PS: english is not my native language so sorry if it's not clear ! Feel free to correct me :)
If my understanding is correct, You are referring to a situation where the manager wanted to get something done in limited time(4 hours in your case) and you ended up delivering the feature that works but may not be up to the standard(coding style/technicality/implementation details).
I agree that the situation is more common and the technical term is "Technical debt", where the delivery of the feature prioritized over the code quality/implementation details.
At the basic, I would suggest adding a comment on PR that why particular decision was taken (timing constraints, limitations, etc.,) and possibly a "To-Do" in the code to mark the implementation is not completed yet.
However I'd tread carefully on what is considered as "Technical debt".
If my answer satisfies your question, I would recommend reading this.
Thanks for your answer ! I think I still got some difficulties with the technical debt when a feature is urgent because end users are in pain with the actuel system (or without system at all).
The article you gave to me is great !
I think we, managers and code reviewer, must care about technical debt only when the end users aren't in pain with the current system and when they don't need a quick release to solve a job issue. Finally, if we choose the dirty approach to solve a problem quickly and to make the users happy, it's only us, devs, which gonna have pain in the future. I think we must find the middle ground :)
Again, thanks for your answer, I learned a lot ! :)
Yes You are correct. To quote from a book that I was reading earlier,
“It is better to ship something and accept some prudent technical debt than it is to be late for the sake of gilding a working solution. That said, every project is unique in its tolerance for timeliness versus completeness.”
Excerpt From: Gary McLean Hall. “Adaptive Code: Agile coding with design patterns and SOLID principles, Second Edition
Nice !
It is funny that I met today at my job the exact situation described in your exerpt. We build a new system from nowhere which was badly integrated in our current system. In one hour, we could make the new system work permanently with the previous one without any conflicts (the dirty way). Or, we could rebuild all the system from scratch with new specs and new hands (the cleanest way). But we had users which have waited a lot of time for this new system. From this, the decision has been made : we are assuming the technical debt. To solve the debate, a teammate asked this question : "Why the end users have to pay for ours bullshits ?". And for the first time, I knew that the cleanest way is only about us... :)
I will look after this book, thanks for sharing !
Lots of good things in this thread so far! I can't offer a guaranteed solution of course. When I get into this situation, I have always found that communication is key. I would also suggest that face-to-face or at least voice-to-voice communication between the 3 parties is key. What I see likely has happened here is that your manager has inadvertently given you one direction ("I need something working in 4 hours"), and your team lead a different direction (something like "It is your responsibility to make sure the code is solid and maintainable"). Each request in and of itself is reasonable. However, taken together, they are sort of at odds with each other. I am guessing that neither of them has realized this. So a quick conversation with both of them will likely help.
Another thing that I have found has helped hugely with these situations is iterative development. Sure, get something done and working in the 4 hours required by your manager. Let your team lead know that this is your first goal and is your priority and that it needs to go through. Then iterate once or twice more to fix bugs and clean up the code styles.
The whole getting past "Your code is bad but it ain't personal" is quite hard to get past, but hang in there you will get it! It took me the better part of 20 years myself!
In social work, they use something called "the feedback sandwich."
Say something positive.
Say the negative thing that needed to be said.
Say something positive.
It's surprisingly hard to do, and amazingly effective in getting the critical message across without causing resentment.
To me that is one of the most beneficial things about pull request reviews! Glad you are seeing the benefits!