Code Reviews

The team lead is responsible for giving the final approval to a pull request, and will usually be the one to merge the pull request in GitHub. A developer must request the team lead to review all pull requests. A developer may also choose any other developers to review their code.

If possible, pull requests should be reviewed by the team lead the same day that the “Awaiting Review” label is added to the pull request. Obviously if the PR is submitted later in the day it can be reviewed the next morning. Because our teams are small and distributed and we are often working on tight deadlines, prompt code reviews are very important.

Developers should ask themselves the following when requesting a code review:

Is this ready for production?

Are there tests written for all functionality? Are the tests passing every time the suite is run locally? Have I removed all logs and puts statements? Always assume that your pull request will be deployed to production quickly after being reviewed

Have I tested for edge cases?

Are all possible errors being handled appropriately? Does my code depend on the front or back-end being deployed?

Have I documented my code in comments when necessary?

Sometimes we have to write complex code to solve complex problems. In these cases, we should always document our code with comments to explain what each piece of the code is doing and why it is important for each piece to exist in order to solve the problem

Developers should not perform a code review unless the merge status and Codeship integration are successful. These two checks must pass before a pull request will be considered for merge.

Code reviewers should ask themselves the following:

Are there any obvious security issues?

Is any sensitive or personal data being leaked? Most likely not, but this is probably one of the most important things to catch in a code review

Can I read the code?

Does the code make sense? Is it relatively easy to follow? DRY? Not over-abstracted? These things can be subjective, but it is always better to ask why something was done than to approve code you do not understand.

Are there any apparent bugs?

Do any syntax errors jump out? Is a module being used but not imported? It’s easy to miss small things when developing a feature or fixing a bug. We’re here to support each other, so nobody will get offended if you point out a missing semicolon or an extra parentheses.

Does it effectively address the user story or bug that was assigned?

Programmers have different ways of solving problems, but if a pull request is missing an edge case, could be causing false positives in testing, or could be accomplished with significantly less code, it is important to let the author of the code know what could be improved or fixed.

Does all of the code adhere to kohactive’s style guides?

The linters we use should catch the majority of cases when code doesn’t match the kohactive style guides, but it’s important to keep an eye on style when reviewing to make sure that the author is writing code that is clean, consistent, and readable by the entire team.

Always be kind and courteous when giving feedback in a code review. If something can be accomplished in a quicker or cleaner way, include links to documentation that will help the original developer to understand your recommendation.