Code Reviews

Submitting a pull request

Code reviews are almost always recommended for merging into the dev and master branches. Code reviews give beginner programmers a chance to be taught how to code, and senior programmers a chance to show off (so to speak :)

1. Work on your own branch of business (BOB). A BOB is dedicated to implementing one feature or fixing only one bug.
   • Branch off your regular branch, or if you don't have one, branch off dev (git checkout dev && git pull origin dev && git checkout -b {branch_name} dev).
   • Regularly, sync (pull dev, checkout yours, merge dev) your own branch with the dev to prevent merge conflicts when your feature is ready.
   • Document all new features: how they work, what inputs the functions accept, what the outputs are, etc.
2. Test on your own branch
   • Is it working? If not, fix it before going to the next step.
3. Submit a pull request from your BOB to dev
   • Stop working on the BOB. To work on other issues while the code review is pending, create a new BOB for each issue.
4. One domain lead, or two or more general developers will be checking your code.
   • Unless you are the only employee in the company, you cannot review your own code.
5. Perform changes directed by code reviewer[s], if necessary.
   • Code reviewers will be asking you why you did something and possibly suggest a better way.
     If you know you are right, tell them!
6. Merge into dev!
   • This may or may not be done by you; if the change can be merged automatically, the code reviewers will usually do it for you.
   • If there is a merge conflict, fix it and merge again.

Reviewing a pull request

As a senior developer, you have a chance to maintain the quality of our codebase; and as a junior developer, you get hands-on experience with reviewing other people's code - the best way to learn your peers' coding style!

1. You may open or close a pull request if and only if you are the lead code reviewer. You are the lead code reviewer if:
   • You did not submit the pull request, and
   • You are the only reviewer assigned, or
   • You are the only employee, or
   • You are the domain lead of the area of change, or
   • You are the subject matter expert of the field.
2. Check the code to make sure everything is:
   • well-documented
   • meets code conventions and style guides File convention is preferred over language convention.
   • follows structure of the rest of the code
   • logically laid out, and makes sense - if you don't understand it, it is too complicated.
     You have the right to ask for more documentation, recode, or reject the pull request!
   • efficient (time, space, DB pulls / puts, CPU, etc. Efficiency is second to maintainability, unless you are wasting resources outright)
   • just feels right in your gut
3. If the pull request satisfies the previous step, skip this one.
   • comment on offending lines to request a fix.
   • check pull request regularly to address responses.
   • repeat this step until all comments have been addressed by the developer.
4. Be extremely critical - the Willet codebase should be a pristine object of codified perfection! It is up to you to keep it perfect!

How to write a commit message

Do:

Fix Product loading style, UUID generator inputs 

Don't:

Fixing thing
Oops, fixing again
Stupid button won't work so here it is

Notes

Code reviews are not meant to slow things down - it's all about ensuring we have a good code base and that everyone knows more about parts they are not working on. In fact, if you have a mundane fix that needs to be pushed out as soon as possible and no one will object to such change, you might as well work on master directly.

Have comments, concerns, complaints, improvements? Tell your team!