From 35c26478cb82f55681a571c9782cad3cb7434bad Mon Sep 17 00:00:00 2001 From: Daniel Bader Date: Tue, 21 Apr 2015 15:19:05 -0700 Subject: [PATCH 1/7] Code review guide initial commit --- code-review/README.md | 79 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 79 insertions(+) create mode 100644 code-review/README.md diff --git a/code-review/README.md b/code-review/README.md new file mode 100644 index 0000000..e2d4e80 --- /dev/null +++ b/code-review/README.md @@ -0,0 +1,79 @@ +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. + +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. ("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. ("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. + +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. +* 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 (TDDium, TravisCI, etc.) + 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. +* Identify ways to simplify the code while still solving the problem. +* If discussions turn too philosophical or academic, move the discussion offline + to a regular Friday afternoon technique discussion. In the meantime, let the + author make the final decision on alternative implementations. +* 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](../style) +guidelines. Example comment: + + [Style](../style): + + > Order resourceful routes alphabetically by name. + +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. From f143d709c4d84fbe6a35551b9214acd05783c610 Mon Sep 17 00:00:00 2001 From: Daniel Bader Date: Tue, 21 Apr 2015 15:22:41 -0700 Subject: [PATCH 2/7] We don't have regular Friday tech reviews :) --- code-review/README.md | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/code-review/README.md b/code-review/README.md index e2d4e80..16eddea 100644 --- a/code-review/README.md +++ b/code-review/README.md @@ -53,9 +53,8 @@ Understand why the code is necessary (bug, user experience, refactoring). Then: * Communicate which ideas you feel strongly about and those you don't. * Identify ways to simplify the code while still solving the problem. -* If discussions turn too philosophical or academic, move the discussion offline - to a regular Friday afternoon technique discussion. In the meantime, let the - author make the final decision on alternative implementations. +* If discussions turn too philosophical or academic, move the discussion offline. + In the meantime, let the author make the final decision on alternative implementations. * 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. From 55c2f87d2f7c7ccdae7c762255c2e68909b35498 Mon Sep 17 00:00:00 2001 From: Daniel Bader Date: Tue, 21 Apr 2015 15:33:36 -0700 Subject: [PATCH 3/7] Tweak the code review guide --- code-review/README.md | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/code-review/README.md b/code-review/README.md index 16eddea..e68a5ab 100644 --- a/code-review/README.md +++ b/code-review/README.md @@ -53,8 +53,9 @@ Understand why the code is necessary (bug, user experience, refactoring). Then: * Communicate which ideas you feel strongly about and those you don't. * Identify ways to simplify the code while still solving the problem. -* If discussions turn too philosophical or academic, move the discussion offline. - In the meantime, let the author make the final decision on alternative implementations. +* 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. @@ -63,12 +64,13 @@ Understand why the code is necessary (bug, user experience, refactoring). Then: Style Comments -------------- -Reviewers should comment on missed [style](../style) -guidelines. Example comment: +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. - [Style](../style): +Example comment: - > Order resourceful routes alphabetically by name. + [Return early when possible](https://github.com/mobify/mobify-code-style/tree/master/javascript#return-early-when-possible) An example response to style comments: From 5dee0f8965d71c307070c1ee0eba9cce9963c9a6 Mon Sep 17 00:00:00 2001 From: Daniel Bader Date: Tue, 21 Apr 2015 16:06:28 -0700 Subject: [PATCH 4/7] Clarify the section on style comments --- code-review/README.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/code-review/README.md b/code-review/README.md index e68a5ab..548ac6e 100644 --- a/code-review/README.md +++ b/code-review/README.md @@ -68,9 +68,13 @@ 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: +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: From 18c328500f4881111a46bb5c0f4ee56c8feca288 Mon Sep 17 00:00:00 2001 From: Daniel Bader Date: Tue, 21 Apr 2015 16:29:38 -0700 Subject: [PATCH 5/7] Include things already mentioned on the Dev Process Confluence page --- code-review/README.md | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/code-review/README.md b/code-review/README.md index 548ac6e..da9e3ca 100644 --- a/code-review/README.md +++ b/code-review/README.md @@ -6,6 +6,29 @@ This is forked/stolen from ^W^W inspired by 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. + +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. + +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 -------- @@ -82,3 +105,8 @@ An example response to style comments: 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/ From 8d8a8837f6c41cf95d239f89e2c82ef08b1ae1de Mon Sep 17 00:00:00 2001 From: Daniel Bader Date: Tue, 21 Apr 2015 16:46:58 -0700 Subject: [PATCH 6/7] Update Code Review Guide --- code-review/README.md | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/code-review/README.md b/code-review/README.md index da9e3ca..137a007 100644 --- a/code-review/README.md +++ b/code-review/README.md @@ -8,6 +8,7 @@ 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: @@ -21,13 +22,14 @@ 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. -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. +Code Review Mindset +------------------- -Is the copy written in such a way that it would be understand by someone who has learned English as their second language? +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. -Is the change consistent with the user's mental model of our system? +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 -------- @@ -52,20 +54,20 @@ Everyone Having Your Code Reviewed ------------------------- -* Be grateful for the reviewer's suggestions. ("Good call. I'll make that - change.") +* 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. -* 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?") +* 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") +* 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 (TDDium, TravisCI, etc.) +* 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. From 47517b61ce5979b56d006879cdaff6a4a7620bcc Mon Sep 17 00:00:00 2001 From: Daniel Bader Date: Tue, 21 Apr 2015 16:50:48 -0700 Subject: [PATCH 7/7] Update Code Review Guide --- code-review/README.md | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/code-review/README.md b/code-review/README.md index 137a007..f78bbd1 100644 --- a/code-review/README.md +++ b/code-review/README.md @@ -34,20 +34,25 @@ When reviewing code that changes user facing components it is important that rev 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. ("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. +* 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. ("always", "never", "endlessly", "nothing") +* 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. +* 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. @@ -81,8 +86,8 @@ Understand why the code is necessary (bug, user experience, refactoring). Then: * 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?") +* 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.