Code of Conduct for Pull Requests

This code of conduct is based on a ClubCollect document that evolved over the years. It may contain also snippets lifted from other places. I am unable to attribute all authors. Typos are all my fault.

In no particular order, each point having its own merits:

• Code reviews are not meant to humiliate the submitter, ever. We judge the code, not the submitter. You are not your code.
• Be polite. Say please and thank you.
• It is your professional duty to make it as good as you can.
• Apply the Scientific method to your code reviews (careful observation, measurement-based testing, ...). Don’t assume or guess but prove that the author’s code is right or wrong based on data and “experiments” via tests code and manual testing.
• Do not defend your code because it’s “yours” (it’s not) or “it’s readable to you”. Always try to understand the intent of the suggested changes and apply it.
• If the change suggested by the reviewer doesn’t sound sensible, the submitter of the PR is expected to ping other reviewers for a 2nd opinion.
• No one here knows everything. No one here is John Carmack. It’s fine to say “I don’t know” or “I have no experience with that”. Insecure, defensive behaviour is considered harmful.
• Don’t make people fiddle around for ways to check out and run your PR locally. Add clear, specific instructions.
• Involve testers in the PR review proces by assigning the PR not only to code reviewers (developers) but also to QA/test engineers and/or product owners. Ensure they know how and what to test. This requires a clear a feature/bugfix description, optionally and preferably including screenshots or a URL to a test server running this particular PR. Make sure you include edge cases to test you (as the author) already know about.
• Do not look for a “tone” in written communication. Do not make assumptions about intentions/politics. No one has time for that.
• Never refuse discussion. But also, don’t turn PR reviews into off-topic discussions.
• Don’t add additional features or fixes to your PRs along the way. Create a new issue when you come across a bug or issue not related to your PR (also when the bug is not in “your” code).
• Always keep in mind the business value your code provides and the impact it has on the future.
• Avoid busy-work. If you are missing the context of the task at hand, always question it. “Why do we need it?”, “How does that help us?”.
• Less is more. Do not underestimate future maintenance costs of your code. Always look for places where code can be removed.
• We read code 100x more than we write it, if it takes you 10 more minutes to make sure someone reading the code will spend 10 second less to understand it, it’s worth it.
• Performance is great, but when choosing between simplicity and performance, always err on the side of simplicity. Until you know you have a performance problem, you don’t have one. “First make it work, then make it pretty, then make it fast”.
• Never rush unless absolutely necessary (hotfixes are unavoidable - let’s learn from them).
• Discuss patterns, then decide which one to use. Stick to it.
• PR reviews should be a part of the team culture and we should be proactive in doing that. PRs are part of your job as a coder. Doing code reviews is coding, in a way. Learn to enjoy it. Appreciate what you learn from having your code reviewed.
• The author of a PR should take ownership of the feature / PR, and as owner, take the responsibility to ask other devs on the team for feedback and also have as first priority apply the required changes and make the PR ready before working on a new PR.
• Poking each other (“hey, can you please review X or Y”) is a good step in the right direction to try and promote that culture, it’s easy to forget.
• Ideally, a developer should start a new PR only when there aren’t any loose ends with the most recent PR. To enforce the adoption of this rule, we may consider to set a hard limit for number of PRs that a dev can have open.
• For the rule above to be helpful, the collaboration of all devs in the team is required. If one devs has reached the limit of open PRs, but the team is not reviewing them, they would be indirectly hindering their co-workers and the progress of the team.
• Success (or productivity) is not to be measured by PRs submitted, but by PRs deployed, a feature is not useful unless it’s on production.
• Taking one step further, a total success is when a PR is deployed and it has no bugs or side effects. And the better a PR is reviewed, the higher the odds that there aren’t any issues after the PR deployed
• Use (GitHub) labels or tags (in VSO/VSTS) to make specific request such as “Needs testing”, “Needs design”, “Needs translations”, “Needs copywriting”, et cetera.