Pull requests are an essential collaborative tool for modern software development. I wanted to capture the kinds of questions a developer should ask themselves before asking another developer for a peer review.

These are general guidelines, and are not exhaustive of the types of due diligence every software engineer undertake prior to seeking a peer review, with the aim to:

• Reduce the amount of recurrent defects
• Reduce the impact to the other developer’s time and energy spent defects in your code
• Increase the overall autonomy and productivity of individual developers

Developers should ask these types of questions prior to submitting work for review.

Prior to coding

• Can you articulate what problem you are trying to solve?
• Can you articulate your proposed solution before coding it?
• If the stakeholder is present/available, have you validated your solution with them?

Learning from previous reviews

Have you had a code review in the area of the code affected by your previous PR? If so, have you double checked your current PR for the existence of similar defects?

Some people may find it useful to maintain a (private) journal or log of previous PRs.

Code hygiene

• Does the code compile?
• Does the code pass all linting and sanity checks locally? How about the the CI server?
• If you are working on a UI project - does your code conform to the UI coding and naming conventions agreed upon by your organization?
• Have you removed any unused or dead code (such as scaffolding or work product code)? Anything related to debugging?
• Are there any merge conflicts you need to resolve as a result of your PR?

Code organization and correctness

• If an algorithm is material to your code changes, have you ensured that its as efficient and correct as possible?
• Can a hardened third-party component be used in place of anything you’ve hand coded for your solution?
• Assuming the code compiles, runs and spits out the correct behavior -- have you gone back and optimized the solution with the benefit of hindsight?
• If you are working on a multifactor project (react native + react web for example) - - are you properly separating component logic from the visual components for maximum reuse?
• If you are adding logic as part of your PR -- are you adding it in the right level? Is this logic appropriate, for example, in the component versus the reducers, versus the sagas?

Documentation

• Are you working on a ticket that is undocumented? Eg. if we are using trello tickets to track sprint activities -- have you added at ticket capturing your effort?
• Are there any external or internal facing documents that need to updated should your PR be accepted?
• Have you made comments in the code you are touching that will be necessary to explain what you did?
• If the change you are effecting originates from a trello ticket, JIRA, or some other bug tracking system -- have you documented there how your changes can be tested?
• In your PR, have you pasted links to the to the originating trello or JIRA item, so the reviewer has context for testing and review?
• If your PR affects the user interface, have you added screenshots of the affected UIs?

Testing

• Can you describe what functional change your PR is trying to effect? For example, what behaviors are changing as a result of the code you are submitting?
• Assuming your code is runnable, does it actually fulfill the behavioral requirements set forth in the comps, functional requirements spec, trello tickets, etc.?
• Have you manually tested the behavior you are affecting in the PR?
• If there is an automated testing framework in place, have you built tests that cover the code touched by your PR?
• If you are working on a UI project, have you:
  • Considered the cross browser implications? Have you tried testing it in the target browsers?
  • Considered the operating system implications? Have you tried testing it in the target operating systems and the versions thereof?
  • Considered the device implications? Have you tried testing it in the target devices and form factors?

Resource Bundles

The types of applications that Olio creates will often use resource bundles to describe the text or colors presented to the user.

• If text variabilization is supported (via internationalization or otherwise) -- have you made sure that any text you’ve added or modified isn’t a hard coded string?
• If color variabilization is supported -- have you made sure that any colors you’ve added or modified isn’t a hard coded color or hex string?

Exiting the current review

• Based on the PR feedback, have you identified new or existing areas of knowledge or ability to work on?
• Have you thanked your peer reviewer, and offered to reciprocate? :)

Do you have any suggestions for check list items or questions to add to our list?