New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
What makes a good code review? #5
Comments
You don't need comments everywhere but if there are any places that you have to think too long about what's going on, ask for a comment. |
I've seen during refactors or new projects moving rapidly fast that there can be dead code or unused code that may be left in. |
"Does this variable name make sense?" A real basic example: |
"Does this function call account for all possible inputs/ outputs/ errors" |
"Is it possible to generalize some logic for extensibility" |
An incomplete list: High-level questions:
Low-level questions:
|
If we are answering the question of "what makes a good code review" vs "what makes good code" then here are some thoughts:
|
I see the best code reviews providing two types of benefits: a second set of eyes for finding errors and a learning opportunity. Good reviews catch incorrect code -- subtleties missed by the original author. These might be getting business logic slightly wrong, missing a security or performance concern, misusing a library, off-by-one errors, etc. etc. Unit tests catch some of this, but I think that, from the perspective of a project, this is the primary use for code reviews. More than that, though, good reviews offer suggestions on making working code better. These could take the form of pointing out difficult to follow reasoning, alerting of other libraries which might help, suggesting certain patterns and avoiding anti-patterns, using more concise style when it makes sense, and so forth. I think that these types of comments are the most helpful for our growth as developers -- they hone our craft through constructive criticism and commentary. They are, however, harder (if not impossible) to validate. |
big +1 to what @cmc333333 wrote. |
@dlapiduz can you expand on
|
@cmc333333 I think that there are some code reviews that come up every time you do a PR and prevent you/discourage you from sending more PRs... I don't think this is easy to watch at an individual layer but more at an aggregate one... |
I would like to expand upon the second part of @cmc333333's comment above ("More than that..."). At the recommendation of @baccigalupi, I re-watched this excellent talk on cultivating a code review culture: https://www.youtube.com/watch?v=PJjmw9TRB7s (shout out to @derekprior for doing a In the talk, Derek cites research that shows that people think code review is about finding bugs, but really we don't find many bugs while doing code review. But that's OK! Because there are other, more important benefits to having a strong code review culture:
These are along the lines of what @cmc333333 said above. Beyond having a shared vision for the goals of code review, having a good process in place only works if everyone in the organization is fully participating in code review. Derek provides two interesting high-level tips for cultivating a code review culture. The first is for the author: provide lots of context about the PR in the PR itself. Don't make reviewers hunt through GH issues or Pivotal Tracker. Don't make them look at a comment thread on a Trello card. Explain to the reviewers the why of the code so they can understand its purpose and suggest reasonable alternatives. The second is for reviewers: ask, don't tell. What this really means is "take special care to be nice." Getting critical feedback is hard, and it's even harder when it comes via GItHub where it's impossible to read a person's tone. An antidote to coming off as cold is for reviewers to pose their input as questions to start a discussion rather than commands. In the final part of Derek's talk, he covers three important topics that arise once you are actually doing code review: disagreements, what to review, and style. Disagreements: these are inevitable. And they are good. Disagreements mean people are engaged, which means they are learning. Once code has reached a certain threshold of quality, any disagreement you have on a PR is going to be about tradeoffs. At this point, it's fine for people to agree to disagree. Following the tip for reviewers above should help keep the tone of the conversation positive even when two people disagree. What to review: we each bring our own strengths to the table when doing a code review. Some people care a lot about naming, others about removing duplication or test coverage or reducing complexity. The goal shouldn't be for any org to come up with a list of things to look for when doing code review; the goal is for each individual person to bring their own strengths and interests to the table when doing a code review. And, over time, through expressing thoughts on the topics of interest that interest and related knowledge will be transferred to the rest of the team. Style: this is where @ertzeid and the Code Style Guide working group come in. Style is important in the context of a code review. But people who review based on style are perceived negatively. So it's important to have automated tools to detect code style guides AND to write down preferred styles in a place outside of a PR so that the conversation can happen independently. In summary: What makes a good code review is a code review that achieves the three goals listed at the beginning of this commentessay:
My feeling is that people will only participate in a code review if they enjoy code review. I covered a lot in this comment because I felt it was all interesting food for thought on the topic of code review. Perhaps someone can give me some feedback on how to summarize all of these thoughts into something more digestible? |
The goals of a code review are to ensure:
Everything we do in a code review should derive from those. I encourage teams to create a checklist of things to check in a code review, as it's easy to forget to look for things like security issues. Here's an example from one team I worked with. However, you have to be careful to keep the focus on the high level goals, and not get stuck in the weeds. And everyone gets stuck in the weeds — especially on style issues, which is why having automatic style checking is definitely preferred. If I were to write such a checklist today, I'd have the 3 goals at the top, in large bold letters. Then I'd try to find some resources with good suggestions on how to do good code reviews, including this page. Then I'd have the team decide which ideas will be most effective at achieving the 3 goals. |
This is a great conversation. I'd especially like to emphasize some of @jessieay's points. A code review should be a conversation, not an argument. You shouldn't generally be telling someone that they need to make changes. Instead, ask questions and make suggestions. Try to get a better understanding of the trade-offs that were involved in their thinking. If you think there's a better way, ask if they considered it, and why they chose the way they did. The code review is not a one-way conversation — the code reviewer should be learning as much as teaching. Thanks for sharing this development guide with the community. |
Another element: constructive tone on all sides. @ohrite's "Better Code Review" slides include some useful suggestions and go deeper than the surface: https://speakerdeck.com/ohrite/better-code-review |
@fureigh I think you're the last person here who was in on this conversation at the time. Do you think the thoughts were captured in the code review section? Is there additional work to do, or can we close this? |
I'd say we can close. This was opened to spark a discussion and the resulting guides content on code review can be added to if anyone sees anything missing. |
Squashed commit of the following: commit 4d63ae3 Merge: 45ec357 6b0f565 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 14:33:15 2020 -0700 Merge pull request #29 from 18F/rmh-gitattributes-glob Fix glob used in .gitattributes commit 6b0f565 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 14:29:34 2020 -0700 Fix glob used in .gitattributes .gitattributes patterns use same convention as .gitignore, where trailing `/**` is used to match everything inside a directory. commit 45ec357 Merge: cf3e0a1 849e5c3 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 10:25:42 2020 -0700 Merge pull request #27 from 18F/rmh-add-ci-cache Add dependency caching in CircleCI commit 849e5c3 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 10:15:56 2020 -0700 Add dependency caching in CircleCI commit cf3e0a1 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 09:18:09 2020 -0700 Remove remaining next steps and migrate to issues commit bb58c7f Merge: c06c47d 2c6f1c0 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 09:11:12 2020 -0700 Merge pull request #21 from 18F/rmh-ci-tests Add tests that get run in CircleCI commit 2c6f1c0 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 08:54:40 2020 -0700 Added missing commas commit 0c691f1 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 08:52:23 2020 -0700 Add ignore URLs from application.js commit 871dd08 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 08:08:02 2020 -0700 Validate HTML, check internal links in CircleCI Adopting approach used in 18F/before-you-ship commit c06c47d Merge: 8b0257d db465de Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 08:03:07 2020 -0700 Merge pull request #20 from 18F/rmh-improve-example Improve example guide commit db465de Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 07:50:11 2020 -0700 Improve example guide - Use structure from ux-guide index to nudge folks in good direction - Show pages under a topic directory to show automatic URL generation - Use real links in example navigation.yml so HTMLProofer will not error commit 8b0257d Merge: d25c1ff f9e166f Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 07:46:29 2020 -0700 Merge pull request #19 from 18F/rmh-syntax-error-footer Remove stray Liquid end tags commit f9e166f Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 11 07:40:35 2020 -0700 Remove stray Liquid end tags commit d25c1ff Merge: 7fb6bf9 a2c101e Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Thu Sep 10 15:38:21 2020 -0700 Merge pull request #15 from 18F/rmh-config-federalist Support _guide/_config.yml, previews, etc. on Federalist commit a2c101e Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Thu Sep 10 15:22:35 2020 -0700 snake_case consistency commit c5156f3 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Thu Sep 10 13:55:52 2020 -0700 Remove last_updated, not supported by Federalist Federalist only does a shallow clone of repo, so last_updated does not work. Remove from example anchor.yml. See: https://gsa-tts.slack.com/archives/C1NUUGTT5/p1565804388080700 commit b9f7b77 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Thu Sep 10 13:44:07 2020 -0700 Make SVG paths relative for Federalist previews commit c83fc9e Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Thu Sep 10 13:24:59 2020 -0700 Clean up code, add comments commit 57b1551 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Tue Sep 8 14:10:28 2020 -0700 Support _guide/_config.yml on Federalist Federalist doesn't support multiple Jekyll config files, so load _guide/_config.yml in the generator. commit 7fb6bf9 Merge: 20fbb7a d1fe837 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Tue Sep 8 13:16:54 2020 -0700 Merge pull request #14 from 18F/rmh-271-rubyver Explicitly tell Federalist ruby/bundler versions commit d1fe837 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Tue Sep 8 13:06:25 2020 -0700 Explicitly tell Federalist ruby/bundler versions See: - https://federalist.18f.gov/documentation/rvm-on-federalist/ - https://federalist.18f.gov/documentation/bundler-on-federalist/ commit 20fbb7a Merge: 986aabd 0b2671c Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Tue Sep 8 11:12:09 2020 -0700 Merge pull request #12 from 18F/rmh-specify-ruby-ver Explicitly set ruby version to use commit 0b2671c Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Tue Sep 8 11:07:00 2020 -0700 Explicitly set ruby version to use - Bundler already included with 2.7.1 image - Lock to specific version of ruby to prevent any unexpected errors from appearing out of blue in dowstream guides (bundler versions, etc.) commit 986aabd Merge: a50bbe6 f7e765c Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 16:54:22 2020 -0700 Merge pull request #11 from 18F/rmh-allow-unrelated Allow unrelated histories for GitHub template commit f7e765c Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 16:52:34 2020 -0700 Allow unrelated histories for GitHub template When downstream repository created from GitHub's Use this template feature, the repos won't have a common history like in a fork. commit a50bbe6 Merge: 83a66e2 c6e4dac Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 16:29:03 2020 -0700 Merge pull request #10 from 18F/rmh-remove-master-refs Fix action to pull from default branch commit c6e4dac Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 16:27:13 2020 -0700 Fix action to pull from default branch Just use default branch, remove master references. Also fix references to renamed paths that represent guide-specific files. commit 83a66e2 Merge: 8d87fd0 4e69acf Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 15:07:21 2020 -0700 Merge pull request #9 from 18F/rmh-add-tts-orgs Add anchor data for other TTS orgs commit 4e69acf Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 15:05:55 2020 -0700 Update docs commit 66ae3a1 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 15:04:37 2020 -0700 Add anchor data for other TTS orgs Make secondary logo optional to accomodate TTS Solutions commit 8d87fd0 Merge: 73b1e5b 0aa8377 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 13:47:55 2020 -0700 Merge pull request #8 from 18F/rmh-nest-under-guide Nest all customizable files under _guide commit 0aa8377 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 13:44:06 2020 -0700 Update with revised paths, filenames commit 4e0c147 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 13:32:16 2020 -0700 Structure loops for easier log comprehension Putting all the org-specific merges in the output first makes it a little easier to understand what is geting overriden by what. commit b64f1ef Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 13:18:56 2020 -0700 Update to match new paths, suborgs -> orgs change commit 91cc670 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 13:10:55 2020 -0700 Removing soft-link to resolve watch error Soft-link was an attempt to allow guide-specific data files to reside under _guide, but still be loaded into the site.data hiearchy. However, that results in this error: ** ERROR: directory is already being watched! ** commit 599cc22 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 13:02:23 2020 -0700 Move files around to simplify edits downstream commit 73b1e5b Merge: 4160ec2 718c0a7 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 11:44:09 2020 -0700 Merge pull request #7 from 18F/rmh-exclude-docker Exclude Docker files from _site commit 718c0a7 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 11:43:21 2020 -0700 Exclude Docker files from _site commit 4160ec2 Merge: fc9c4ff abd9a39 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 11:42:01 2020 -0700 Merge pull request #6 from 18F/rmh-cleanup-assets Move images used by template to assets folder commit abd9a39 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 11:40:04 2020 -0700 Fix images URLs. Add Slack URL example. commit 34dc2e6 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Sep 4 11:29:05 2020 -0700 Remove unused images, move rest under asssets commit fc9c4ff Merge: 3b174c6 218cd29 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Mon Aug 31 19:37:57 2020 -0700 Merge pull request #5 from 18F/rmh-tweak-anchor-api Tweak the footer/anchor API commit 218cd29 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Mon Aug 31 19:35:07 2020 -0700 Tweak the footer/anchor API - Restore the ability to display the standard uswds-jekyll footer - Explicitly turn the standard footer off - Structure the anchor.yml edit_page key like the footer.yml one - Disable the anchor last_updated, edit_page if footer enabled commit 3b174c6 Merge: 159c419 da2b1d2 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Mon Aug 31 19:26:01 2020 -0700 Merge pull request #4 from 18F/rmh-rename-anchor Rename usa_anchor.yml to just anchor.yml commit da2b1d2 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Mon Aug 31 19:25:10 2020 -0700 Rename usa_anchor.yml to just anchor.yml commit 159c419 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Mon Aug 31 13:38:28 2020 -0700 Note that usa_anchor.yml can now be overriden commit 24ad83f Merge: 4aeb2ea 369dcae Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Mon Aug 31 13:36:54 2020 -0700 Merge pull request #3 from 18F/rmh-page-metadata Add date last updated and edit page URL to anchor commit 369dcae Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Mon Aug 31 13:34:03 2020 -0700 Add date last updated and edit page URL to anchor Re-implement the last_updated and edit_page features from the uswds-jekyll footer into the new footer/anchor. commit 06228b5 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Mon Aug 31 13:28:57 2020 -0700 Add empty main.js to satisfy build exception Without main.js, this exception is thrown during the Jekyll build: Errno::ENOENT: No such file or directory - isildurs-bane/assets/js/main.js does not exist! The root cause is not clear at this time – it's present in the sample uswds-jekyll site. commit 8f94dc4 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Mon Aug 31 13:27:25 2020 -0700 Add date last updated and edit page URL to anchor Re-implement the last_updated and edit_page features from the uswds-jekyll footer into the new footer/anchor. commit 4aeb2ea Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Mon Aug 31 11:10:05 2020 -0700 Update README.md commit 9321479 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Mon Aug 31 09:05:00 2020 -0700 Update README.md commit 77b8aec Merge: 1521138 a854135 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 19:20:23 2020 -0700 Merge pull request #2 from 18F/rmh-add-private-eye Add private-eye commit a854135 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 19:14:28 2020 -0700 Add private-eye Provenance of changes: - private domains/URLs from 18F/handbook (https://github.com/18F/handbook/blob/master/javascripts/application.js) - defaultMessage from 18F/ux-guide (18F/ux-guide#176) - code from 18F/private-eye (https://github.com/18F/private-eye/releases/tag/v2.0.0) The code almost never changes, so skip npm/package.json and just duplicate as other handbooks have done. commit 1521138 Merge: 6bfa20e 3020dc6 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 14:52:56 2020 -0700 Merge pull request #1 from 18F/rmh-fix-footer Fix and generalize footer from 18F/ux-guide commit 3020dc6 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 14:46:39 2020 -0700 Add support for sub-organization specific themes By default website will just be affiliated with GSA/TTS. If you set the suborg key in override.yml to 18F, 18F branding will be used in the footer. Other sub-organizations can be added in similar fashion. Configuration is read in this order, last wins: - _config.yml - override.yml - YAML directly in _data - YAML in _data/suborgs/[18F, etc.] - YAML in _data/override commit bcf8177 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 14:35:20 2020 -0700 Use site.title instead of anchor.site_name commit d5c152d Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 14:05:58 2020 -0700 Fix path for footer data commit deab660 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 13:47:21 2020 -0700 Remove unnecessary nested structure Removing nested structure allows us to override individual keys more easily and is consistent with other YAML files. commit 01bbc69 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 13:37:40 2020 -0700 Remove unused keys, make secondary bio consistent commit 031fbe3 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 13:27:16 2020 -0700 Fix secondary org references Optionally include secondary bio, logo, and org. commit 6bfa20e Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 12:35:45 2020 -0700 Add 404 page from 18F/ux-guide commit a51536f Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 12:16:35 2020 -0700 Add additional discussion commit 8fbf785 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 10:56:31 2020 -0700 Highlight primary section in nav when on subpage Reusing fix from 18F/user-guide: 18F/ux-guide@044a5b6 Assumes that your primary nav defines directories as section hrefs and your pages have sidenavs with URLs nested under those directories. commit d4ed30b Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 09:18:29 2020 -0700 Turn on sticky sidenav for example page commit bf1f9df Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 08:56:30 2020 -0700 Footer (usa_anchor) only works with post layout The new footer copied from 18F/ux-guide only works with the post layout. Not quite sure of intent, but related to a uswds-jekyll 5.0 change: 18F/uswds-jekyll@7e663f0 18F/uswds-jekyll#201 commit 286de9a Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 08:56:00 2020 -0700 Add ability to override usa_anchor.yaml commit 677ae08 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Fri Aug 28 08:49:04 2020 -0700 Reusing styling from 18F/ux-guide Reusing styling from 18F/ux-guide, which at least partially reused code from 18F/federalist-jekyll-uswds-18f-port. commit 7780cf5 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Wed Aug 26 10:34:29 2020 -0700 Add note about GitHub setting commit bdab361 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Wed Aug 26 10:32:14 2020 -0700 Adding discussion links commit b84ca35 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Wed Aug 26 10:27:28 2020 -0700 Remove debug output commit def5ee7 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Wed Aug 26 10:27:01 2020 -0700 Added more possible next steps commit e0f6be1 Author: Ryan Hofschneider <ryan.hofschneider@gsa.gov> Date: Wed Aug 26 10:22:32 2020 -0700 Initial import
Lively discussion in chat; let's open 'er up.
The text was updated successfully, but these errors were encountered: