I'm never sure whether I'm using this tool as effectively and efficiently as possible.
Comment describing your workflow so we can share tips and get to know our unknown unknowns.
For further actions, you may consider blocking this person and/or reporting abuse
Top comments (27)
When I did code reviews, and for any team I lead from a technical aspect for, my a review has three phases of confirmation:
Automatic
Operation
Experience
Sure it seems like a lot, and yes it may slow down the merge process a bit. But in my experience taking a little longer to review code can help prevent issues from getting to production.
The holy grain though is a 100% automated process with confidence in the automation that when a change begins throwing errors the changes are rolled back, the errors logged, and the committing developer alerted. But that is a story for another time.
Love this list!
A comment on the first point...
I donโt always stop if the tests arenโt passing , especially when reviewing less senior level engineers PRs. Iโve found that in a collaborative environment, a detailed review in spite of test failures often shows an obvious mistake that can quickly be pointed out, increasing the overall velocity of the initiative.
One possible addition...
Reviewing the test additions or updates first can often guide the review nicely, framing the changes in an easy to follow and logical way. Plus, if reviewing someone for the first time, it helps clarify the patterns and structures they use.
Awesome
Wow, I need to write that down. Great list
For frontend development, there seems to be an additional step that is needed.
Pull Requests should be accompanied by UI reviews. Because you can only see so much in code.
Adding to this I ask to use screencastify plugin and create gif to compare experience/features before and after changes.
Really thorough answer. I agree with it all!
Great looking checklist!
Generally I'm sanity checking. Does this look like code? Are there any obvious typos? Any residual debugging statements? Anything that feels like a mistake?
If I want to get more involved, I'll pull the branch and test it. But sometimes it's better to get the code merged and fix bugs later, in smaller PRs.
The longer a pull request is, the less likely the reviewer will catch anything of importance.
It started as nice javascript in the 1st picture and in the 2nd is just a clusterfuck of symbols and letters hahaha. I agree with the smaller PRs, easier to catch stuff there.
I would advise against merging code with bugs, which can end up in production pretty easily if you are not careful.
That's a great point, and to clarify, I wouldn't usually approve a PR with bugs. But I think there are cases where it's acceptable:
Every day that a PR sits unresolved is technical debt; it gets further and further out of sync with the master branch, the original author starts to forget what they did, and all the people that could have been using and testing the new code, aren't. So the risk of letting bugs into production isn't the only risk in this situation, nor, I submit, is it always the greatest risk.
When I was teaching, students would submit their homework/labs as pull requests, which leads to reviewing a couple hundred a week. My quickest tip is the tiny
command line instructionslink on GitHub saves a ton of time.I would copy and paste the
Step 1changes into the CLI and then test the code on my computer. Then, I would normally do inline comments using the plus button that pops up when you hover over a line of code in thefiles changedtab with things I would change or things that were awesome!Yeah I do that too. This morning (prompting this discussion), I had the feeling that maybe I want to do this via GitHub Desktop because it just seems like the more flowy way to do it. I don't like that app much for most of my workflow, but in terms of quickly going from web to review it could be better.
I'd add that you can now make multi-link comments (comments on multiple lines of code) which makes it much easier to provide context
Flow for when I am familiar with the code
The Why - Understanding the the reason for the changes. Was this a bug? Was this a feature request?
a. Bug - Does this change fix the bug? Does it introduce any other possible bugs?
b. Feature Request - Look at the corresponding JIRA ticket to ensure that all parts of the feature request are complete.
Logic - Dig into the logic and ensure that it all works like it should. Unless the logic is extremely complex, I usually will not pull a branch down and test it myself. We have a pretty comprehensive test suite and if there are good tests written for the new code its usually unnecessary to test manually unless its a big feature which will go through Q/A.
Performance - Are there any unnecessary MySQL queries? Are indexes in place where they should be? How will this code perform when executed a million times? How will this code affect our databases?
Edge Cases - Since becoming a Site Reliability Engineer, I feel like a lot of my work deals with edge cases. Code works 99% of the time, but if given this scenario, it breaks. I like to take a step back and look at the big picture for PR. How will this interact with other moving pieces of the code?
Code style and cleanliness - Are there bits that could be refactored to be more readable or more reusable? Are any variables not quite clear based on their naming?
When the PR deals with code I am unfamiliar with
+1 to "Ask Questions". Not only from the perspective of giving the author a chance to explain why they chose the path they took, but also from the perspective of a reviewer trying to learn, as well as a mechanism for prompting discussion.
I've learned a ton asking questions in PR (ex: "oh, so you can define a nested class in this language?", etc), but I've also used it with great effect to guide a more junior dev to realize ways they could improve their code and understand the "why" of one approach to solving a problem being better than another (ex: "so if we had to change this value here, what else would have to change?" or "if this value was null here, what would happen in the subsequent lines that follow?", etc) Ie "ask don't tell" can be an effective review technique for avoiding putting the author on the defensive.
I actually do that a lot when I am teaching in person but I never thought about doing it on a PR! Good call!
Huge +1 about asking clarifying questions. You get to learn something and the person who raised the PR also ends up with more clarity. Sometimes a better understanding and sometimes better solution - win-win!!
For long pull requests, I use a practice that I call "micro/macro reviewing"
Stage 1: Micro reviewing
Stage 2: Macro reviewing
I hardly run the code myself. Mostly because I'm lazy, but I trust code authors and our test suite.
My day to day job requires PR to be reviewed by my teammates and typically what the procedure is like is
Look through the PR file changes and go ahead and pull the branch to test the feature is working correctly.
Also make use of GitHub's tool for suggesting changes and adding comments to individual lines of code
0 Should pass in the test (all) to open a pull request
1 Pull the code. For me is not easy to understand the changes on a pull request
2 I can reviewing in different ways it depends if is a senior or a junior developer if is a junior the reviewing process is in pair.
3 Look through duplication code, performance and the most important details
4 Reviewing process is not a feature/bug test remind the developer of this
We use specific PR template which includes points about how was the code tested, new learnings, etc
Sample here github.com/Kommunicate-io/Kommunic...
I am not going to repeat what others have already said, but I will just add that it is sometimes important to ask the author to explain what he wanted to do and why. Just because the code says it does something and it is properly executed, it does not mean it was what suppose to be done. Make sure they actually understand what the task they are trying to do is about.
Adding to the awesome list -