This repository has been archived by the owner on Aug 8, 2023. It is now read-only.
[WORK IN PROGRESS] Code review guide #55
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| Code Review | ||
| =========== | ||
|
|
||
| This is forked/stolen from ^W^W inspired by | ||
| [Thoughtbot's code review guide](https://raw.githubusercontent.com/thoughtbot/guides/master/code-review/README.md) | ||
|
|
||
| A guide for reviewing code and having your code reviewed. | ||
|
|
||
| Why code reviews? | ||
| ----------------- | ||
|
|
||
| Code is reviewed before being merged into `master`. The goal of review is to ensure that changes are delivering business value to our customers - not to perfect our code. | ||
|
|
||
| When reviewing, consider whether a change: | ||
|
|
||
| * Delivers business value. Understand why it is being made and why it is required. | ||
| * Does what it says it does. Try to break it. | ||
| * Can be maintained by the most junior member of our team. | ||
| * Meets our standards and is well written. | ||
|
|
||
| Don't let minor technical issues block shipping. | ||
|
|
||
| A reviewer may give +1 on a review. Small fixes need +1, complex fixes and large features +2. A branch should have at least +1 before you hit the green button. | ||
|
|
||
| Code Review Mindset | ||
| ------------------- | ||
|
|
||
| It's important to separate our self-worth from the code we write so that we are always open to feedback that will help us improve. | ||
|
|
||
| When reviewing code that changes user facing components it is important that reviews play the role of a User Advocate and consider how the change will impact our users, for example: | ||
| * Is the copy written in such a way that it would be understand by someone who has learned English as their second language? | ||
| * Is the change consistent with the user's mental model of our system? | ||
|
|
||
| Everyone | ||
| -------- | ||
|
|
||
| * Accept that many programming decisions are opinions. Discuss tradeoffs | ||
| which you prefer, and reach a resolution quickly. | ||
| * Ask questions; don't make demands. | ||
| * *What do you think about naming this `:user_id`?* | ||
| * Ask for clarification. | ||
| * *I didn't understand. Can you clarify?* | ||
| * Avoid selective ownership of code. | ||
| * Avoid these words: "mine", "not mine", "yours" | ||
| * Avoid using terms that could be seen as referring to personal traits. | ||
| ("dumb", "stupid"). Assume everyone is attractive, intelligent, and | ||
| well-meaning. | ||
| * Be explicit. Remember people don't always understand your intentions online. | ||
| * Be humble. | ||
| * *I'm not sure - let's look it up.* | ||
| * Don't use hyperbole. | ||
| * Avoid "always", "never", "endlessly", "nothing" | ||
| * Don't use sarcasm. | ||
| * Keep it real. If emoji, animated gifs, or humor aren't you, don't force | ||
| them. If they are, use them with aplomb. | ||
| * Talk in person if there are too many "I didn't understand" or "Alternative | ||
| solution:" comments. Post a follow-up comment summarizing offline discussion. | ||
|
|
||
|
There was a problem hiding this comment. Can I suggest also: Make it clear whether your comment is blocking (you think it needs to be addressed before you grant a +1) or non-blocking (it's a suggestion or observation that the author can choose to implement or not). Without this distinction, it's not clear to the author what she needs to do to gain a +1. |
||
| Having Your Code Reviewed | ||
| ------------------------- | ||
|
|
||
| * Be grateful for the reviewer's suggestions. | ||
| * *Good call. I'll make that change.* | ||
| * Don't take it personally. The review is of the code, not you. | ||
|
There was a problem hiding this comment. I humbly suggest that this is far, far easier to write than to follow. To be blunt: it feels somewhat patronizing to say to someone "here is some criticism of something you worked long and hard on - don't take this as criticism". The problem (which is not unique to code review, but comes up in most human interactions) is that we don't make a rational decision to feel attacked or criticized: we just feel that way. It's not under the control of the person receiving the comments. So telling someone "don't take it personally" is rather like saying "don't feel pain when I punch you in the face". That doesn't mean that we can't make comments: it just means we need to be aware that people's self-worth is invested in what they create (or it damn well should be if we want passionate people to care about what they do). |
||
| * Explain why the code exists. | ||
| * *It's like that because of these reasons. Would it be more clear if I rename this class/file/method/variable?* | ||
| * Extract some changes and refactorings into future tickets/stories. | ||
| * Link to the code review from the ticket/story. | ||
| * *Ready for review: https://github.com/organization/project/pull/1* | ||
| * Push commits based on earlier rounds of feedback as isolated commits to the | ||
| branch. Do not squash until the branch is ready to merge. Reviewers should be | ||
| able to read individual updates based on their earlier feedback. | ||
| * Seek to understand the reviewer's perspective. | ||
| * Try to respond to every comment. | ||
| * Wait to merge the branch until Continuous Integration (CircleCI) | ||
| tells you the test suite is green in the branch. | ||
| * Merge once you feel confident in the code and its impact on the project. | ||
|
|
||
| Reviewing Code | ||
| -------------- | ||
|
|
||
| Understand why the code is necessary (bug, user experience, refactoring). Then: | ||
|
|
||
| * Communicate which ideas you feel strongly about and those you don't. | ||
|
There was a problem hiding this comment. Is this in line with my comment about non-blocking and blocking? Is it better to be explicit, rather than "I feel strongly"? |
||
| * Identify ways to simplify the code while still solving the problem. | ||
| * If discussions threaten progress on the PR then move it offline. Usually | ||
| disagreement can be resolved quickly if the involved parties discuss things in | ||
| front of a whiteboard. | ||
|
|
||
| * Offer alternative implementations, but assume the author already considered them. | ||
| * *What do you think about using a custom validator here?* | ||
| * Seek to understand the author's perspective. | ||
| * Sign off on the pull request with a :thumbsup: or "Ready to merge" comment. | ||
|
|
||
| Style Comments | ||
| -------------- | ||
|
|
||
| Reviewers should comment on missed style guidelines. Ideally you should add | ||
| a link to the relevant section in the style guide. That way you don't have to | ||
| repeat the reasoning of a particular guideline in your comment. | ||
|
|
||
| Example comment (markdown): | ||
|
|
||
| [Return early when possible](https://github.com/mobify/mobify-code-style/tree/master/javascript#return-early-when-possible) | ||
|
|
||
| Which renders as: | ||
|
|
||
| > [Return early when possible](https://github.com/mobify/mobify-code-style/tree/master/javascript#return-early-when-possible) | ||
|
|
||
| An example response to style comments: | ||
|
|
||
| Whoops. Good catch, thanks. Fixed in a4994ec. | ||
|
|
||
| If you disagree with a guideline, open an issue on the guides repo rather than | ||
| debating it within the code review. In the meantime, apply the guideline. | ||
|
|
||
| Reading material | ||
| ---------------- | ||
| * http://blogs.atlassian.com/2009/11/code_review_in_agile_teams_part_i/ | ||
| * https://blogs.atlassian.com/2010/03/code_review_in_agile_teams_part_ii/ | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be a paraphrasing of a main point from Weinberg's Egoless Programming (1971). Jeff "CodingHorror" Atwood extracts 10 "commandments" here. I think it's an extreme oversimplification to extract just this one principle and omit others. In particular, the point summarized in the tenth commandment is:
On the other hand, Atwood also mentions how egoless programming has been considered a fallacy or myth. The tl;dr here is that we want people to be personally invested in (to own) their work: we want their passion, commitment:
So the paradox of peer review is that we expect developers to own/invest/care deeply for the work they do, right up until the point that their work is reviewed, when they should magically adopt a zen-like disinterest in having it reviewed. I submit that this is unreasonable, unless we're very careful about how reviews are done, and what reviewing is.
Karl Wiegers wrote an excellent book in 2001 called Peer Reviews in Software: A Practical Guide which looked at the psychological aspects of peer review. Chapter two is available online, and here's a relevant quote (my emphasis):
The reason that I quote these at this point is because where I have in the past seen code reviews go quite spectacularly wrong, it's been because of (1) a lack of attention to these points of personal interaction, and (2) reviews focusing on subjective attributes of code style or system design. The two factors multiply together and have bad effects:
Above in this document, the statement is made that The goal of review is to ensure that changes are delivering business value to our customers - not to perfect our code. For me, this (excellent principle) means that reviews are intended to spot defects and ensure that code meets the business-value requirements: and in particular, that subjective opinions about how code should be written are not in agreement with that goal.
tl;dr - I don't think we pay nearly enough attention to the personal interaction aspects of code review, and subjective opinions exacerbate this problem because they change discussion into argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @benlast. That's excellent feedback and I appreciate the thoughtfulness and the comprehensive references. I'm far less eloquent in providing my own thoughts but hope that I am able to phrase them appropriately.
I definitely agree with your point that the personal interaction is the most important part of the review and will most likely decide the tone and perception of the feedback provided. I think it is something that we should clarify in the list of bullet points above. We should make it clear that reviewing a person's code means criticizing their work and each of us handles criticism differently. This will have to be an improvement process for both the reviewer and reviewee in which they work together rather than compete. That's at least the thought that I have.
Do you have any suggestions on how to clarify this in the above guidelines?
I also think you have a valid regarding the subjective opinion on how code should be written. I think a code review should mainly be about its suitability to the problem it is trying to solve and the pros and cons of the chosen implementation over alternatives. What I am not quite able to gather from your comment is, what your thoughts are on qualitative measures of code such as maintainability and best practices? Taking Python as an example, the community considers certain patterns Pythonic and others not. Would you consider these as subjective opinions?
I personally think that the line around where subjective opinion starts or ends is quite blurry. And I personally think maintainability and - in the case of Python - writing idiomatic Python are important parts of a code review. However, I think the extend to which they factor into a code review is to be determined by the group of people carrying out the reviews between themselves.
And as such, I think developing guidelines that we as Mobifiers agree on as best practice will make it clearer what is considered objective or subjective. Which is the reason why I'd like to see us all work together on developing these guidelines and establishing them across our various code bases.
Overall, I state my position on subjective opinion on code and code style along the lines of the last section:
I think we should establish some guideline that we all agree on. Until they are established, we apply the ones that we have.
Does that make sense to you and how does that overlap with your position and thoughts? How do you think should we change this suggested document for you to be comfortable with it?
Also thanks for the references above, I'll definitely add them to my reading list. Especially the Peer Reviews in Software sounds pretty interesting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Seb. That's a great response: thank you!
On "a code review should mainly be about its suitability to the problem it is trying to solve and the pros and cons of the chosen implementation over alternatives" - I 100% agree completely with this statement.
On "developing guidelines that we as Mobifiers agree on as best practice will make it clearer what is considered objective or subjective" - I also agree with this, with a small caveat. The more rules we add, the more complex the process of following those rules becomes. @dbader and I have been using PyLint and ESLint on the web-push code, and it drastically simplifies the process of having code syntactically compliant. I'd summarize it as if the linter complains, fix it, otherwise it doesn't matter.
Sometimes code reviews frustrate me. For instance, where the process of getting working code rolled out to production is blocked by an apparently endless series of requests for minor tweaks that demonstrably have no bearing on whether the code is a reasonable solution to the problem. Or at other times (as we've seen), the effort that should go into attempting to find actual issues is instead diverted into arguments about whether one style or another is more "readable", as though readability was not subjective. Reviews like that, at their worst, damage relations between developers, affect morale and reduce our productivity.
Lastly: "We should make it clear that reviewing a person's code means criticizing their work...". I don't agree with this, because it shouldn't be about criticizing. Reviewers help the author find defects in the code. That's not criticizing, it's assisting, providing an alternative point of view, asking for clarification. It's extremely difficult to criticize someone's else's work in a way that you can be sure won't have a negative effect. It takes time, care and effort to follow rules (e.g. phrase your observations as questions, not orders, start comments with "I feel", and so on). Better to adopt the position that reviewing is helping, not correcting.