DEV Community

I made over 1,000 code reviews - this is what I learned

Juraj Malenica on June 04, 2020

Over the last 3 years, I’ve reviewed more than 1,000 pull (merge) requests. During that time I learned a lot – mostly about how not to review code,...
Collapse
 
miketalbot profile image
Mike Talbot ⭐

Very good points, I like the "I ran this locally" - might be adding that flag :)

Collapse
 
jurajmalenica profile image
Juraj Malenica

It's a life-saver :D

Collapse
 
emperorkonstantin profile image
Konstantin Anthony

Turns out I don't also have to be a QA tester when I'm approving pull requests... this is a great idea.

Collapse
 
andsmile profile image
Andrii

Sit with the author to review the code

from my point of view, you should do this only when you have question to PR or if it is quite big, otherwise it is just waste of time.

you will have a faster feedback loop which can help you both grow.

i don't see how 'sitting with author to review code' it can help both grow.

I ran this code locally

this looks really great

Collapse
 
jurajmalenica profile image
Juraj Malenica

I completely agree that if a MR od straight forward, there is actually no need to sit together. Will update to make it explicit.

For the growth effect, it's more relevant to learn how to better communicate, teach and take real-life criticism.

Collapse
 
mx profile image
Maxime Moreau • Edited

Sit with the author to review the code

Voice feedbacks is always great :) But of course, as mentioned the PR should be "important" (yeah, it's relative... But if the PR only changes a very small thing it's not necessary).

I'd also add something to improve the conversation: I always read the PR before (maybe two times) & try to understand briefly, if I can't I write a question. In brief, I prepare the meeting.

I had a tech-lead that didn't read any PR before the sit, and so he was reading the code along with me. It was such a pain and a wast of time... That's why I use this process for me to review PR :)

Collapse
 
rchernanko profile image
Richard Chernanko

Nice post :-) One thing I changed in my code review style - In the past, I always reviewed the code just in e.g. GitHub, but I soon switched to actually checking out the code locally and having a bit more of an explore. Found that I would spot things in my IDE that I never would have flagged just by looking on GitHub. Seems so obvious really but it really made a big difference!

Collapse
 
robertbeier profile image
Robert Beier

I can confirm from my experience that those are really important points! Thanks for the article.

A have a concern to add to the point "Sit with the author to review the code". I need some time alone for every PR. With just few exceptions. I need some time to go over the code line by line, mark conspicuous parts, and check them. This may take more than 30 minutes for many pull requests because I really make sure to check everything. I already found many important bugs like this. If the author is sitting next to me for the whole time, it may be boring for him/her. And I feel under time pressure.

Nevertheless, I find this is a good point and I will try to sit more together with the author.

Collapse
 
jurajmalenica profile image
Juraj Malenica

Thanks for your input. I agree that you need to have deep concentration to fully review code

Collapse
 
drhyde profile image
David Cantrell

I think it's OK to be direct about problems and suggest specific improvements, provided you phrase them right. For example "this variable name violates our coding standards [link] because [reason]. How about $wibbly_boing instead?" (but not "I see you haven't read our coding standards, this should be $wibbly_boing"). You should also say stuff like "this is great, I wouldn't have thought of doing it that way, hey @everyone take a look" sometimes.

When I'm writing documentation on working practices I put it something like this: "Code review is not a competitive sport with winners and losers, it is a never-ending conversation aimed both at making improvements but also pointing out best practice.".

As for "I ran this locally" I usually won't even bother reviewing until the automated tests have passed, which makes most of the problem go away. There's still the issue of non-existent (or flawed) tests which mean the code still hasn't been run locally, but that's the sort of thing I'd expect code review to catch. One of the automated checks I like to see (although most places don't have it) is that the test coverage is the same as or higher than the coverage of the merge target. That is again just a guideline, but a dev can expect Questions if he makes the coverage go down!

Collapse
 
mikaelgramont profile image
Mikael Gramont

Thanks for this post, it highlights things that are very important and yet often remain unspoken.

If your org is doing code reviews, it's worth writing something like that down and referring everyone to it.

It will not only push reviewers to be considerate (as is recommended here) but it will also make people who send PRs feel a lot more comfortable. You gotta admit that sending your first PR can be pretty intimidating sometimes, that pressure needs to go.

Collapse
 
charles1303 profile image
charles1303

I am really up for the "I ran this locally" concept. Remember when I had to implement this for a dev team by running all unit tests automatically and update a file before completing a successful git push command when triggered.

Collapse
 
ryanpwalker profile image
Ryan Walker

"Bigger requests require exponentially more time to review." I disagree with this. It really boils down to how complex the changes are, but most of the time it's much easier to review 1 PR that's 20 files than 2 PR's that are 10 files each. Each PR takes some setup time to get the code running and understand what changes are happening. And even if the bigger PR required more time, it wouldn't be "exponentially" more.

Collapse
 
jurajmalenica profile image
Juraj Malenica

Hmmmm maybe we're looking at it differently. It seems to me that you are talking about having one logical change (e.g. feature) in 1 MR vs. 2 MRs. I agree that 1 MR is preferable.

But I often see someone develop a feature and do a refactor or an unrelated bug fix, contributing to the file changed count. In that case I would rather have 2 MRs than one, because I'm losing focus from the important things.

Collapse
 
babakks profile image
Babak K. Shandiz

And still there are pull requests with just a title. 🤦‍♂️

Collapse
 
hannahsaurusrex profile image
Hannah

These are great tips. I like the template that everyone uses when creating a pull request! 👍🏻

Collapse
 
luispa profile image
LuisPa

Great post! So many useful insights

Collapse
 
rndmh3ro profile image
rndmh3ro

Add a flag called I ran this code locally

In an ideal world the PR would automatically tested on a production like environment so there should be no need to run the code locally.

Collapse
 
slawikus profile image
Slawa Gladkov

Should one split a feature into multiple PRs to make them feasible for review? Like PR per task within the feature scope.

Collapse
 
jurajmalenica profile image
Juraj Malenica

Ideally, the feature would be small enough to make for a reasonable PR. Breaking up the PR would split the context - it's hard to see the big picture.

For example, let's say that you're developing a web app and you are adding comments section. Maybe it would make sense to split the PR into backend and frontend, especially if those two are reviewed by different people.

Collapse
 
slawikus profile image
Slawa Gladkov

Thanks for reply! Sure, feature slicing is always a good idea but I am talking about situations when feature cannot be further sliced down.

Thread Thread
 
jurajmalenica profile image
Juraj Malenica

Yeah, then it's better to have it as one PR. What helps me the most then is to sit with the author and speak my thoughts out loud. It makes the process go faster

Collapse
 
nsacerdote profile image
nsacerdote

Nice points!
Small typo found: ... we all actually knew that it should be ...

Collapse
 
jurajmalenica profile image
Juraj Malenica

Updated :)

Collapse
 
craigeddy profile image
Craig R. Eddy

Adding a "reviewer(s) ran the code locally" checkbox would be a good idea as well. 8}

Collapse
 
jurajmalenica profile image
Juraj Malenica

Hahahahaha we can let him slide I guess

Collapse
 
wilsonmar_6 profile image
Wilson Mar

Super useful suggestions. Thought you'd want to know about a small typo:
"we all actually new that it should be done."
I think should be knew as in
"we all actually knew that it should be done."

Collapse
 
infomiho profile image
Mihovil Ilakovac

I love it!