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
ASK QUESTIONS! - I cannot tell you how valuable it can be when someone asks you questions about a PR. For a while, at my job I was the resident export at Elasticsearch. Because of that people, who shy away from reviewing my PRs bc they went beyond their own Elasticsearch knowledge. Since I wanted my code reviewed I started to tell people to just ask me questions. Have me explain the parts you dont get. By explaining how things worked, half of the time I would find the bugs myself. Plus, if you ask questions then you become more familiar with the code yourself.
+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.
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 further actions, you may consider blocking this person and/or reporting abuse
We're a place where coders share, stay up-to-date and grow their careers.
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!!