Be Kind

Being better  prepping your work for code review

Less seasoned programmers do not have the built-in habits and knowledge required to have work ready for code review but even programmers who are more experienced may have not picked up these habits in their work environments. You can do better at respecting the time it takes to review code by making sure what you’ve brought to the table is worthy of your peers’ time.

Why review?

Reviewing should accomplish a few goals:

Find bugs
Enforce coding style and idioms (to make sure nothing has — slipped through.) There should NOT be significant style guide lapses that need to be commented on.
Check documentation
Share knowledge. This is probably the MOST pertinent use for reviews. This reduces bus factor in your workplace, and is a solid investment of time. I work with a team that really fights for us to be better programmers. We have a Web team SOP (standard operating procedure) which is a quick read to communicate workflow standards in our group. After having committed the atrocious sin of getting code through PR review without having run all tests on it, I broke our build. I immediately went into ‘must fix this’ mode. Unfortunately for me, embarassment can be a driving motivator.

How do I fix this?

In many fields, checklists exist because human brains are fallable and prone to distraction. — David Trowbridge

I missed an important step in my development process because it is not muscle memory for me, yet. I know. I KNOW. It’s horrible. When I’m trying to learn a ton of new things on a daily basis, having to run through a mental checklist leaves room for obviously horrible errors. So I set out to write the ‘Being Better’ lets-get-ready-to-PR checklist.

This checklist is specific to my team’s workflow. It covers basic assumptions that a more seasoned developer would hopefully know by heart.

Should.

Phase one:

Look at all pages which ‘require’ any modules you modified
Test locally by running through the story user states in browser(s)
Check with AND without feature flags enabled
Any permissions? Check different permission views
Is push involved? Must send pushes for states.
Look back over the visual comps. Did you miss anything?
Doublecheck with a linter — we use jsl
Run scoped — A tool to check lexical scope
Did you write / modify jstests?
Are you sure you do not need to add any tests?
Run npm test on all jstests, [1] command line and [2] in browser
Is there any Python work that coincided with this? Write / run python tests! (And wash your hands)

Phase two:

Rebase and squash to minimum number of relevant commits
Ensure all pertinent context is provided for reviewers in PR summary

The existence of this checklist does not mean you follow it verbatim. I have been taught that these are healthy habits to follow that allow you to respect the time of your teammates. It makes the most of your PR by having your talented coworkers only have to focus on what’s important in your review.

Do not be a jerk.

Something I did not put on this checklist is timing. When is your feature DUE? If you have a deadline, stay cognizant of it. It is NOT the responsibility of your teammates to rush through a review because you did not give them enough time to give it a thorough look. If it is an especially dense or heady review (max LOC is a restraint for PR on our team, so that would not usually be an issue for us), giving your teammates a glimpse at your WIP gives them a little prep time for digesting your final review.

All of this boils down to being a good code citizen. I have awesome colleagues. They deserve time to themselves. Let’s waste time talking over a cup of coffee or playing ping pong. Give your coworker the gift of a better quality day!

Originally published at hackygolucky.com on January 23, 2014.