It's been often said that programming is part art, part science - that because lots of times there's no single, simple solution to a problem; or if there is, we might not know about it. There's also an infamous joke that if there are n developers in the room, then there are n+1 opinions on how things should be done. That being said, here are some guidelines that should prevent friction when submitting or reviewing code.
The code has to work. Unless you open a PR as a work in progress, the code should be built and tested on a device or emulator.
If you have touched the gradle build files and changed build setup, it's useful to test the whole build from scratch (clean build) and all of the types and flavours. If you have touched payments (logic or UI), you should test that it still works correctly, both in test and production builds. If you updated external libraries, test the pertaining features (e.g. if you updated the Facebook library, test that FB login, logout, sharing and inviting friends features work as expected). If you changed the build version, make a build and test that the version is correct.
Having people review your code is one thing, but you should not expect them to also test the code for you.
One important thing that lots of guidelines forget to mention is the context of the pull request: sometimes it's a big refactor, sometimes it a new feature, sometimes it's a bug fix. Some of those might be more urgent than the others, and sometimes you might be under pressure to ship as soon as possible so the code might not be perfect or there won't be any tests or code might not be extendable. That's ok.
- There is no perfect code: good enough is usually good enough. That being said, try to keep the number of WTFs per minute to a minimum.
- Accept that many programming decisions are opinions. Discuss tradeoffs, which you prefer, and reach a resolution quickly.
- Ask for clarification. ("I didn't understand. Can you clarify?")
- Offer clarification, explain the decisions you made to reach a solution in question.
- Avoid using terms that could be seen as referring to personal traits. ("dumb", "stupid"). Assume everyone is intelligent and well-meaning.
- Be humble. ("I'm not sure - let's look it up.")
- Don't use hyperbole. ("always", "never", "endlessly", "nothing") Don't use sarcasm.
- Remember that you're both on the same side - the goal is to make the code better. Understand that sometimes your ideas will be overruled. Even if you do turn out to be right, don't take revenge or say, "I told you so".
- Talk synchronously (e.g. chat, screensharing, in person) if there are too many "I didn't understand" or "Alternative solution:" comments. Pull requests should not be the place for long discussions, architectural or otherwise.
- Put notes on what's missing or could be improved in the PR description or comments. You can also make a Trello card with discussions points and possible problems or things to do and discuss it offline.
- PRs should be about one thing. If you do multiple things in one PR, it's hard to review. If you're fixing stuff as you go, you might want to make atomic commits and then cherry-pick those commits into separate branches, leaving the PR clean.
- Try to keep the PRs small. There has been some research to indicate that beyond 400 LOC the ability to detect defects diminishes. (We're talking about LOC proper, unit tests and layouts don't count)
- Having a PR description is useful. Additionally, you can also link to the card on Trello.
- Ideally, the PR should be finished when submitted. If the PR is work in progress, add (WIP) or [WIP] to the title.
- You should have tests that at least cover the logic, and ideally also cover the input/output parameters and methods. (depends on context)
- Reviewing code is part of a normal workday. You should check for open/updated PRs at least once a day.
- Ask, don’t tell. (“What do you think about trying…?” rather than “Don’t do…”)
- Offer ways to simplify or improve code.
- Code beautification and refactoring ought to be tasks in the next sprint, except for obvious and easy-to-fix things.
- Communicate which ideas you feel strongly about and those you don't. Explain your reasons why code should be changed. (Not in line with the style guide? A personal preference?)
- If you disagree strongly, consider giving it a few minutes before responding; think before you react.
- Offer alternative implementations, but assume the author already considered them. ("What do you think about using a custom validator here?")
- If discussions turn too theoretical or touch big architectural questions, move the discussion offline. In the meantime, let the author make the final decision on alternative implementations.
- Don't keep the code hostage. Keep in mind the context and the most important thing - does it work?
- Try to respond to every comment. Be grateful for the reviewer's suggestions. ("Good call. I'll make that change.")
- Be aware of how hard it is to convey emotion online and how easy it is to misinterpret feedback. If a review seems aggressive or angry or otherwise personal, consider if it is intended to be read that way and ask the person for clarification of intent, in person if possible.
- Link to any follow up commits or Pull Requests. (“Good call! Done in 1682851”)
- Wait to merge the branch until Continuous Integration tells you the test suite is green in the branch. Merge once you feel confident in the code and its impact on the project.
Mostly this makes sense to me and is in line with how I typically try to review and submit PRs. Just a couple of comments
I think our guidelines should reflect best practices. This sentence suggests that it's fine to write bad, untested code. If this does happen it should be an exception to the rule and not the rule. i.e. if you are under pressure you should try to write good, tested code.
Adding non-production ready code to the code base increases workload for the future and is, more often than not, never done. This adds to a future developer being finding a task difficult, begin under pressure and perhaps accepting that it's ok to write sub-par code. The cycle continues
Of course there is always exceptions to the rule, if it is necessary to ignore the guidelines it should be discussed and agreed with the reviewer. If it's a regular occurrence then there is something broken in the workflow, that issue should be raised with the team.
I don't think this is a necessity and I haven't experienced it in a number of dev teams that I have worked with. Of course disagreements will come about but consensus building should take precedent e.g.
- Looking at pros and cons of both arguments
- Speak with other members of the team both android and other developers that might have an understanding of the problem
- Look at alternatives to both ideas
Dangers of "overruling" others are
- Team members have less ownership of the codebase
- Team members will be less inclinded to suggest ideas or feedback which will affect the codebase negatively as well as inhibit learning from all members of the team
- It can build resentment across the team
- It propogates the idea of some peoples ideas are worth more than others
- It propogates the idea of a hierachical structure in a team of 3. This will harm STMs ability to retain and attract good talent
It's typically the responsibility of the submitter to get the PR merged. It's difficult for a reviewer to be the driver as they don't always know where abouts the PR is upto. If you are waiting for a review or a response, it's not a problem to ping the android-development channel or talk to a reviewer.
Additions For Submitters
Extra notes
It looks like this proposal suggests some changes to how PRs are submitted and for the better. We should probably expect some PRs being submitted that don't follow the guidelines so it would make sense for us all to keep note and suggest how a PR could have been constructed in a better way.