Pull requests are part and parcel of a soft engineer’s life unless you work in a horrible place where people just merge the codes to master directly. How a team handles the pull request and code reviews tells a lot about the team’s culture.
Last week we were discussing pull request ethics in our bi-weekly tech retros in Traveloka.com. Some of the points we discussed are, they should have meaning commit message and description and criticism about code should never offend anyone. If your team is relatively new, you can think about setting some ethical standard about pull requests to avoid conflicts and maintain healthy relationships. Here are my two cents about pull request ethics:
- Treat it as a learning opportunity: One of the best ways to improve in programming is learning from the best engineers in the team. Don’t be sad if someone makes a harsh comment about your design choice, variable naming etc, treat it as an opportunity to improve. The best way to learn to code is by experience and good code reviews can really accelerate your growth. If you don’t have anyone in a team who have the ability to examine your code in details and make constructive criticism, you need to think again if you should really work in this team for a long time.
- Criticism are never personal: Don’t take any criticism in code review personally. Your teammate doesn’t have the intention to hurt you, just take it professionally and try to improve. Don’t be afraid to admit your mistake, humility is a great virtue.
- Write meaningful description: Honestly I myself is lazy about this, I am trying to improve this. Code review is often a thankless job, try not to make it hard for the reviewer. Write a meaningful description which should contain what is the scope of your change, what is the problem you are trying to solve. If you are upgrading any library version, mention if clearly including the reason for the upgrade. Try to imagine yourself as the reviewer and try to describe as much as possible.
- Don’t poke someone in the zone for code review: Every software engineer knows how annoying it is when someone breaks the focus. It can take a minimum of 30mins or more to get into the zone and it’s the highest level crime to break someone’s zone for non-urgent reasons. Unless it’s urgent, don’t continuously poke someone for code-review, use a messenger/email to communicate, never disturb someone who is wearing a headphone. Most importantly, know the preference of the reviewer, know how he/she wants to be communicated.
- Don’t break the tests and build: Recently I have annoyed a few fellow engineers by breaking the test cases frequently and I am quite embarrassed about it. Run the test cases before you ask for a review, make sure your code compiles and builds. Try to set up a git pre-hook which will make sure you won’t forget to run the tests.
- Don’t repeat same mistake too many times: If in your every pull request I have to remind you to use proper variable name and you still use a, b, c as variables, may be you are not taking me seriously and as a reviewer I will get very annoyed.
- Don’t ignore the minor issues: Sometimes reviewers approve the pull request with commenting about some minor issues like “add a new line”, “remove space”. You are free to ignore the comments and merge it to master but don’t do it! Why the reviewer approved the pull requests even though there are some issues? Because he or she trusted you that you will fix them before merging to master. If you ignore them, you lose my trust and next time I will wait for you to fix every single tiny issue before I approve.
- Don’t be afraid to express your concern: Don’t like the way the reviewer is criticizing your code? A reviewer is being unnecessarily rude? Express it directly, talk to your team-lead, don’t suffer silently!
That’s all for now. If there is anything your team is doing which is worth sharing, feel free to comment.
+1 from the chosen one 😛