mirror of
https://github.com/eng-practices/eng-practices.git
synced 2024-12-21 20:30:15 +08:00
Add simplified Chinese translation (#1)
This commit is contained in:
Jimmy Song
Ta-Ching Chen
parent
58cbf89be3
commit
6f755adb6e
2
.gitignore
vendored
Normal file
2
.gitignore
vendored
Normal file
@ -0,0 +1,2 @@
|
||||
_site
|
||||
.DS_Store
|
||||
@ -1,119 +0,0 @@
|
||||
# Writing good CL descriptions
|
||||
|
||||
|
||||
|
||||
A CL description is a public record of **what** change is being made and **why**
|
||||
it was made. It will become a permanent part of our version control history, and
|
||||
will possibly be read by hundreds of people other than your reviewers over the
|
||||
years.
|
||||
|
||||
Future developers will search for your CL based on its description. Someone in
|
||||
the future might be looking for your change because of a faint memory of its
|
||||
relevance but without the specifics handy. If all the important information is
|
||||
in the code and not the description, it's going to be a lot harder for them to
|
||||
locate your CL.
|
||||
|
||||
## First Line {#firstline}
|
||||
|
||||
* Short summary of what is being done.
|
||||
* Complete sentence, written as though it was an order.
|
||||
* Follow by empty line.
|
||||
|
||||
The **first line** of a CL description should be a short summary of
|
||||
*specifically* **what** *is being done by the CL*, followed by a blank line.
|
||||
This is what most future code searchers will see when they are browsing the
|
||||
version control history of a piece of code, so this first line should be
|
||||
informative enough that they don't have to read your CL or its whole description
|
||||
just to get a general idea of what your CL actually *did*.
|
||||
|
||||
By tradition, the first line of a CL description is a complete sentence, written
|
||||
as though it were an order (an imperative sentence). For example, say
|
||||
\"**Delete** the FizzBuzz RPC and **replace** it with the new system." instead
|
||||
of \"**Deleting** the FizzBuzz RPC and **replacing** it with the new system."
|
||||
You don't have to write the rest of the description as an imperative sentence,
|
||||
though.
|
||||
|
||||
## Body is Informative {#informative}
|
||||
|
||||
The rest of the description should be informative. It might include a brief
|
||||
description of the problem that's being solved, and why this is the best
|
||||
approach. If there are any shortcomings to the approach, they should be
|
||||
mentioned. If relevant, include background information such as bug numbers,
|
||||
benchmark results, and links to design documents.
|
||||
|
||||
Even small CLs deserve a little attention to detail. Put the CL in context.
|
||||
|
||||
## Bad CL Descriptions {#bad}
|
||||
|
||||
"Fix bug" is an inadequate CL description. What bug? What did you do to fix it?
|
||||
Other similarly bad descriptions include:
|
||||
|
||||
- "Fix build."
|
||||
- "Add patch."
|
||||
- "Moving code from A to B."
|
||||
- "Phase 1."
|
||||
- "Add convenience functions."
|
||||
- "kill weird URLs."
|
||||
|
||||
Some of those are real CL descriptions. Their authors may believe they are
|
||||
providing useful information, but they are not serving the purpose of a CL
|
||||
description.
|
||||
|
||||
## Good CL Descriptions {#good}
|
||||
|
||||
Here are some examples of good descriptions.
|
||||
|
||||
### Functionality change
|
||||
|
||||
> rpc: remove size limit on RPC server message freelist.
|
||||
>
|
||||
> Servers like FizzBuzz have very large messages and would benefit from reuse.
|
||||
> Make the freelist larger, and add a goroutine that frees the freelist entries
|
||||
> slowly over time, so that idle servers eventually release all freelist
|
||||
> entries.
|
||||
|
||||
The first few words describe what the CL actually does. The rest of the
|
||||
description talks about the problem being solved, why this is a good solution,
|
||||
and a bit more information about the specific implementation.
|
||||
|
||||
### Refactoring
|
||||
|
||||
> Construct a Task with a TimeKeeper to use its TimeStr and Now methods.
|
||||
>
|
||||
> Add a Now method to Task, so the borglet() getter method can be removed (which
|
||||
> was only used by OOMCandidate to call borglet's Now method). This replaces the
|
||||
> methods on Borglet that delegate to a TimeKeeper.
|
||||
>
|
||||
> Allowing Tasks to supply Now is a step toward eliminating the dependency on
|
||||
> Borglet. Eventually, collaborators that depend on getting Now from the Task
|
||||
> should be changed to use a TimeKeeper directly, but this has been an
|
||||
> accommodation to refactoring in small steps.
|
||||
>
|
||||
> Continuing the long-range goal of refactoring the Borglet Hierarchy.
|
||||
|
||||
The first line describes what the CL does and how this is a change from the
|
||||
past. The rest of the description talks about the specific implementation, the
|
||||
context of the CL, that the solution isn't ideal, and possible future direction.
|
||||
It also explains *why* this change is being made.
|
||||
|
||||
### Small CL that needs some context
|
||||
|
||||
> Create a Python3 build rule for status.py.
|
||||
>
|
||||
> This allows consumers who are already using this as in Python3 to depend on a
|
||||
> rule that is next to the original status build rule instead of somewhere in
|
||||
> their own tree. It encourages new consumers to use Python3 if they can,
|
||||
> instead of Python2, and significantly simplifies some automated build file
|
||||
> refactoring tools being worked on currently.
|
||||
|
||||
The first sentence describes what's actually being done. The rest of the
|
||||
description explains *why* the change is being made and gives the reviewer a lot
|
||||
of context.
|
||||
|
||||
## Review the description before submitting the CL
|
||||
|
||||
CLs can undergo significant change during review. It can be worthwhile to review
|
||||
a CL description before submitting the CL, to ensure that the description still
|
||||
reflects what the CL does.
|
||||
|
||||
Next: [Small CLs](small-cls.md)
|
||||
@ -1,78 +0,0 @@
|
||||
# How to handle reviewer comments
|
||||
|
||||
|
||||
|
||||
When you've sent a CL out for review, it's likely that your reviewer will
|
||||
respond with several comments on your CL. Here are some useful things to know
|
||||
about handling reviewer comments.
|
||||
|
||||
## Don't Take it Personally {#personal}
|
||||
|
||||
The goal of review is to maintain the quality of our codebase and our products.
|
||||
When a reviewer provides a critique of your code, think of it as their attempt
|
||||
to help you, the codebase, and Google, rather than as a personal attack on you
|
||||
or your abilities.
|
||||
|
||||
Sometimes reviewers feel frustrated and they express that frustration in their
|
||||
comments. This isn't a good practice for reviewers, but as a developer you
|
||||
should be prepared for this. Ask yourself, "What is the constructive thing that
|
||||
the reviewer is trying to communicate to me?" and then operate as though that's
|
||||
what they actually said.
|
||||
|
||||
**Never respond in anger to code review comments.** That is a serious breach of
|
||||
professional etiquette that will live forever in the code review tool. If you
|
||||
are too angry or annoyed to respond kindly, then walk away from your computer
|
||||
for a while, or work on something else until you feel calm enough to reply
|
||||
politely.
|
||||
|
||||
In general, if a reviewer isn't providing feedback in a way that's constructive
|
||||
and polite, explain this to them in person. If you can't talk to them in person
|
||||
or on a video call, then send them a private email. Explain to them in a kind
|
||||
way what you don't like and what you'd like them to do differently. If they also
|
||||
respond in a non-constructive way to this private discussion, or it doesn't have
|
||||
the intended effect, then
|
||||
escalate to your manager as
|
||||
appropriate.
|
||||
|
||||
## Fix the Code {#code}
|
||||
|
||||
If a reviewer says that they don't understand something in your code, your first
|
||||
response should be to clarify the code itself. If the code can't be clarified,
|
||||
add a code comment that explains why the code is there. If a comment seems
|
||||
pointless, only then should your response be an explanation in the code review
|
||||
tool.
|
||||
|
||||
If a reviewer didn't understand some piece of your code, it's likely other
|
||||
future readers of the code won't understand either. Writing a response in the
|
||||
code review tool doesn't help future code readers, but clarifying your code or
|
||||
adding code comments does help them.
|
||||
|
||||
## Think for Yourself {#think}
|
||||
|
||||
Writing a CL can take a lot of work. It's often really satisfying to finally
|
||||
send one out for review, feel like it's done, and be pretty sure that no further
|
||||
work is needed. So when a reviewer comes back with comments on things that could
|
||||
be improved, it's easy to reflexively think the comments are wrong, the reviewer
|
||||
is blocking you unnecessarily, or they should just let you submit the CL.
|
||||
However, **no matter how certain you are** at this point, take a moment to step
|
||||
back and consider if the reviewer is providing valuable feedback that will help
|
||||
the codebase and Google. Your first question to yourself should always be, "Is
|
||||
the reviewer correct?"
|
||||
|
||||
If you can't answer that question, it's likely the reviewer needs to clarify
|
||||
their comments.
|
||||
|
||||
If you *have* considered it and you still think you're right, feel free to
|
||||
respond with an explanation of why your method of doing things is better for the
|
||||
codebase, users, and/or Google. Often, reviewers are actually providing
|
||||
*suggestions* and they want you to think for yourself about what's best. You
|
||||
might actually know something about the users, codebase, or CL that the reviewer
|
||||
doesn't know. So fill them in; give them more context. Usually you can come to
|
||||
some consensus between yourself and the reviewer based on technical facts.
|
||||
|
||||
## Resolving Conflicts {#conflicts}
|
||||
|
||||
Your first step in resolving conflicts should always be to try to come to
|
||||
consensus with your reviewer. If you can't achieve consensus, see
|
||||
[The Standard of Code Review](../reviewer/standard.md), which gives principles
|
||||
to follow in such a situation.
|
||||
@ -1,14 +0,0 @@
|
||||
# The CL author's guide to getting through code review
|
||||
|
||||
The pages in this section contain best practices for developers going through
|
||||
code review. These guidelines should help you get through reviews faster and
|
||||
with higher-quality results. You don't have to read them all, but they are
|
||||
intended to apply to every Google developer, and many people have found it
|
||||
helpful to read the whole set.
|
||||
|
||||
- [Writing Good CL Descriptions](cl-descriptions.md)
|
||||
- [Small CLs](small-cls.md)
|
||||
- [How to Handle Reviewer Comments](handling-comments.md)
|
||||
|
||||
See also [How to Do a Code Review](../reviewer/), which gives detailed guidance
|
||||
for code reviewers.
|
||||
@ -1,153 +0,0 @@
|
||||
# Small CLs
|
||||
|
||||
|
||||
|
||||
## Why Write Small CLs? {#why}
|
||||
|
||||
Small, simple CLs are:
|
||||
|
||||
- **Reviewed more quickly.** It's easier for a reviewer to find five minutes
|
||||
several times to review small CLs than to set aside a 30 minute block to
|
||||
review one large CL.
|
||||
- **Reviewed more thoroughly.** With large changes, reviewers and authors tend
|
||||
to get frustrated by large volumes of detailed commentary shifting back and
|
||||
forth—sometimes to the point where important points get missed or dropped.
|
||||
- **Less likely to introduce bugs.** Since you're making fewer changes, it's
|
||||
easier for you and your reviewer to reason effectively about the impact of
|
||||
the CL and see if a bug has been introduced.
|
||||
- **Less wasted work if they are rejected.** If you write a huge CL and then
|
||||
your reviewer says that the overall direction is wrong, you've wasted a lot
|
||||
of work.
|
||||
- **Easier to merge.** Working on a large CL takes a long time, so you will
|
||||
have lots of conflicts when you merge, and you will have to merge
|
||||
frequently.
|
||||
- **Easier to design well.** It's a lot easier to polish the design and code
|
||||
health of a small change than it is to refine all the details of a large
|
||||
change.
|
||||
- **Less blocking on reviews.** Sending self-contained portions of your
|
||||
overall change allows you to continue coding while you wait for your current
|
||||
CL in review.
|
||||
- **Simpler to roll back.** A large CL will more likely touch files that get
|
||||
updated between the initial CL submission and a rollback CL, complicating
|
||||
the rollback (the intermediate CLs will probably need to be rolled back
|
||||
too).
|
||||
|
||||
Note that **reviewers have discretion to reject your change outright for the
|
||||
sole reason of it being too large.** Usually they will thank you for your
|
||||
contribution but request that you somehow make it into a series of smaller
|
||||
changes. It can be a lot of work to split up a change after you've already
|
||||
written it, or require lots of time arguing about why the reviewer should accept
|
||||
your large change. It's easier to just write small CLs in the first place.
|
||||
|
||||
## What is Small? {#what_is_small}
|
||||
|
||||
In general, the right size for a CL is **one self-contained change**. This means
|
||||
that:
|
||||
|
||||
- The CL makes a minimal change that addresses **just one thing**. This is
|
||||
usually just one part of a feature, rather than a whole feature at once. In
|
||||
general it's better to err on the side of writing CLs that are too small vs.
|
||||
CLs that are too large. Work with your reviewer to find out what an
|
||||
acceptable size is.
|
||||
- Everything the reviewer needs to understand about the CL (except future
|
||||
development) is in the CL, the CL's description, the existing codebase, or a
|
||||
CL they've already reviewed.
|
||||
- The system will continue to work well for its users and for the developers
|
||||
after the CL is checked in.
|
||||
- The CL is not so small that its implications are difficult to understand. If
|
||||
you add a new API, you should include a usage of the API in the same CL so
|
||||
that reviewers can better understand how the API will be used. This also
|
||||
prevents checking in unused APIs.
|
||||
|
||||
There are no hard and fast rules about how large is "too large." 100 lines is
|
||||
usually a reasonable size for a CL, and 1000 lines is usually too large, but
|
||||
it's up to the judgment of your reviewer. The number of files that a change is
|
||||
spread across also affects its "size." A 200-line change in one file might be
|
||||
okay, but spread across 50 files it would usually be too large.
|
||||
|
||||
Keep in mind that although you have been intimately involved with your code from
|
||||
the moment you started to write it, the reviewer often has no context. What
|
||||
seems like an acceptably-sized CL to you might be overwhelming to your reviewer.
|
||||
When in doubt, write CLs that are smaller than you think you need to write.
|
||||
Reviewers rarely complain about getting CLs that are too small.
|
||||
|
||||
## When are Large CLs Okay? {#large_okay}
|
||||
|
||||
There are a few situations in which large changes aren't as bad:
|
||||
|
||||
- You can usually count deletion of an entire file as being just one line of
|
||||
change, because it doesn't take the reviewer very long to review.
|
||||
- Sometimes a large CL has been generated by an automatic refactoring tool
|
||||
that you trust completely, and the reviewer's job is just to sanity check
|
||||
and say that they really do want the change. These CLs can be larger,
|
||||
although some of the caveats from above (such as merging and testing) still
|
||||
apply.
|
||||
|
||||
### Splitting by Files {#splitting-files}
|
||||
|
||||
Another way to split up a CL is by groupings of files that will require
|
||||
different reviewers but are otherwise self-contained changes.
|
||||
|
||||
For example: you send off one CL for modifications to a protocol buffer and
|
||||
another CL for changes to the code that uses that proto. You have to submit the
|
||||
proto CL before the code CL, but they can both be reviewed simultaneously. If
|
||||
you do this, you might want to inform both sets of reviewers about the other CL
|
||||
that you wrote, so that they have context for your changes.
|
||||
|
||||
Another example: you send one CL for a code change and another for the
|
||||
configuration or experiment that uses that code; this is easier to roll back
|
||||
too, if necessary, as configuration/experiment files are sometimes pushed to
|
||||
production faster than code changes.
|
||||
|
||||
## Separate Out Refactorings {#refactoring}
|
||||
|
||||
It's usually best to do refactorings in a separate CL from feature changes or
|
||||
bug fixes. For example, moving and renaming a class should be in a different CL
|
||||
from fixing a bug in that class. It is much easier for reviewers to understand
|
||||
the changes introduced by each CL when they are separate.
|
||||
|
||||
Small cleanups such as fixing a local variable name can be included inside of a
|
||||
feature change or bug fix CL, though. It's up to the judgment of developers and
|
||||
reviewers to decide when a refactoring is so large that it will make the review
|
||||
more difficult if included in your current CL.
|
||||
|
||||
## Keep related test code in the same CL {#test_code}
|
||||
|
||||
Avoid splitting test code into a separate CL. Tests validating your code
|
||||
modifications should go into the same CL, even if it increases the code line
|
||||
count.
|
||||
|
||||
However, <i>independent</i> test modifications can go into separate CLs first,
|
||||
similar to the [refactorings guidelines](#refactoring). That includes:
|
||||
|
||||
* validating pre-existing, submitted code with new tests.
|
||||
* refactoring the test code (e.g. introduce helper functions).
|
||||
* introducing larger test framework code (e.g. an integration test).
|
||||
|
||||
## Don't Break the Build {#break}
|
||||
|
||||
If you have several CLs that depend on each other, you need to find a way to
|
||||
make sure the whole system keeps working after each CL is submitted. Otherwise
|
||||
you might break the build for all your fellow developers for a few minutes
|
||||
between your CL submissions (or even longer if something goes wrong unexpectedly
|
||||
with your later CL submissions).
|
||||
|
||||
## Can't Make it Small Enough {#cant}
|
||||
|
||||
Sometimes you will encounter situations where it seems like your CL *has* to be
|
||||
large. This is very rarely true. Authors who practice writing small CLs can
|
||||
almost always find a way to decompose functionality into a series of small
|
||||
changes.
|
||||
|
||||
Before writing a large CL, consider whether preceding it with a refactoring-only
|
||||
CL could pave the way for a cleaner implementation. Talk to your teammates and
|
||||
see if anybody has thoughts on how to implement the functionality in small CLs
|
||||
instead.
|
||||
|
||||
If all of these options fail (which should be extremely rare) then get consent
|
||||
from your reviewers in advance to review a large CL, so they are warned about
|
||||
what is coming. In this situation, expect to be going through the review process
|
||||
for a long time, be vigilant about not introducing bugs, and be extra diligent
|
||||
about writing tests.
|
||||
|
||||
Next: [How to Handle Reviewer Comments](handling-comments.md)
|
||||
@ -1,69 +0,0 @@
|
||||
# Emergencies
|
||||
|
||||
Sometimes there are emergency CLs that must pass through the entire code review
|
||||
process as quickly as
|
||||
possible.
|
||||
|
||||
|
||||
|
||||
## What Is An Emergency? {#what}
|
||||
|
||||
An emergency CL would be a **small** change that: allows a major launch to
|
||||
continue instead of rolling back, fixes a bug significantly affecting users in
|
||||
production, handles a pressing legal issue, closes a major security hole, etc.
|
||||
|
||||
In emergencies we really do care about the speed of the entire code review
|
||||
process, not just the [speed of response](reviewer/speed.md). In this case
|
||||
*only*, the reviewer should care more about the speed of the review and the
|
||||
correctness of the code (does it actually resolve the emergency?) than anything
|
||||
else. Also (perhaps obviously) such reviews should take priority over all other
|
||||
code reviews, when they come up.
|
||||
|
||||
However, after the emergency is resolved you should look over the emergency CLs
|
||||
again and give them a [more thorough review](reviewer/looking-for.md).
|
||||
|
||||
## What Is Not An Emergency? {#not}
|
||||
|
||||
To be clear, the following cases are *not* an emergency:
|
||||
|
||||
- Wanting to launch this week rather than next week (unless there is some
|
||||
actual [hard deadline](#deadlines) for launch such as a partner agreement).
|
||||
- The developer has worked on a feature for a very long time and they really
|
||||
want to get the CL in.
|
||||
- The reviewers are all in another timezone where it is currently nighttime or
|
||||
they are away on an off-site.
|
||||
- It is the end of the day on a Friday and it would just be great to get this
|
||||
CL in before the developer leaves for the weekend.
|
||||
- A manager says that this review has to be complete and the CL checked in
|
||||
today because of a [soft (not hard) deadline](#deadlines).
|
||||
- Rolling back a CL that is causing test failures or build breakages.
|
||||
|
||||
And so on.
|
||||
|
||||
## What Is a Hard Deadline? {#deadlines}
|
||||
|
||||
A hard deadline is one where **something disastrous would happen** if you miss
|
||||
it. For example:
|
||||
|
||||
- Submitting your CL by a certain date is necessary for a contractual
|
||||
obligation.
|
||||
- Your product will completely fail in the marketplace if not released by a
|
||||
certain date.
|
||||
- Some hardware manufacturers only ship new hardware once a year. If you miss
|
||||
the deadline to submit code to them, that could be disastrous, depending on
|
||||
what type of code you’re trying to ship.
|
||||
|
||||
Delaying a release for a week is not disastrous. Missing an important conference
|
||||
might be disastrous, but often is not.
|
||||
|
||||
Most deadlines are soft deadlines, not hard deadlines. They represent a desire
|
||||
for a feature to be done by a certain time. They are important, but you
|
||||
shouldn’t be sacrificing code health to make them.
|
||||
|
||||
If you have a long release cycle (several weeks) it can be tempting to sacrifice
|
||||
code review quality to get a feature in before the next cycle. However, this
|
||||
pattern, if repeated, is a common way for projects to build up overwhelming
|
||||
technical debt. If developers are routinely submitting CLs near the end of the
|
||||
cycle that "must get in" with only superficial review, then the team should
|
||||
modify its process so that large feature changes happen early in the cycle and
|
||||
have enough time for good review.
|
||||
@ -1,69 +0,0 @@
|
||||
# Code Review Developer Guide
|
||||
|
||||
## Introduction {#intro}
|
||||
|
||||
A code review is a process where someone other than the author(s) of a piece of
|
||||
code examines that code.
|
||||
|
||||
At Google we use code review to maintain the quality of our code and products.
|
||||
|
||||
This documentation is the canonical description of Google's code review
|
||||
processes and policies.
|
||||
|
||||
|
||||
|
||||
This page is an overview of our code review process. There are two other large
|
||||
documents that are a part of this guide:
|
||||
|
||||
- **[How To Do A Code Review](reviewer/)**: A detailed guide for code
|
||||
reviewers.
|
||||
- **[The CL Author's Guide](developer/)**: A detailed guide for developers
|
||||
whose CLs are going through review.
|
||||
|
||||
## What Do Code Reviewers Look For? {#look_for}
|
||||
|
||||
Code reviews should look at:
|
||||
|
||||
- **Design**: Is the code well-designed and appropriate for your system?
|
||||
- **Functionality**: Does the code behave as the author likely intended? Is
|
||||
the way the code behaves good for its users?
|
||||
- **Complexity**: Could the code be made simpler? Would another developer be
|
||||
able to easily understand and use this code when they come across it in the
|
||||
future?
|
||||
- **Tests**: Does the code have correct and well-designed automated tests?
|
||||
- **Naming**: Did the developer choose clear names for variables, classes,
|
||||
methods, etc.?
|
||||
- **Comments**: Are the comments clear and useful?
|
||||
- **Style**: Does the code follow our
|
||||
[style guides](http://google.github.io/styleguide/)?
|
||||
- **Documentation**: Did the developer also update relevant documentation?
|
||||
|
||||
See **[How To Do A Code Review](reviewer/)** for more information.
|
||||
|
||||
### Picking the Best Reviewers {#best_reviewers}
|
||||
|
||||
In general, you want to find the *best* reviewers you can who are capable of
|
||||
responding to your review within a reasonable period of time.
|
||||
|
||||
The best reviewer is the person who will be able to give you the most thorough
|
||||
and correct review for the piece of code you are writing. This usually means the
|
||||
owner(s) of the code, who may or may not be the people in the OWNERS file.
|
||||
Sometimes this means asking different people to review different parts of the
|
||||
CL.
|
||||
|
||||
If you find an ideal reviewer but they are not available, you should at least CC
|
||||
them on your change.
|
||||
|
||||
### In-Person Reviews {#in_person}
|
||||
|
||||
If you pair-programmed a piece of code with somebody who was qualified to do a
|
||||
good code review on it, then that code is considered reviewed.
|
||||
|
||||
You can also do in-person code reviews where the reviewer asks questions and the
|
||||
developer of the change speaks only when spoken to.
|
||||
|
||||
## See Also {#seealso}
|
||||
|
||||
- [How To Do A Code Review](reviewer/): A detailed guide for code reviewers.
|
||||
- [The CL Author's Guide](developer/): A detailed guide for developers whose
|
||||
CLs are going through review.
|
||||
@ -1,70 +0,0 @@
|
||||
# How to write code review comments
|
||||
|
||||
|
||||
|
||||
## Summary
|
||||
|
||||
- Be kind.
|
||||
- Explain your reasoning.
|
||||
- Balance giving explicit directions with just pointing out problems and
|
||||
letting the developer decide.
|
||||
- Encourage developers to simplify code or add code comments instead of just
|
||||
explaining the complexity to you.
|
||||
|
||||
## Courtesy
|
||||
|
||||
In general, it is important to be
|
||||
courteous and respectful while also being
|
||||
very clear and helpful to the developer whose code you are reviewing. One way to
|
||||
do this is to be sure that you are always making comments about the *code* and
|
||||
never making comments about the *developer*. You don't always have to follow
|
||||
this practice, but you should definitely use it when saying something that might
|
||||
otherwise be upsetting or contentious. For example:
|
||||
|
||||
Bad: "Why did **you** use threads here when there's obviously no benefit to be
|
||||
gained from concurrency?"
|
||||
|
||||
Good: "The concurrency model here is adding complexity to the system without any
|
||||
actual performance benefit that I can see. Because there's no performance
|
||||
benefit, it's best for this code to be single-threaded instead of using multiple
|
||||
threads."
|
||||
|
||||
## Explain Why {#why}
|
||||
|
||||
One thing you'll notice about the "good" example from above is that it helps the
|
||||
developer understand *why* you are making your comment. You don't always need to
|
||||
include this information in your review comments, but sometimes it's appropriate
|
||||
to give a bit more explanation around your intent, the best practice you're
|
||||
following, or how your suggestion improves code health.
|
||||
|
||||
## Giving Guidance {#guidance}
|
||||
|
||||
**In general it is the developer's responsibility to fix a CL, not the
|
||||
reviewer's.** You are not required to do detailed design of a solution or write
|
||||
code for the developer.
|
||||
|
||||
This doesn't mean the reviewer should be unhelpful, though. In general you
|
||||
should strike an appropriate balance between pointing out problems and providing
|
||||
direct guidance. Pointing out problems and letting the developer make a decision
|
||||
often helps the developer learn, and makes it easier to do code reviews. It also
|
||||
can result in a better solution, because the developer is closer to the code
|
||||
than the reviewer is.
|
||||
|
||||
However, sometimes direct instructions, suggestions, or even code are more
|
||||
helpful. The primary goal of code review is to get the best CL possible. A
|
||||
secondary goal is improving the skills of developers so that they require less
|
||||
and less review over time.
|
||||
|
||||
## Accepting Explanations {#explanations}
|
||||
|
||||
If you ask a developer to explain a piece of code that you don't understand,
|
||||
that should usually result in them **rewriting the code more clearly**.
|
||||
Occasionally, adding a comment in the code is also an appropriate response, as
|
||||
long as it's not just explaining overly complex code.
|
||||
|
||||
**Explanations written only in the code review tool are not helpful to future
|
||||
code readers.** They are acceptable only in a few circumstances, such as when
|
||||
you are reviewing an area you are not very familiar with and the developer
|
||||
explains something that normal readers of the code would have already known.
|
||||
|
||||
Next: [Handling Pushback in Code Reviews](pushback.md)
|
||||
@ -1,17 +0,0 @@
|
||||
# How to do a code review
|
||||
|
||||
The pages in this section contain recommendations on the best way to do code
|
||||
reviews, based on long experience. All together they represent one complete
|
||||
document, broken up into many separate sections. You don't have to read them
|
||||
all, but many people have found it very helpful to themselves and their team to
|
||||
read the entire set.
|
||||
|
||||
- [The Standard of Code Review](standard.md)
|
||||
- [What to Look For In a Code Review](looking-for.md)
|
||||
- [Navigating a CL in Review](navigate.md)
|
||||
- [Speed of Code Reviews](speed.md)
|
||||
- [How to Write Code Review Comments](comments.md)
|
||||
- [Handling Pushback in Code Reviews](pushback.md)
|
||||
|
||||
See also the [CL Author's Guide](../developer/), which gives detailed guidance
|
||||
to developers whose CLs are undergoing review.
|
||||
@ -1,204 +0,0 @@
|
||||
# What to look for in a code review
|
||||
|
||||
|
||||
|
||||
Note: Always make sure to take into account
|
||||
[The Standard of Code Review](standard.md) when considering each of these
|
||||
points.
|
||||
|
||||
## Design
|
||||
|
||||
The most important thing to cover in a review is the overall design of the CL.
|
||||
Do the interactions of various pieces of code in the CL make sense? Does this
|
||||
change belong in your codebase, or in a library? Does it integrate well with the
|
||||
rest of your system? Is now a good time to add this functionality?
|
||||
|
||||
## Functionality
|
||||
|
||||
Does this CL do what the developer intended? Is what the developer intended good
|
||||
for the users of this code? The "users" are usually both end-users (when they
|
||||
are affected by the change) and developers (who will have to "use" this code in
|
||||
the future).
|
||||
|
||||
Mostly, we expect developers to test CLs well-enough that they work correctly by
|
||||
the time they get to code review. However, as the reviewer you should still be
|
||||
thinking about edge cases, looking for concurrency problems, trying to think
|
||||
like a user, and making sure that there are no bugs that you see just by reading
|
||||
the code.
|
||||
|
||||
You *can* validate the CL if you want—the time when it's most important for a
|
||||
reviewer to check a CL's behavior is when it has a user-facing impact, such as a
|
||||
**UI change**. It's hard to understand how some changes will impact a user when
|
||||
you're just reading the code. For changes like that, you can have the developer
|
||||
give you a demo of the functionality if it's too inconvenient to patch in the CL
|
||||
and try it yourself.
|
||||
|
||||
Another time when it's particularly important to think about functionality
|
||||
during a code review is if there is some sort of **parallel programming** going
|
||||
on in the CL that could theoretically cause deadlocks or race conditions. These
|
||||
sorts of issues are very hard to detect by just running the code and usually
|
||||
need somebody (both the developer and the reviewer) to think through them
|
||||
carefully to be sure that problems aren't being introduced. (Note that this is
|
||||
also a good reason not to use concurrency models where race conditions or
|
||||
deadlocks are possible—it can make it very complex to do code reviews or
|
||||
understand the code.)
|
||||
|
||||
## Complexity
|
||||
|
||||
Is the CL more complex than it should be? Check this at every level of the
|
||||
CL—are individual lines too complex? Are functions too complex? Are classes too
|
||||
complex? "Too complex" usually means **"can't be understood quickly by code
|
||||
readers."** It can also mean **"developers are likely to introduce bugs when
|
||||
they try to call or modify this code."**
|
||||
|
||||
A particular type of complexity is **over-engineering**, where developers have
|
||||
made the code more generic than it needs to be, or added functionality that
|
||||
isn't presently needed by the system. Reviewers should be especially vigilant
|
||||
about over-engineering. Encourage developers to solve the problem they know
|
||||
needs to be solved *now*, not the problem that the developer speculates *might*
|
||||
need to be solved in the future. The future problem should be solved once it
|
||||
arrives and you can see its actual shape and requirements in the physical
|
||||
universe.
|
||||
|
||||
## Tests
|
||||
|
||||
Ask for unit, integration, or end-to-end
|
||||
tests as appropriate for the change. In general, tests should be added in the
|
||||
same CL as the production code unless the CL is handling an
|
||||
[emergency](../emergencies.md).
|
||||
|
||||
Make sure that the tests in the CL are correct, sensible, and useful. Tests do
|
||||
not test themselves, and we rarely write tests for our tests—a human must ensure
|
||||
that tests are valid.
|
||||
|
||||
Will the tests actually fail when the code is broken? If the code changes
|
||||
beneath them, will they start producing false positives? Does each test make
|
||||
simple and useful assertions? Are the tests separated appropriately between
|
||||
different test methods?
|
||||
|
||||
Remember that tests are also code that has to be maintained. Don't accept
|
||||
complexity in tests just because they aren't part of the main binary.
|
||||
|
||||
## Naming
|
||||
|
||||
Did the developer pick good names for everything? A good name is long enough to
|
||||
fully communicate what the item is or does, without being so long that it
|
||||
becomes hard to read.
|
||||
|
||||
## Comments
|
||||
|
||||
Did the developer write clear comments in understandable English? Are all of the
|
||||
comments actually necessary? Usually comments are useful when they **explain
|
||||
why** some code exists, and should not be explaining *what* some code is doing.
|
||||
If the code isn't clear enough to explain itself, then the code should be made
|
||||
simpler. There are some exceptions (regular expressions and complex algorithms
|
||||
often benefit greatly from comments that explain what they're doing, for
|
||||
example) but mostly comments are for information that the code itself can't
|
||||
possibly contain, like the reasoning behind a decision.
|
||||
|
||||
It can also be helpful to look at comments that were there before this CL. Maybe
|
||||
there is a TODO that can be removed now, a comment advising against this change
|
||||
being made, etc.
|
||||
|
||||
Note that comments are different from *documentation* of classes, modules, or
|
||||
functions, which should instead express the purpose of a piece of code, how it
|
||||
should be used, and how it behaves when used.
|
||||
|
||||
## Style
|
||||
|
||||
We have [style guides](http://google.github.io/styleguide/) at Google for all
|
||||
of our major languages, and even for most of the minor languages. Make sure the
|
||||
CL follows the appropriate style guides.
|
||||
|
||||
If you want to improve some style point that isn't in the style guide, prefix
|
||||
your comment with "Nit:" to let the developer know that it's a nitpick that you
|
||||
think would improve the code but isn't mandatory. Don't block CLs from being
|
||||
submitted based only on personal style preferences.
|
||||
|
||||
The author of the CL should not include major style changes combined with other
|
||||
changes. It makes it hard to see what is being changed in the CL, makes merges
|
||||
and rollbacks more complex, and causes other problems. For example, if the
|
||||
author wants to reformat the whole file, have them send you just the
|
||||
reformatting as one CL, and then send another CL with their functional changes
|
||||
after that.
|
||||
|
||||
## Documentation
|
||||
|
||||
If a CL changes how users build, test, interact with, or release code, check to
|
||||
see that it also updates associated documentation, including
|
||||
READMEs, g3doc pages, and any generated
|
||||
reference docs. If the CL deletes or deprecates code, consider whether the
|
||||
documentation should also be deleted.
|
||||
If documentation is
|
||||
missing, ask for it.
|
||||
|
||||
## Every Line {#every_line}
|
||||
|
||||
Look at *every* line of code that you have been assigned to review. Some things
|
||||
like data files, generated code, or large data structures you can scan over
|
||||
sometimes, but don't scan over a human-written class, function, or block of code
|
||||
and assume that what's inside of it is okay. Obviously some code deserves more
|
||||
careful scrutiny than other code—that's a judgment call that you have to
|
||||
make—but you should at least be sure that you *understand* what all the
|
||||
code is doing.
|
||||
|
||||
If it's too hard for you to read the code and this is slowing down the review,
|
||||
then you should let the developer know that
|
||||
and wait for them to clarify it before you try to review it. At Google, we hire
|
||||
great software engineers, and you are one of them. If you can't understand the
|
||||
code, it's very likely that other developers won't either. So you're also
|
||||
helping future developers understand this code, when you ask the developer to
|
||||
clarify it.
|
||||
|
||||
If you understand the code but you don't feel qualified to do some part of the
|
||||
review, make sure there is a reviewer on the CL who is qualified, particularly
|
||||
for complex issues such as security, concurrency, accessibility,
|
||||
internationalization, etc.
|
||||
|
||||
## Context
|
||||
|
||||
It is often helpful to look at the CL in a broad context. Usually the code
|
||||
review tool will only show you a few lines of code around the parts that are
|
||||
being changed. Sometimes you have to look at the whole file to be sure that the
|
||||
change actually makes sense. For example, you might see only four new lines
|
||||
being added, but when you look at the whole file, you see those four lines are
|
||||
in a 50-line method that now really needs to be broken up into smaller methods.
|
||||
|
||||
It's also useful to think about the CL in the context of the system as a whole.
|
||||
Is this CL improving the code health of the system or is it making the whole
|
||||
system more complex, less tested, etc.? **Don't accept CLs that degrade the code
|
||||
health of the system.** Most systems become complex through many small changes
|
||||
that add up, so it's important to prevent even small complexities in new
|
||||
changes.
|
||||
|
||||
## Good Things {#good_things}
|
||||
|
||||
If you see something nice in the CL, tell the developer, especially when they
|
||||
addressed one of your comments in a great way. Code reviews often just focus on
|
||||
mistakes, but they should offer encouragement and appreciation for good
|
||||
practices, as well. It’s sometimes even more valuable, in terms of mentoring, to
|
||||
tell a developer what they did right than to tell them what they did wrong.
|
||||
|
||||
## Summary
|
||||
|
||||
In doing a code review, you should make sure that:
|
||||
|
||||
- The code is well-designed.
|
||||
- The functionality is good for the users of the code.
|
||||
- Any UI changes are sensible and look good.
|
||||
- Any parallel programming is done safely.
|
||||
- The code isn't more complex than it needs to be.
|
||||
- The developer isn't implementing things they *might* need in the future but
|
||||
don't know they need now.
|
||||
- Code has appropriate unit tests.
|
||||
- Tests are well-designed.
|
||||
- The developer used clear names for everything.
|
||||
- Comments are clear and useful, and mostly explain *why* instead of *what*.
|
||||
- Code is appropriately documented (generally in g3doc).
|
||||
- The code conforms to our style guides.
|
||||
|
||||
Make sure to review **every line** of code you've been asked to review, look at
|
||||
the **context**, make sure you're **improving code health**, and compliment
|
||||
developers on **good things** that they do.
|
||||
|
||||
Next: [Navigating a CL in Review](navigate.md)
|
||||
@ -1,79 +0,0 @@
|
||||
# Navigating a CL in review
|
||||
|
||||
|
||||
|
||||
## Summary
|
||||
|
||||
Now that you know [what to look for](looking-for.md), what's the most efficient
|
||||
way to manage a review that's spread across multiple files?
|
||||
|
||||
1. Does the change make sense? Does it have a good
|
||||
[description](../developer/cl-descriptions.md)?
|
||||
2. Look at the most important part of the change first. Is it well-designed
|
||||
overall?
|
||||
3. Look at the rest of the CL in an appropriate sequence.
|
||||
|
||||
## Step One: Take a broad view of the change {#step_one}
|
||||
|
||||
Look at the [CL description](../developer/cl-descriptions.md) and what the CL
|
||||
does in general. Does this change even make sense? If this change shouldn't have
|
||||
happened in the first place, please respond immediately with an explanation of
|
||||
why the change should not be happening. When you reject a change like this, it's
|
||||
also a good idea to suggest to the developer what they should have done instead.
|
||||
|
||||
For example, you might say "Looks like you put some good work into this, thanks!
|
||||
However, we're actually going in the direction of removing the FooWidget system
|
||||
that you're modifying here, and so we don't want to make any new modifications
|
||||
to it right now. How about instead you refactor our new BarWidget class?"
|
||||
|
||||
Note that not only did the reviewer reject the current CL and provide an
|
||||
alternative suggestion, but they did it *courteously*. This kind of courtesy is
|
||||
important because we want to show that we respect each other as developers even
|
||||
when we disagree.
|
||||
|
||||
If you get more than a few CLs that represent changes you don't want to make,
|
||||
you should consider re-working your team's development process or the posted
|
||||
process for external contributors so that there is more communication before CLs
|
||||
are written. It's better to tell people "no" before they've done a ton of work
|
||||
that now has to be thrown away or drastically re-written.
|
||||
|
||||
## Step Two: Examine the main parts of the CL {#step_two}
|
||||
|
||||
Find the file or files that are the "main" part of this CL. Often, there is one
|
||||
file that has the largest number of logical changes, and it's the major piece of
|
||||
the CL. Look at these major parts first. This helps give context to all of the
|
||||
smaller parts of the CL, and generally accelerates doing the code review. If the
|
||||
CL is too large for you to figure out which parts are the major parts, ask the
|
||||
developer what you should look at first, or ask them to
|
||||
[split up the CL into multiple CLs](../developer/small-cls.md).
|
||||
|
||||
If you see some major design problems with this part of the CL, you should send
|
||||
those comments immediately, even if you don't have time to review the rest of
|
||||
the CL right now. In fact, reviewing the rest of the CL might be a waste of
|
||||
time, because if the design problems are significant enough, a lot of the other
|
||||
code under review is going to disappear and not matter anyway.
|
||||
|
||||
There are two major reasons it's so important to send these major design
|
||||
comments out immediately:
|
||||
|
||||
- Developers often mail a CL and then immediately start new work based on that
|
||||
CL while they wait for review. If there are major design problems in the CL
|
||||
you're reviewing, they're also going to have to re-work their later CL. You
|
||||
want to catch them before they've done too much extra work on top of the
|
||||
problematic design.
|
||||
- Major design changes take longer to do than small changes. Developers nearly
|
||||
all have deadlines; in order to make those deadlines and still have quality
|
||||
code in the codebase, the developer needs to start on any major re-work of
|
||||
the CL as soon as possible.
|
||||
|
||||
## Step Three: Look through the rest of the CL in an appropriate sequence {#step_three}
|
||||
|
||||
Once you've confirmed there are no major design problems with the CL as a whole,
|
||||
try to figure out a logical sequence to look through the files while also making
|
||||
sure you don't miss reviewing any file. Usually after you've looked through the
|
||||
major files, it's simplest to just go through each file in the order that
|
||||
the code review tool presents them to you. Sometimes it's also helpful to read the tests
|
||||
first before you read the main code, because then you have an idea of what the
|
||||
change is supposed to be doing.
|
||||
|
||||
Next: [Speed of Code Reviews](speed.md)
|
||||
@ -1,83 +0,0 @@
|
||||
# Handling pushback in code reviews
|
||||
|
||||
|
||||
|
||||
Sometimes a developer will push back on a code review. Either they will disagree
|
||||
with your suggestion or they will complain that you are being too strict in
|
||||
general.
|
||||
|
||||
## Who is right? {#who_is_right}
|
||||
|
||||
When a developer disagrees with your suggestion, first take a moment to consider
|
||||
if they are correct. Often, they are closer to the code than you are, and so
|
||||
they might really have a better insight about certain aspects of it. Does their
|
||||
argument make sense? Does it make sense from a code health perspective? If so,
|
||||
let them know that they are right and let the issue drop.
|
||||
|
||||
However, developers are not always right. In this case the reviewer should
|
||||
further explain why they believe that their suggestion is correct. A good
|
||||
explanation demonstrates both an understanding of the developer's reply, and
|
||||
additional information about why the change is being requested.
|
||||
|
||||
In particular, when the reviewer believes their suggestion will improve code
|
||||
health, they should continue to advocate for the change, if they believe the
|
||||
resulting code quality improvement justifies the additional work requested.
|
||||
**Improving code health tends to happen in small steps.**
|
||||
|
||||
Sometimes it takes a few rounds of explaining a suggestion before it really
|
||||
sinks in. Just make sure to always stay [polite](comments.md#courtesy) and let
|
||||
the developer know that you *hear* what they're saying, you just don't *agree*.
|
||||
|
||||
## Upsetting Developers {#upsetting_developers}
|
||||
|
||||
Reviewers sometimes believe that the developer will be upset if the reviewer
|
||||
insists on an improvement. Sometimes developers do become upset, but it is
|
||||
usually brief and they become very thankful later that you helped them improve
|
||||
the quality of their code. Usually, if you are [polite](comments.md#courtesy) in
|
||||
your comments, developers actually don't become upset at all, and the worry is
|
||||
just in the reviewer's mind. Upsets are usually more about
|
||||
[the way comments are written](comments.md#courtesy) than about the reviewer's
|
||||
insistence on code quality.
|
||||
|
||||
## Cleaning It Up Later {#later}
|
||||
|
||||
A common source of push back is that developers (understandably) want to get
|
||||
things done. They don't want to go through another round of review just to get
|
||||
this CL in. So they say they will clean something up in a later CL, and thus you
|
||||
should LGTM *this* CL now. Some developers are very good about this, and will
|
||||
immediately write a follow-up CL that fixes the issue. However, experience shows
|
||||
that as more time passes after a developer writes the original CL, the less
|
||||
likely this clean up is to happen. In fact, usually unless the developer does
|
||||
the clean up *immediately* after the present CL, it never happens. This isn't
|
||||
because developers are irresponsible, but because they have a lot of work to do
|
||||
and the cleanup gets lost or forgotten in the press of other work. Thus, it is
|
||||
usually best to insist that the developer clean up their CL *now*, before the
|
||||
code is in the codebase and "done." Letting people "clean things up later" is a
|
||||
common way for codebases to degenerate.
|
||||
|
||||
If a CL introduces new complexity, it must be cleaned up before submission
|
||||
unless it is an [emergency](../emergencies.md). If the CL exposes surrounding
|
||||
problems and they can't be addressed right now, the developer should file a bug
|
||||
for the cleanup and assign it to themselves so that it doesn't get lost. They
|
||||
can optionally also write a TODO comment in the code that references the filed
|
||||
bug.
|
||||
|
||||
## General Complaints About Strictness {#strictness}
|
||||
|
||||
If you previously had fairly lax code reviews and you switch to having strict
|
||||
reviews, some developers will complain very loudly. Improving the
|
||||
[speed](speed.md) of your code reviews usually causes these complaints to fade
|
||||
away.
|
||||
|
||||
Sometimes it can take months for these complaints to fade away, but eventually
|
||||
developers tend to see the value of strict code reviews as they see what great
|
||||
code they help generate. Sometimes the loudest protesters even become your
|
||||
strongest supporters once something happens that causes them to really see the
|
||||
value you're adding by being strict.
|
||||
|
||||
## Resolving Conflicts {#conflicts}
|
||||
|
||||
If you are following all of the above but you still encounter a conflict between
|
||||
yourself and a developer that can't be resolved, see
|
||||
[The Standard of Code Review](standard.md) for guidelines and principles that
|
||||
can help resolve the conflict.
|
||||
@ -1,142 +0,0 @@
|
||||
# Speed of Code Reviews
|
||||
|
||||
|
||||
|
||||
## Why Should Code Reviews Be Fast? {#why}
|
||||
|
||||
**At Google, we optimize for the speed at which a team of developers can produce
|
||||
a product together**, as opposed to optimizing for the speed at which an
|
||||
individual developer can write code. The speed of individual development is
|
||||
important, it's just not _as_ important as the velocity of the entire team.
|
||||
|
||||
When code reviews are slow, several things happen:
|
||||
|
||||
* **The velocity of the team as a whole is decreased.** Yes, the individual,
|
||||
who doesn't respond quickly to the review, gets other work done. However,
|
||||
new features and bug fixes for the rest of the team are delayed by days,
|
||||
weeks, or months as each CL waits for review and re-review.
|
||||
* **Developers start to protest the code review process.** If a reviewer only
|
||||
responds every few days, but requests major changes to the CL each time,
|
||||
that can be frustrating and difficult for developers. Often, this is
|
||||
expressed as complaints about how "strict" the reviewer is being. If the
|
||||
reviewer requests the _same_ substantial changes (changes which really do
|
||||
improve code health) but responds _quickly_ every time the developer makes
|
||||
an update, the complaints tend to disappear. **Most complaints about the
|
||||
code review process are actually resolved by making the process faster.**
|
||||
* **Code health can be impacted.** When reviews are slow, there is increased
|
||||
pressure to allow developers to submit CLs that are not as good as they
|
||||
could be. Slow reviews also discourage code cleanups, refactorings, and
|
||||
further improvements to existing CLs.
|
||||
|
||||
## How Fast Should Code Reviews Be? {#fast}
|
||||
|
||||
If you are not in the middle of a focused task, **you should do a code review
|
||||
shortly after it comes in.**
|
||||
|
||||
**One business day is the maximum time it should take to respond** to a code
|
||||
review request (i.e. first thing the next morning).
|
||||
|
||||
Following these guidelines means that a typical CL should get multiple rounds of
|
||||
review (if needed) within a single day.
|
||||
|
||||
## Speed vs. Interruption {#interruption}
|
||||
|
||||
There is one time where the consideration of personal velocity trumps team
|
||||
velocity. **If you are in the middle of a focused task, such as writing code,
|
||||
don't interrupt yourself to do a code review.** Research has shown that it can
|
||||
take a long time for a developer to get back into a smooth flow of development
|
||||
after being interrupted. So interrupting yourself while coding is actually
|
||||
_more_ expensive to the team than making another developer wait a bit for a code
|
||||
review.
|
||||
|
||||
Instead, wait for a break point in your work before you respond to a request for
|
||||
review. This could be when your current coding task is completed, after lunch,
|
||||
returning from a meeting, coming back from the microkitchen, etc.
|
||||
|
||||
## Fast Responses {#responses}
|
||||
|
||||
When we talk about the speed of code reviews, it is the _response_ time that we
|
||||
are concerned with, as opposed to how long it takes a CL to get through the
|
||||
whole review and be submitted. The whole process should also be fast, ideally,
|
||||
but **it's even more important for the _individual responses_ to come quickly
|
||||
than it is for the whole process to happen rapidly.**
|
||||
|
||||
Even if it sometimes takes a long time to get through the entire review
|
||||
_process_, having quick responses from the reviewer throughout the process
|
||||
significantly eases the frustration developers can feel with "slow" code
|
||||
reviews.
|
||||
|
||||
If you are too busy to do a full review on a CL when it comes in, you can still
|
||||
send a quick response that lets the developer know when you will get to it,
|
||||
suggest other reviewers who might be able to respond more quickly, or
|
||||
[provide some initial broad comments](navigate.md). (Note: none of this means
|
||||
you should interrupt coding even to send a response like this—send the
|
||||
response at a reasonable break point in your work.)
|
||||
|
||||
**It is important that reviewers spend enough time on review that they are
|
||||
certain their "LGTM" means "this code meets [our standards](standard.md)."**
|
||||
However, individual responses should still ideally be [fast](#fast).
|
||||
|
||||
## Cross-Time-Zone Reviews {#tz}
|
||||
|
||||
When dealing with time zone differences, try to get back to the author when they
|
||||
are still in the office. If they have already gone home, then try to make sure
|
||||
your review is done before they get back to the office the next day.
|
||||
|
||||
## LGTM With Comments {#lgtm-with-comments}
|
||||
|
||||
In order to speed up code reviews, there are certain situations in which a
|
||||
reviewer should give LGTM/Approval even though they are also leaving unresolved
|
||||
comments on the CL. This is done when either:
|
||||
|
||||
* The reviewer is confident that the developer will appropriately address all
|
||||
the reviewer's remaining comments.
|
||||
* The remaining changes are minor and don't _have_ to be done by the
|
||||
developer.
|
||||
|
||||
The reviewer should specify which of these options they intend, if it is not
|
||||
otherwise clear.
|
||||
|
||||
LGTM With Comments is especially worth considering when the developer and
|
||||
reviewer are in different time zones and otherwise the developer would be
|
||||
waiting for a whole day just to get "LGTM, Approval."
|
||||
|
||||
## Large CLs {#large}
|
||||
|
||||
If somebody sends you a code review that is so large you're not sure when you
|
||||
will be able to have time to review it, your typical response should be to ask
|
||||
the developer to
|
||||
[split the CL into several smaller CLs](../developer/small-cls.md) that build on
|
||||
each other, instead of one huge CL that has to be reviewed all at once. This is
|
||||
usually possible and very helpful to reviewers, even if it takes additional work
|
||||
from the developer.
|
||||
|
||||
If a CL *can't* be broken up into smaller CLs, and you don't have time to review
|
||||
the entire thing quickly, then at least write some comments on the overall
|
||||
design of the CL and send it back to the developer for improvement. One of your
|
||||
goals as a reviewer should be to always unblock the developer or enable them to
|
||||
take some sort of further action quickly, without sacrificing code health to do
|
||||
so.
|
||||
|
||||
## Code Review Improvements Over Time {#time}
|
||||
|
||||
If you follow these guidelines and you are strict with your code reviews, you
|
||||
should find that the entire code review process tends to go faster and faster
|
||||
over time. Developers learn what is required for healthy code, and send you CLs
|
||||
that are great from the start, requiring less and less review time. Reviewers
|
||||
learn to respond quickly and not add unnecessary latency into the review
|
||||
process.
|
||||
But **don't compromise on
|
||||
the [code review standards](standard.md) or quality for an imagined improvement
|
||||
in velocity**—it's not actually going to make anything happen more
|
||||
quickly, in the long run.
|
||||
|
||||
## Emergencies
|
||||
|
||||
There are also [emergencies](../emergencies.md) where CLs must pass through the
|
||||
_whole_ review process very quickly, and where the quality guidelines would be
|
||||
relaxed. However, please see [What Is An Emergency?](../emergencies.md#what) for
|
||||
a description of which situations actually qualify as emergencies and which
|
||||
don't.
|
||||
|
||||
Next: [How to Write Code Review Comments](comments.md)
|
||||
@ -1,112 +0,0 @@
|
||||
# The Standard of Code Review
|
||||
|
||||
|
||||
|
||||
The primary purpose of code review is to make sure that the overall
|
||||
code health of Google's code
|
||||
base is improving over time. All of the tools and processes of code review are
|
||||
designed to this end.
|
||||
|
||||
In order to accomplish this, a series of trade-offs have to be balanced.
|
||||
|
||||
First, developers must be able to _make progress_ on their tasks. If you never
|
||||
submit an improvement to the codebase, then the codebase never improves. Also,
|
||||
if a reviewer makes it very difficult for _any_ change to go in, then developers
|
||||
are disincentivized to make improvements in the future.
|
||||
|
||||
On the other hand, it is the duty of the reviewer to make sure that each CL is
|
||||
of such a quality that the overall code health of their codebase is not
|
||||
decreasing as time goes on. This can be tricky, because often, codebases degrade
|
||||
through small decreases in code health over time, especially when a team is
|
||||
under significant time constraints and they feel that they have to take
|
||||
shortcuts in order to accomplish their goals.
|
||||
|
||||
Also, a reviewer has ownership and responsibility over the code they are
|
||||
reviewing. They want to ensure that the codebase stays consistent, maintainable,
|
||||
and all of the other things mentioned in
|
||||
["What to look for in a code review."](looking-for.md)
|
||||
|
||||
Thus, we get the following rule as the standard we expect in code reviews:
|
||||
|
||||
**In general, reviewers should favor approving a CL once it is in a state where
|
||||
it definitely improves the overall
|
||||
code health of the system
|
||||
being worked on, even if the CL isn't perfect.**
|
||||
|
||||
That is _the_ senior principle among all of the code review guidelines.
|
||||
|
||||
There are limitations to this, of course. For example, if a CL adds a feature
|
||||
that the reviewer doesn't want in their system, then the reviewer can certainly
|
||||
deny approval even if the code is well-designed.
|
||||
|
||||
A key point here is that there is no such thing as "perfect" code—there is
|
||||
only _better_ code. Reviewers should not require the author to polish every tiny
|
||||
piece of a CL before granting approval. Rather, the reviewer should balance out
|
||||
the need to make forward progress compared to the importance of the changes they
|
||||
are suggesting. Instead of seeking perfection, what a reviewer should seek is
|
||||
_continuous improvement_. A CL that, as a whole, improves the maintainability,
|
||||
readability, and understandability of the system shouldn't be delayed for days
|
||||
or weeks because it isn't "perfect."
|
||||
|
||||
Reviewers should _always_ feel free to leave comments expressing that something
|
||||
could be better, but if it's not very important, prefix it with something like
|
||||
"Nit: " to let the author know that it's just a point of polish that they could
|
||||
choose to ignore.
|
||||
|
||||
Note: Nothing in this document justifies checking in CLs that definitely
|
||||
_worsen_ the overall code health of the system. The only time you would do that
|
||||
would be in an [emergency](../emergencies.md).
|
||||
|
||||
## Mentoring
|
||||
|
||||
Code review can have an important function of teaching developers something new
|
||||
about a language, a framework, or general software design principles. It's
|
||||
always fine to leave comments that help a developer learn something new. Sharing
|
||||
knowledge is part of improving the code health of a system over time. Just keep
|
||||
in mind that if your comment is purely educational, but not critical to meeting
|
||||
the standards described in this document, prefix it with "Nit: " or otherwise
|
||||
indicate that it's not mandatory for the author to resolve it in this CL.
|
||||
|
||||
## Principles {#principles}
|
||||
|
||||
* Technical facts and data overrule opinions and personal preferences.
|
||||
|
||||
* On matters of style, the [style guide](http://google.github.io/styleguide/)
|
||||
is the absolute authority. Any purely style point (whitespace, etc.) that is
|
||||
not in the style guide is a matter of personal preference. The style should
|
||||
be consistent with what is there. If there is no previous style, accept the
|
||||
author's.
|
||||
|
||||
* **Aspects of software design are almost never a pure style issue or just a
|
||||
personal preference.** They are based on underlying principles and should be
|
||||
weighed on those principles, not simply by personal opinion. Sometimes there
|
||||
are a few valid options. If the author can demonstrate (either through data
|
||||
or based on solid engineering principles) that several approaches are
|
||||
equally valid, then the reviewer should accept the preference of the author.
|
||||
Otherwise the choice is dictated by standard principles of software design.
|
||||
|
||||
* If no other rule applies, then the reviewer may ask the author to be
|
||||
consistent with what is in the current codebase, as long as that doesn't
|
||||
worsen the overall code health of the system.
|
||||
|
||||
## Resolving Conflicts {#conflicts}
|
||||
|
||||
In any conflict on a code review, the first step should always be for the
|
||||
developer and reviewer to try to come to consensus, based on the contents of
|
||||
this document and the other documents in [The CL Author's Guide](../developer/)
|
||||
and this [Reviewer Guide](index.md).
|
||||
|
||||
When coming to consensus becomes especially difficult, it can help to have a
|
||||
face-to-face meeting or a VC between the reviewer and the author, instead of
|
||||
just trying to resolve the conflict through code review comments. (If you do
|
||||
this, though, make sure to record the results of the discussion in a comment on
|
||||
the CL, for future readers.)
|
||||
|
||||
If that doesn't resolve the situation, the most common way to resolve it would
|
||||
be to escalate. Often the
|
||||
escalation path is to a broader team discussion, having a TL weigh in, asking
|
||||
for a decision from a maintainer of the code, or asking an Eng Manager to help
|
||||
out. **Don't let a CL sit around because the author and the reviewer can't come
|
||||
to an agreement.**
|
||||
|
||||
Next: [What to look for in a code review](looking-for.md)
|
||||
22
zh-cn/README.md
Normal file
22
zh-cn/README.md
Normal file
@ -0,0 +1,22 @@
|
||||
# 谷歌工程实践文档
|
||||
|
||||
Google 有许多通用工程实践，几乎涵盖所有语言和项目。此文档为长期积累的最佳实践，是集体经验的结晶。我们尽可能地将其公之于众，您的组织和开源项目也会从中受益。
|
||||
|
||||
当前包含以下文档：
|
||||
|
||||
* [Google 代码审查指南](review/index.md)，实则两套指南：
|
||||
* [代码审查者指南](review/reviewer/index.md)
|
||||
* [代码开发者指南](review/developer/index.md)
|
||||
|
||||
## 术语
|
||||
|
||||
文档中使用了 Google 内部术语，在此为外部读者澄清：
|
||||
|
||||
* **CL**：代表“变更列表（changelist）”，表示已提交到版本控制或正在进行代码审查的自包含更改。有的组织会将其称为“变更（change）”或“补丁（patch）”。
|
||||
* **LGTM**：意思是“我觉得不错（Looks Good to Me）”。这是批准 CL 时代码审查者所说的。
|
||||
|
||||
## License
|
||||
|
||||
本项目中的文档适用于 CC-By 3.0 许可证，该许可证鼓励您共享这些文档。有关详细信息，请参阅 <https://creativecommons.org/licenses/by/3.0/>。
|
||||
|
||||
<a rel="license" href="https://creativecommons.org/licenses/by/3.0/"><img alt="Creative Commons License" style="border-width:0" src="https://i.creativecommons.org/l/by/3.0/88x31.png" /></a>
|
||||
72
zh-cn/review/developer/cl-descriptions.md
Normal file
72
zh-cn/review/developer/cl-descriptions.md
Normal file
@ -0,0 +1,72 @@
|
||||
# 写好 CL 描述
|
||||
|
||||
CL 描述是进行了**哪些更改**以及**为何更改**的公开记录。CL 将作为版本控制系统中的永久记录，可能会在长时期内被除审查者之外的数百人阅读。
|
||||
|
||||
开发者将来会根据描述搜索您的 CL。有人可能会仅凭有关联性的微弱印象，但没有更多具体细节的情况下，来查找你的改动。。如果所有重要信息都在代码而不是描述中，那么会让他们更加难以找到你的 CL 。
|
||||
|
||||
## 首行 {#firstline}
|
||||
|
||||
* 正在做什么的简短摘要。
|
||||
* 完整的句子，使用祈使句。
|
||||
* 后面跟一个空行。
|
||||
|
||||
CL 描述的**第一行**应该是关于这个 CL 是**做什么**的简短摘要，后面跟一个空白行。这是将来大多数的代码搜索者在浏览代码的版本控制历史时，最常被看到的内容，因此第一行应该提供足够的信息，以便他们不必阅读 CL 的整个描述就可以获得这个 CL 实际上是做了什么的信息。
|
||||
|
||||
按照传统，CL 描述的第一行应该是一个完整的句子，就好像是一个命令（一个命令句）。例如，“**Delete** the FizzBuzz RPC and **replace** it with the new system.”而不是“**Deleting** the FizzBuzz RPC and **replacing** it with the new system.“ 但是，您不必把其余的描述写成祈使句。
|
||||
|
||||
## Body 是信息丰富的 {#informative}
|
||||
|
||||
其余描述应该是提供信息的。可能包括对正在解决的问题的简要描述，以及为什么这是最好的方法。如果方法有任何缺点，应该提到它们。如果相关，请包括背景信息，例如错误编号，基准测试结果以及设计文档的链接。
|
||||
|
||||
即使是小型 CL 也需要注意细节。在 CL 描述中提供上下文以供参照。
|
||||
|
||||
## 糟糕的 CL 描述 {#bad}
|
||||
|
||||
“Fix bug ”是一个不充分的 CL 描述。什么 bug？你做了什么修复？其他类似的不良描述包括：
|
||||
|
||||
- "Fix build."
|
||||
- "Add patch."
|
||||
- "Moving code from A to B."
|
||||
- "Phase 1."
|
||||
- "Add convenience functions."
|
||||
- "kill weird URLs."
|
||||
|
||||
其中一些是真正的 CL 描述。他们的作者可能认为自己提供了有用的信息，却没有达到 CL 描述的目的。
|
||||
|
||||
## 好的 CL 描述 {#good}
|
||||
|
||||
以下是一些很好的描述示例。
|
||||
|
||||
### 功能更新
|
||||
|
||||
> rpc：删除 RPC 服务器消息 freelist 上的大小限制。
|
||||
>
|
||||
> 像 FIzzBuzz 这样的服务器有非常大的消息，并且可以从重用中受益。增大 freelist，添加一个 goroutine，缓慢释放 freelist 条目，以便空闲服务器最终释放所有 freelist 条目。
|
||||
|
||||
前几个词描述了CL实际上做了什么。其余的描述讨论了正在解决的问题，为什么这是一个很好的解决方案，以及有关具体实现的更多信息。
|
||||
|
||||
### 重构
|
||||
|
||||
> Construct a Task with a TimeKeeper to use its TimeStr and Now methods.
|
||||
>
|
||||
> Add a Now method to Task, so the borglet() getter method can be removed (which was only used by OOMCandidate to call borglet's Now method). This replaces the methods on Borglet that delegate to a TimeKeeper.
|
||||
>
|
||||
> Allowing Tasks to supply Now is a step toward eliminating the dependency on Borglet. Eventually, collaborators that depend on getting Now from the Task should be changed to use a TimeKeeper directly, but this has been an accommodation to refactoring in small steps.
|
||||
>
|
||||
> Continuing the long-range goal of refactoring the Borglet Hierarchy.
|
||||
|
||||
第一行描述了 CL 的作用以及改变。其余的描述讨论了具体的实现，CL 的背景，解决方案并不理想，以及未来的可能方向。它还解释了为什么正在进行此更改。
|
||||
|
||||
### 需要上下文的 小 CL
|
||||
|
||||
> Create a Python3 build rule for status.py.
|
||||
>
|
||||
> This allows consumers who are already using this as in Python3 to depend on a rule that is next to the original status build rule instead of somewhere in their own tree. It encourages new consumers to use Python3 if they can, instead of Python2, and significantly simplifies some automated build file refactoring tools being worked on currently.
|
||||
|
||||
第一句话描述实际做了什么。其余的描述解释了为什么正在进行更改并为审查者提供了大量背景信息。
|
||||
|
||||
## 在提交 CL 前审查描述
|
||||
|
||||
CL 在审查期间可能会发生重大变更。在提交 CL 之前检查 CL 描述是必要的，以确保描述仍然反映了 CL 的作用。
|
||||
|
||||
下一篇：[小型 CL](small-cls.md)
|
||||
31
zh-cn/review/developer/handling-comments.md
Normal file
31
zh-cn/review/developer/handling-comments.md
Normal file
@ -0,0 +1,31 @@
|
||||
# 如何处理审查者的评论
|
||||
|
||||
当您发送 CL 进行审查时，您的审查者可能会对您的 CL 发表一些评论。以下是处理审查者评论的一些有用信息。
|
||||
|
||||
## 不是针对您 {#personal}
|
||||
|
||||
审查的目标是保持代码库和产品的质量。当审查者对您的代码提出批评时，请将其视为在帮助您、代码库和 Google，而不是对您或您的能力的个人攻击。
|
||||
|
||||
有时，审查者会感到沮丧并在评论中表达他们的挫折感。对于审查者来说，这不是一个好习惯，但作为开发人员，您应该为此做好准备。问问自己，“审查者试图与我沟通的建设性意见是什么？”然后像他们实际说的那样操作。
|
||||
|
||||
**永远不要愤怒地回应代码审查评论。**这严重违反了专业礼仪且将永远存在于代码审查工具中。如果您太生气或恼火而无法好好的回应，那么请离开电脑一段时间，或者做一些别的事情，直到您感到平静，可以礼貌地回答。
|
||||
|
||||
一般来说，如果审查者没有以建设性和礼貌的方式提供反馈，请亲自向他们解释。如果您无法亲自或通过视频通话与他们交谈，请向他们发送私人电子邮件。以友善的方式向他们解释您不喜欢的东西以及您希望他们以怎样不同的方式来做些什么。如果他们也以非建设性的方式回复此私人讨论，或者没有预期的效果，那么请酌情上报给您的经理。
|
||||
|
||||
## 修复代码 {#code}
|
||||
|
||||
如果审查者说他们不了解您的代码中的某些内容，那么您的第一反应应该是澄清代码本身。 如果无法澄清代码，请添加代码注释，以解释代码存在的原因。 只有在想增加的注释看起来毫无意义时，您才能在代码审查工具中进行回复与解释。
|
||||
|
||||
如果审查者不理解您的某些代码，那么代码的未来读者可能也不会理解。在代码审查工具中回复对未来的代码读者没有帮助，但澄清代码或添加代码注释确可以实实在在得帮助他们。
|
||||
|
||||
## 自我反思 {#think}
|
||||
|
||||
编写 CL 可能需要做很多工作。在终于发送一个 CL 用于审查后，我们通常会感到满足的，认为它已经完成，并且非常确定不需要进一步的工作。这通常是令人满意的。因此，当审查者回复对可以改进的事情的评论时，很容易本能地认为评论是错误的，审查者正在不必要地阻止您，或者他们应该让您提交 CL。但是，**无论您目前多么确定**，请花一点时间退一步，考虑审查者是否提供有助于对代码库和对 Google 的有价值的反馈。您首先应该想到的应该是，“审查者是否正确？”
|
||||
|
||||
如果您无法回答这个问题，那么审查者可能需要澄清他们的意见。
|
||||
|
||||
如果您已经考虑过并且仍然认为自己是正确的，请随时回答一下为什么您的方法对代码库、用户和/或 Google 更好。通常，审查者实际上是在提供建议，他们希望您自己思考什么是最好的。您可能实际上对审阅者不知道的用户、代码库或 CL 有所了解。所以提供并告诉他们更多的上下文。通常，您可以根据技术事实在自己和审查者之间达成一些共识。
|
||||
|
||||
## 解决冲突 {#conflicts}
|
||||
|
||||
解决冲突的第一步应该是尝试与审查者达成共识。 如果您无法达成共识，请参阅“[代码审查标准](../reviewer/standard.md)”，该标准提供了在这种情况下遵循的原则。
|
||||
10
zh-cn/review/developer/index.md
Normal file
10
zh-cn/review/developer/index.md
Normal file
@ -0,0 +1,10 @@
|
||||
# CL 开发者指南
|
||||
|
||||
本节页面的内容为开发人员进行代码审查的最佳实践。这些指南可帮助您更快地完成审核并获得更高质量的结果。您不必全部阅读它们，但它们适用于每个 Google 开发人员，并且许阅读全文通常会很有帮助。
|
||||
|
||||
- [写好 CL 描述](cl-descriptions.md)
|
||||
- [小型 CL](small-cls.md)
|
||||
- [如何处理审查者的评论](handling-comments.md)
|
||||
|
||||
另请参阅[如何执行代码审查](../reviewer/)，它为代码审阅者提供了详细的指导。
|
||||
|
||||
74
zh-cn/review/developer/small-cls.md
Normal file
74
zh-cn/review/developer/small-cls.md
Normal file
@ -0,0 +1,74 @@
|
||||
# 小型 CL
|
||||
|
||||
## 为什么提交小型 CL? {#why}
|
||||
|
||||
小且简单的 CL 是指：
|
||||
|
||||
- **审查更快。**审查者更容易抽多次五分钟时间来审查小型 CL，而不是留出 30 分钟来审查一个大型 CL。
|
||||
- **审查得更彻底。**如果是大的变更，审查者和提交者往往会因为大量细节的讨论翻来覆去而感到沮丧——有时甚至到了重要点被遗漏或丢失的程度。
|
||||
- **不太可能引入错误。** 由于您进行的变更较少，您和您的审查者可以更轻松有效地推断 CL 的影响，并查看是否已引入错误。
|
||||
- **如果被拒绝，减少浪费的工作。** 如果您写了一个巨大的 CL，您的评论者说整个 CL 的方向都错误了，你就浪费了很多精力和时间。
|
||||
- **更容易合并。** 处理大型 CL 需要很长时间，在合并时会出现很多冲突，并且必须经常合并。
|
||||
- **更容易设计好。** 打磨一个小变更的设计和代码健康状况比完善一个大变更的所有细节要容易得多。
|
||||
- **减少对审查的阻碍。** 发送整体变更的自包含部分可让您在等待当前 CL 审核时继续编码。
|
||||
- **更简单的回滚。** 大型 CL 更有可能触及在初始 CL 提交和回滚 CL 之间更新的文件，从而使回滚变得复杂（中间的 CL 也可能需要回滚）。
|
||||
|
||||
请注意，**审查者可以仅凭 CL 过大而自行决定完全拒绝您的变更。**通常他们会感谢您的贡献，但要求您以某种方式将其 CL 改成一系列较小的变更。在您编写完变更后，或者需要花费大量时间来讨论为什么审查者应该接受您的大变更，这可能需要做很多工作。首先编写小型 CL 更容易。
|
||||
|
||||
## 什么是小型 CL？ {#what_is_small}
|
||||
|
||||
一般来说，CL 的正确大小是**自包含的变更**。这意味着：
|
||||
|
||||
- CL 进行了一项最小的变更，**只解决了一件事**。通常只是功能的一部分，而不是一个完整的功能。一般来说，因为编写过小的 CL 而犯错也比过大的 CL 犯错要好。与您的审查者讨论以确定可接受的大小。
|
||||
- 审查者需要了解的关于 CL 的所有内容（除了未来的开发）都在 CL 的描述、现有的代码库或已经审查过的 CL 中。
|
||||
- 对其用户和开发者来说，在签入 CL 后系统能继续良好的工作。
|
||||
- CL 不会过小以致于其含义难以理解。如果您添加新 API，则应在同一 CL 中包含 API 的用法，以便审查者可以更好地了解 API 的使用方式。这也可以防止签入未使用的 API。
|
||||
|
||||
关于多大算“太大”没有严格的规则。对于 CL 来说，100 行通常是合理的大小，1000 行通常太大，但这取决于您的审查者的判断。变更中包含的文件数也会影响其“大小”。一个文件中的 200 行变更可能没问题，但是分布在 50 个文件中通常会太大。
|
||||
|
||||
请记住，尽管从开始编写代码开始就您就已经密切参与了代码，但审查者通常不清楚背景信息。对您来说，看起来像是一个可接受的大小的 CL 对您的审查者来说可能是压倒性的。如有疑问，请编写比您认为需要编写的要小的 CL。审查者很少抱怨收到过小的 CL 提交。
|
||||
|
||||
## 什么时候大 CL 是可以的？ {#large_okay}
|
||||
|
||||
在某些情况下，大变更也是可以接受的：
|
||||
|
||||
- 您通常可以将整个文件的删除视为一行变更，因为审核人员不需要很长时间审核。
|
||||
- 有时一个大的 CL 是由您完全信任的自动重构工具生成的，而审查者的工作只是检查并确定想要这样的变更。但这些 CL 可以更大，尽管上面的一些警告（例如合并和测试）仍然适用。
|
||||
|
||||
### 按文件拆分 {#splitting-files}
|
||||
|
||||
拆分 CL 的另一种方法是对文件进行分组，这些文件需要不同的审查者，否则就是自包含的变更。
|
||||
|
||||
例如：您发送一个 CL 以修改协议缓冲区，另一个 CL 发送变更使用该原型的代码。您必须在代码 CL 之前提交 proto CL，但它们都可以同时进行审查。如果这样做，您可能希望通知两组审查者您编写的其他 CL，以便他们对您的变更具有更充足的上下文。
|
||||
|
||||
另一个例子：你发送一个 CL 用于代码更改，另一个用于使用该代码的配置或实验；如果需要，这也更容易回滚，因为配置/实验文件有时会比代码变更更快地推向生产。
|
||||
|
||||
## 分离出重构 {#refactoring}
|
||||
|
||||
通常最好在功能变更或错误修复的单独 CL 中进行重构。例如，移动和重命名类应该与修复该类中的错误的 CL 不同。审查者更容易理解每个 CL 在单独时引入的更改。
|
||||
|
||||
但是，修复本地变量名称等小清理可以包含在功能变更或错误修复 CL 中。如果重构大到包含在您当前的 CL 中，会使审查更加困难的话，需要开发者和审查者一起判断是否将其拆开。
|
||||
|
||||
## 将相关的测试代码保存在同一个 CL 中 {#test_code}
|
||||
|
||||
避免将测试代码拆分为单独的 CL。验证代码修改的测试应该进入相同的 CL，即使它增加了代码行数。
|
||||
|
||||
但是，独立的测试修改可以首先进入单独的 CL，类似于[重构指南](#refactoring)。包括：
|
||||
|
||||
- 使用新测试验证预先存在的已提交代码。
|
||||
- 重构测试代码（例如引入辅助函数）。
|
||||
- 引入更大的测试框架代码（例如集成测试）。
|
||||
|
||||
## 不要破坏构建 {#break}
|
||||
|
||||
如果您有几个相互依赖的 CL，您需要找到一种方法来确保在每次提交 CL 后整个系统能够继续运作。否则可能会在您的 CL 提交的几分钟内打破所有开发人员的构建（如果您之后的 CL 提交意外出错，时间可能会甚至更长）。
|
||||
|
||||
## 如果不能让它足够小 {#cant}
|
||||
|
||||
有时你会遇到看起来您的 CL 必须如此庞大，但这通常很少是正确的。习惯于编写小型 CL 的提交者几乎总能找到将功能分解为一系列小变更的方法。
|
||||
|
||||
在编写大型 CL 之前，请考虑在重构 CL 之前是否可以为更清晰的实现铺平道路。与你的同伴聊聊，看看是否有人想过如何在小型 CL 中实现这些功能。
|
||||
|
||||
如果以上的努力都失败了（这应该是非常罕见的），那么请在事先征得审查者的同意后提交大型 CL，以便他们收到有关即将发生的事情的警告。在这种情况下，做好完成审查过程需要很长一段时间的准备，对不引入错误保持警惕，并且在编写测试时要更下功夫。
|
||||
|
||||
下一篇：[如何处理审查者评论](handling-comments.md)
|
||||
38
zh-cn/review/emergencies.md
Normal file
38
zh-cn/review/emergencies.md
Normal file
@ -0,0 +1,38 @@
|
||||
# 紧急情况
|
||||
|
||||
有时候紧急 CL 必须尽快通过 code review 过程。
|
||||
|
||||
## 什么是紧急情况？ {#what}
|
||||
|
||||
紧急 CL 是这样的**小**更新：允许主要发布继续而不是回滚，修复显着影响用户生产的错误，处理紧迫的法律问题，关闭主要安全漏洞等。
|
||||
|
||||
在紧急情况下，我们确实关心 Code Review 的整体速度，而不仅仅是响应的速度。仅在这种情况下，审查人员应该更关心审查的速度和代码的正确性（是否解决了紧急情况？）。此外（显然）这类状况的审查应该优先于所有其他 code reivew。
|
||||
|
||||
但是，在紧急情况解决后，您应该再次查看紧急 CL 并进行[更彻底的审查](reviewer/looking-for.md)。
|
||||
|
||||
## 什么不是紧急情况？ {#not}
|
||||
|
||||
需要说明的是，以下情况并非紧急情况：
|
||||
|
||||
- 想要在本周而不是下周推出（除非有一些实际[硬性截止日期](#deadlines)，例如合作伙伴协议）。
|
||||
- 开发人员已经在很长一段时间内完成了一项功能想要获得 CL。
|
||||
- 审查者都在另一个时区，目前是夜间或他们已离开现场。
|
||||
- 现在是星期五，在开发者在过周末之前获得这个 CL 会很棒。
|
||||
- 今天因为[软（非硬）截止日期](#deadlines)，经理表示必须完成此审核并签入 CL。
|
||||
- 回滚导致测试失败或构建破坏的 CL。
|
||||
|
||||
等等。
|
||||
|
||||
## 什么是 Hard Deadline？ {#deadlines}
|
||||
|
||||
硬性截止日期（Hard Deadline）是指如果你错过它会发生灾难性的事情。例如：
|
||||
|
||||
- 对于合同义务，必须在特定日期之前提交 CL。
|
||||
- 如果在某个日期之前没有发布，您的产品将在市场上完全失败。
|
||||
- 一些硬件制造商每年只发送一次新硬件。如果您错过了向他们提交代码的截止日期，那么这可能是灾难性的，具体取决于您尝试发布的代码类型。
|
||||
|
||||
延迟发布一周并不是灾难性的。错过重要会议可能是灾难性的，但往往不是。
|
||||
|
||||
大多数截止日期都是软截止日期，而非最后期限。软截止日期表示希望在特定时间内完成某项功能。它们很重要，但你不应该以牺牲代码健康为前提来达到。
|
||||
|
||||
如果您的发布周期很长（几周），那么在下一个周期之前就可能会牺牲代码审查质量来获取功能。然而，如果重复这种模式，往往会给项目建立压倒性技术债务。如果开发人员在周期结束时经常提交 CL，只需要进行表面评审就必须“进入”，那么团队应该修改其流程，以便在周期的早期发生大的功能变更，并有足够的时间进行良好的审查。
|
||||
48
zh-cn/review/index.md
Normal file
48
zh-cn/review/index.md
Normal file
@ -0,0 +1,48 @@
|
||||
# 开发者代码审查指南
|
||||
|
||||
## 简介 {#intro}
|
||||
|
||||
代码审查是除了代码作者之外，其他人检查代码的过程。
|
||||
|
||||
Google 通过 Code Review 来维护代码和产品质量。
|
||||
|
||||
此文档是 Google Code Review 流程和政策的规范说明。
|
||||
|
||||
此页面是我们进行 Code Review 流程的概述。本指南还有另外两套文档：
|
||||
|
||||
- **[如何进行 Code Review](reviewer/)**：针对代码审查者的详细指南。
|
||||
- **[代码开发者指南](developer/)**：针对 CL 开发者的的详细指南。
|
||||
|
||||
## 代码审查者应该关注哪些方面？ {#look_for}
|
||||
|
||||
代码审查时应该关注以下方面：
|
||||
|
||||
- **设计**：代码是否经过精心设计并适合您的系统？
|
||||
- **功能**：代码的行为是否与作者的意图相同？代码是否可以正常响应用户的行为？
|
||||
- **复杂度**：代码能更简单吗？将来其他开发人员能轻松理解并使用此代码吗？
|
||||
- **测试**：代码是否具有正确且设计良好的自动化测试？
|
||||
- **命名**：开发人员是否为变量、类、方法等选择了明确的名称？
|
||||
- **注释**：评论是否清晰有用？
|
||||
- **风格**：代码是否遵守了[风格指南](http://google.github.io/styleguide/)？
|
||||
- **文档**：开发人员是否同时更新了相关文档？
|
||||
|
||||
参阅**[如何进行 Code Review](reviewer/)** 获取更多资料。
|
||||
|
||||
### 选择最合适审查者 {#best_reviewers}
|
||||
|
||||
一般而言，您希望找到能在合理的时间内回复您的评论的最合适的审查者。
|
||||
|
||||
最合适的审查者应该是能彻底了解和审查您代码的人。他们通常是代码的所有者，可能是 OWNERS 文件中的人，也可能不是。有时 CL 的不同部分可能需要不同的人审查。
|
||||
|
||||
如果您找到了理想的审查者但他们又没空，那您也至少要抄送他们。
|
||||
|
||||
### 面对面审查 {#in_person}
|
||||
|
||||
如果您与有资格做代码审查的人一起结对编程了一段代码，那么该代码将被视为已审查。
|
||||
|
||||
您还可以进行面对面的代码审查，审查者提问，CL 的开发人员作答。
|
||||
|
||||
## 参考 {#seealso}
|
||||
|
||||
- [如何进行 Code Review](reviewer/)：针对代码审查者的详细指南。
|
||||
- [代码开发者指南](developer/)：针对 CL 开发者的的详细指南。
|
||||
37
zh-cn/review/reviewer/comments.md
Normal file
37
zh-cn/review/reviewer/comments.md
Normal file
@ -0,0 +1,37 @@
|
||||
# 如何撰写 Code Review 评论
|
||||
|
||||
## 总结
|
||||
|
||||
- 保持友善。
|
||||
- 解释你的推理。
|
||||
- 在给出明确的指示与只指出问题并让开发人员自己决定间做好平衡。
|
||||
- 鼓励开发人员简化代码或添加代码注释，而不仅仅是向你解释复杂性。
|
||||
|
||||
## 礼貌
|
||||
|
||||
一般而言，对于那些正在被您审查代码的人，除了保持有礼貌且尊重以外，重要的是还要确保您（的评论）是非常清楚且有帮助的。你并不总是必须遵循这种做法，但在说出可能令人不安或有争议的事情时你绝对应该使用它。 例如：
|
||||
|
||||
糟糕的示例：“为什么这里**你**使用了线程，显然并发并没有带来什么好处？”
|
||||
|
||||
好的示例：“这里的并发模型增加了系统的复杂性，但没有任何实际的性能优势，因为没有性能优势，最好是将这些代码作为单线程处理而不是使用多线程。”
|
||||
|
||||
## 解释为什么{#why}
|
||||
|
||||
关于上面的“好”示例，您会注意到的一件事是，它可以帮助开发人员理解您发表评论的原因。 并不总是需要您在审查评论中包含此信息，但有时候提供更多解释，对于表明您的意图，您在遵循的最佳实践，或为您建议如何提高代码健康状况是十分恰当的。
|
||||
|
||||
## 给予指导 {#guidance}
|
||||
|
||||
**一般来说，修复 CL 是开发人员的责任，而不是审查者。** 您无需为开发人员详细设计解决方案或编写代码。
|
||||
|
||||
但这并不意味着审查者应该没有帮助。一般来说，您应该在指出问题和提供直接指导之间取得适当的平衡。指出问题并让开发人员做出决定通常有助于开发人员学习，并使代码审查变得更容易。它还可能产生更好的解决方案，因为开发人员比审查者更接近代码。
|
||||
|
||||
但是，有时直接说明，建议甚至代码会更有帮助。代码审查的主要目标是尽可能获得最佳 CL。第二个目标是提高开发人员的技能，以便他们随着时间的推移需要的审查越来越少。
|
||||
|
||||
## 接受解释 {#explanations}
|
||||
|
||||
如果您要求开发人员解释一段您不理解的代码，那通常会导致他们**更清楚地重写代码**。偶尔，在代码中添加注释也是一种恰当的响应，只要它不仅仅是解释过于复杂的代码。
|
||||
|
||||
**仅在代码审查工具中编写的解释对未来的代码阅读者没有帮助。**这仅在少数情况下是可接受的，例如当您查看一个您不熟悉的领域时，开发人员会用来向您解释普通读者已经知道的内容。
|
||||
|
||||
下一篇：[处理 Code Review 中的抵触](pushback.md)
|
||||
|
||||
12
zh-cn/review/reviewer/index.md
Normal file
12
zh-cn/review/reviewer/index.md
Normal file
@ -0,0 +1,12 @@
|
||||
# 如何进行 Code Review
|
||||
|
||||
本节是基于过往经验编写的 Code Review 最佳方式建议。其中分为了很多独立的部分，共同组成完整的文档。虽然您不必阅读文档，但通读一遍会对您自己和团队很有帮助。
|
||||
|
||||
- [Code Review 标准](standard.md)
|
||||
- [Code Review 要点](looking-for.md)
|
||||
- [查看 CL 的步骤](navigate.md)
|
||||
- [Code Review 速度](speed.md)
|
||||
- [如何撰写 Code Review 评论](comments.md)
|
||||
- [处理 Code Review 中的抵触](pushback.md)
|
||||
|
||||
另请参阅 [CL 开发者指南](../developer/)，该指南为正在进行 Code Review 的开发开发者提供详细指导。
|
||||
96
zh-cn/review/reviewer/looking-for.md
Normal file
96
zh-cn/review/reviewer/looking-for.md
Normal file
@ -0,0 +1,96 @@
|
||||
# Code Review 要点
|
||||
|
||||
注意：在考虑这些要点时，请谨记 “[Code Review 标准](standard.md)”。
|
||||
|
||||
## 设计
|
||||
|
||||
审查中最重要的是 CL 的整体设计。CL 中各种代码的交互是否有意义？此变更是属于您的代码库（codebase）还是属于库（library）？它是否与您系统的其他部分很好地集成？现在是添加此功能的好时机吗？
|
||||
|
||||
## 功能
|
||||
|
||||
这个 CL 是否符合开发者的意图？开发者的意图对代码的用户是否是好的？ “用户”通常都是最终用户（当他们受到变更影响时）和开发者（将来必须“使用”此代码）。
|
||||
|
||||
大多数情况下，我们希望开发者能够很好地测试 CL，以便在审查时代码能够正常工作。但是，作为审查者，仍然应该考虑边缘情况，寻找并发问题，尝试像用户一样思考，并确保您单纯透过阅读方式审查时，代码没有包含任何 bug。
|
||||
|
||||
当要检查 CL 的行为会对用户有重大影响时，验证 CL 的变化将变得十分重要。例如 **UI 变更**。当您只是阅读代码时，很难理解某些变更会如何影响用户。如果在 CL 中打 patch 或自行尝试这样的变更太不方便，您可以让开发人员为您提供功能演示。
|
||||
|
||||
另一个在代码审查期间特别需要考虑功能的时机，就是如果 CL 中存在某种**并行编程**，理论上可能导致死锁或竞争条件。通过运行代码很难检测到这些类型的问题，并且通常需要某人（开发者和审查者）仔细思考它们以确保不会引入问题。 （请注意，这也是在可能出现竞争条件或死锁的情况下，不使用并发模型的一个很好的理由——它会使代码审查或理解代码变得非常复杂。）
|
||||
|
||||
## 复杂度
|
||||
|
||||
CL 是否已经超过它原本所必须的复杂度？针对任何层级的 CL 请务必确认这点——每行程序是否过于复杂？ 功能太复杂了吗？类太复杂了吗？ “太复杂”通常意味着**“阅读代码的人无法快速理解**。”也可能意味着**“开发者在尝试调用或修改此代码时可能会引入错误。”**
|
||||
|
||||
其中一种复杂性就是**过度工程（over-engineering）**，如开发人员使代码过度通用，超过它原本所需的，或者添加系统当前不需要的功能。审查者应特别警惕过度工程。未来的问题应该在它实际到达后解决，且届时才能更清晰的看到其真实样貌及在现实环境里的需求，鼓励开发人员解决他们现在需要解决的问题，而不是开发人员推测**可能**需要在未来解决的问题。
|
||||
|
||||
## 测试
|
||||
|
||||
将要求单元、集成或端到端测试视为应该做的适当变更。通常，除非 CL 处理[紧急情况](../emergencies.md)，否则应在与生产代码相同的 CL 中添加测试。
|
||||
|
||||
确保 CL 中的测试正确，合理且有用。测试并非用来测试自己本身，且我们很少为测试编写测试——人类必须确保测试有效。
|
||||
|
||||
当代码被破坏时，测试是否真的会失败？ 如果代码发生变化时，它们会开始产生误报吗？ 每个测试都会做出简单而有用的断言吗？ 不同测试方法的测试是否适当分开？
|
||||
|
||||
请记住，测试也是必须维护的代码。不要仅仅因为它们不是主二进制文件的一部分而接受测试中的复杂性。
|
||||
|
||||
## 命名
|
||||
|
||||
开发人员是否为所有内容选择了好名字？ 一个好名字应该足够长，可以完全传达项目的内容或作用，但又不会太长，以至于难以阅读。
|
||||
|
||||
## 注释
|
||||
|
||||
开发者是否用可理解的英语撰写了清晰的注释？所有注释都是必要的吗？通常，注释**解释为什么**某些代码存在时很有用，且不应该用来解释某些代码正在做什么。如果代码无法清楚到去解释自己时，那么代码应该变得更简单。有一些例外（正则表达式和复杂算法通常会从解释他们正在做什么事情的注释中获益很多），但大多数注释都是针对代码本身可能无法包含的信息，例如决策背后的推理。
|
||||
|
||||
查看此 CL 之前的注释也很有帮助。 也许有一个 TODO 现在可以删除，一个注释建议不要进行这种改变，等等。
|
||||
|
||||
请注意，注释与类、模块或函数的**文档**不同，它们应该代表一段代码的目的，如何使用它，以及使用时它的行为方式。
|
||||
|
||||
## 风格
|
||||
|
||||
Google 提供了所有主要语言的[风格指南](http://google.github.io/styleguide/)，甚至包括大多数小众语言。确保 CL 遵循适当的风格指南。
|
||||
|
||||
如果您想改进风格指南中没有的一些样式点，请在评论前加上“Nit：”，让开发人员知道这是您认为可改善代码的小瑕疵，但不是强制性的。不要仅根据个人风格偏好阻止提交 CL。
|
||||
|
||||
CL 的作者不应在主要风格变更中，包括与其他种类的变更。它会使得很难看到 CL 中的变更了什么，使合并和回滚更复杂，并导致其他问题。例如，如果作者想要重新格式化整个文件，让他们只将重新格式化变为一个 CL，其后再发送另一个包含功能变更的 CL。
|
||||
|
||||
## 文档
|
||||
|
||||
如果 CL 变更了用户构建、测试、交互或发布代码的方式，请检查相关文档是否有更新，包括 README、g3doc 页面和任何生成的参考文档。如果 CL 删除或弃用代码，请考虑是否也应删除文档。 如果缺少文档，请询问。
|
||||
|
||||
## 每一行 {#every_line}
|
||||
|
||||
查看分配给您审查的**每行**代码。有时如数据文件、生成的代码或大型数据结构等东西，您可以快速扫过。但不要快速扫过人类编写的类、函数或代码块，并假设其中的内容是 OK 的。显然，某些代码需要比其他代码更仔细的审查——这是您必须做出的判断——但您至少应该确定您**理解**所有代码正在做什么。
|
||||
|
||||
如果您觉得这些代码太难以阅读了并减慢您审查的速度，您应该在您尝试继续审核前要让开发者知道这件事，并等待他们为程序做出解释、澄清。在 Google，我们聘请了优秀的软件工程师，您就是其中之一。如果您无法理解代码，那么很可能其他开发人员也不会。因此，当您要求开发人员澄清此代码时，您也会帮助未来的开发人员理解这些代码。
|
||||
|
||||
如果您了解代码但觉得没有资格做某些部分的审查，请确保 CL 上有一个合格的审查人，特别是对于安全性、并发性、可访问性、国际化等复杂问题。
|
||||
|
||||
## 上下文
|
||||
|
||||
在广泛的上下文下查看 CL 通常很有帮助。通常，代码审查工具只会显示变更的部分的周围的几行。有时您必须查看整个文件以确保变更确实有意义。例如，您可能只看到添加了四行新代码，但是当您查看整个文件时，您会看到这四行是添加在一个 50 行的方法里，现在确实需要将它们分解为更小的方法。
|
||||
|
||||
在整个系统的上下文中考虑 CL 也很有用。 这个 CL 是否改善了系统的代码健康状况，还是使整个系统更复杂，测试更少等等？**不要接受降低系统代码运行状况的 CL**。大多数系统通过许多小的变化而变得复杂，因此防止新变更引入即便很小的复杂性也非常重要。
|
||||
|
||||
## 好的事情 {#good_things}
|
||||
|
||||
如果您在 CL 中看到一些不错的东西，请告诉开发者，特别是当他们以一种很好的方式解决了您的的一个评论时。代码审查通常只关注错误，但也应该为良好实践提供鼓励。在指导方面，比起告诉他们他们做错了什么，有时更有价值的是告诉开发人员他们做对了什么。
|
||||
|
||||
## 总结
|
||||
|
||||
在进行代码审查时，您应该确保：
|
||||
|
||||
- 代码设计精良。
|
||||
- 该功能对代码用户是有好处的。
|
||||
- 任何 UI 变更都是合理的且看起来是好的。
|
||||
- 其中任何并行编程都是安全的。
|
||||
- 代码并不比它需要的复杂。
|
||||
- 开发人员没有实现他们将来**可能**需要，但不知道他们现在是否需要的东西。
|
||||
- 代码有适当的单元测试。
|
||||
- 测试精心设计。
|
||||
- 开发人员使用了清晰的名称。
|
||||
- 评论清晰有用，且大多用来解释**为什么**而不是**做什么**。
|
||||
- 代码有适当记录成文件（通常在 g3doc 中）。
|
||||
- 代码符合我们的风格指南。
|
||||
|
||||
确保查看您被要求查看的**每一行**代码，查看**上下文**，确保您**提高代码健康状况**，并赞扬开发人员所做的**好事**。
|
||||
|
||||
下一篇：[查看 CL 的步骤](navigate.md)
|
||||
36
zh-cn/review/reviewer/navigate.md
Normal file
36
zh-cn/review/reviewer/navigate.md
Normal file
@ -0,0 +1,36 @@
|
||||
# 查看 CL 的步骤
|
||||
|
||||
## 总结
|
||||
|
||||
现在您已经知道了 [Code Review 要点](looking-for.md)，那么管理分布在多个文件中的评论的最有效方法是什么？
|
||||
|
||||
1. 变更是否有意义？它有很好的描述吗？
|
||||
1. 首先看一下变更中最重要的部分。整体设计得好吗？
|
||||
1. 以适当的顺序查看 CL 的其余部分。
|
||||
|
||||
## 第一步：全面了解变更 {#step_one}
|
||||
|
||||
查看 [CL 描述](https://google.github.io/eng-practices/review/developer/cl-descriptions.html)和 CL 大致上用来做什么事情。这种变更是否有意义？如果在最初不应该发生这样的变更，请立即回复，说明为什么不应该进行变更。当您拒绝这样的变更时，向开发人员建议应该做什么也是一个好主意。
|
||||
|
||||
例如，您可能会说“看起来你已经完成一些不错的工作，谢谢！但实际上，我们正朝着删除您在这里修改的 FooWidget 系统的方向演进，所以我们不想对它进行任何新的修改。不过，您来重构下新的 BarWidget 类怎么样？“
|
||||
|
||||
请注意，审查者不仅拒绝了当前的 CL 并提供了替代建议，而且他们保持礼貌地这样做。这种礼貌很重要，因为我们希望表明，即使不同意，我们也会相互尊重。
|
||||
|
||||
如果您获得了多个您不想变更的 CL，您应该考虑重整开发团队的开发过程或外部贡献者的发布过程，以便在编写CL之前有更多的沟通。最好在他们完成大量工作之前说“不”，避免已经投入心血的工作现在必须被抛弃或彻底重写。
|
||||
|
||||
## 第二步：检查 CL 的主要部分 {#step_two}
|
||||
|
||||
查找作为此 CL “主要”部分的文件。通常，包含大量的逻辑变更的文件就是 CL 的主要部分。先看看这些主要部分。这有助于为 CL 的所有较小部分提供上下文，并且通常可以加速代码审查。如果 CL 太大而无法确定哪些部分是主要部分，请向开发人员询问您应该首先查看的内容，或者要求他们[将 CL 拆分为多个 CL](../developer/small-cls.md)。
|
||||
|
||||
如果在该部分发现存在一些主要的设计问题时，即使没有时间立即查看 CL 的其余部分，也应立即留下评论告知此问题。因为事实上，因为该设计问题足够严重的话，继续审查其余部分很可能只是浪费宝贵的时间，因为其他正在审查的程序可能都将无关或消失。
|
||||
|
||||
立即发送这些主要设计评论非常重要，有两个主要原因：
|
||||
|
||||
- 通常开发者在发出 CL 后，在等待审查时立即开始基于该 CL 的新工作。如果您正在审查的 CL 中存在重大设计问题，那么他们以后的 CL 也必须要返工。您应该赶在他们在有问题的设计上做了太多无用功之前通知他们。
|
||||
- 主要的设计变更比起小的变更来说需要更长的时间才能完成。开发人员基本都有截止日期；为了完成这些截止日期并且在代码库中仍然保有高质量代码，开发人员需要尽快开始 CL 的任何重大工作。
|
||||
|
||||
## 第三步：以适当的顺序查看 CL 的其余部分 {#step_three}
|
||||
|
||||
一旦您确认整个 CL 没有重大的设计问题，试着找出一个逻辑顺序来查看文件，同时确保您不会错过查看任何文件。 通常在查看主要文件之后，最简单的方法是按照代码审查工具向您提供的顺序浏览每个文件。有时在阅读主代码之前先阅读测试也很有帮助，因为这样您就可以了解该变更应当做些什么。
|
||||
|
||||
下一篇：[Code Review 速度](speed.md)
|
||||
33
zh-cn/review/reviewer/pushback.md
Normal file
33
zh-cn/review/reviewer/pushback.md
Normal file
@ -0,0 +1,33 @@
|
||||
# 处理 Code Review 中的拖延
|
||||
|
||||
有时开发人员会拖延（Pushback）代码审查。他们要么不同意您的建议，要么抱怨您太严格。
|
||||
|
||||
## 谁是对的？ {#who_is_right}
|
||||
|
||||
当开发人员不同意您的建议时，请先花点时间考虑一下是否正确。通常，他们比你更接近代码，所以他们可能真的对它的某些方面有更好的洞察力。他们的论点有意义吗？从代码健康的角度来看它是否有意义？如果是这样，让他们知道他们是对的，把问题解决。
|
||||
|
||||
但是，开发人员并不总是对的。在这种情况下，审查人应进一步解释为什么认为他们的建议是正确的。好的解释在描述对开发人员回复的理解的同时，还会解释为什么请求更改。
|
||||
|
||||
特别是，当审查人员认为他们的建议会改善代码健康状况时，他们应该继续提倡更改，如果他们认为最终的代码质量改进能够证明所需的额外工作是合理的。**提高代码健康状况往往只需很小的几步。**
|
||||
|
||||
有时需要几轮解释一个建议才能才能让对方真正理解你的用意。只要确保始终保持[礼貌](comments.md#courtesy)，让开发人员知道你有听到他们在说什么，只是你不同意该论点而已。
|
||||
|
||||
## 沮丧的开发者 {#upsetting_developers}
|
||||
|
||||
审查者有时认为，如果审查者人坚持改进，开发人员会感到不安。有时候开发人员会感到很沮丧，但这样的感觉通常只会持续很短的时间，后来他们会非常感谢您在提高代码质量方面给他们的帮助。通常情况下，如果您在评论中表现得很有[礼貌](comments.md#courtesy)，开发人员实际上根本不会感到沮丧，这些担忧都仅存在于审核者心中而已。开发者感到沮丧通常更多地与[评论的写作方式](comments.md#courtesy)有关，而不是审查者对代码质量的坚持。
|
||||
|
||||
## 稍后清理 {#later}
|
||||
|
||||
开发人员拖延的一个常见原因是开发人员（可以理解）希望完成任务。他们不想通过另一轮审查来完成该 CL。所以他们说会在以后的 CL 中清理一些东西，所以您现在应该 LGTM 这个 CL。一些开发人员非常擅长这一点，并会立即编写一个修复问题的后续 CL。但是，经验表明，在开发人员编写原始 CL 后，经过越长的时间这种清理发生的可能性就越小。实际上，通常除非开发人员在当前 CL 之后立即进行清理，否则它就永远不会发生。这不是因为开发人员不负责任，而是因为他们有很多工作要做，清理工作在其他工作中被丢失或遗忘。因此，在代码进入代码库并“完成”之前，通常最好坚持让开发人员现在清理他们的 CL。让人们“稍后清理东西”是代码库质量退化的常见原因。
|
||||
|
||||
如果 CL 引入了新的复杂性，除非是[紧急情况](../emergencies.md)，否则必须在提交之前将其清除。如果 CL 暴露了相关的问题并且现在无法解决，那么开发人员应该将 bug 记录下来并分配给自己，避免后续被遗忘。又或者他们可以选择在程序中留下 TODO 的注释并连结到刚记录下的 bug。
|
||||
|
||||
## 关于严格性的抱怨 {#strictness}
|
||||
|
||||
如果您以前有相当宽松的代码审查，并转而进行严格的审查，一些开发人员会抱怨得非常大声。通常提高代码审查的[速度](speed.md)会让这些抱怨逐渐消失。
|
||||
|
||||
有时，这些投诉可能需要数月才会消失，但最终开发人员往往会看到严格的代码审查的价值，因为他们会看到代码审查帮助生成的优秀代码。而且一旦发生某些事情时，最响亮的抗议者甚至可能会成为你最坚定的支持者，因为他们会看到审核变严格后所带来的价值。
|
||||
|
||||
## 解决冲突 {#conflicts}
|
||||
|
||||
如果上述所有操作仍无法解决您与开发人员之间的冲突，请参阅 “[Code Review 标准](standard.md)”以获取有助于解决冲突的指导和原则。
|
||||
66
zh-cn/review/reviewer/speed.md
Normal file
66
zh-cn/review/reviewer/speed.md
Normal file
@ -0,0 +1,66 @@
|
||||
# Code Review 速度
|
||||
|
||||
## 为什么尽快进行 Code Review？ {#why}
|
||||
|
||||
**在Google，我们优化了开发团队共同开发产品的速度**，而不是优化单个开发者编写代码的速度。个人开发的速度很重要，它并不如整个团队的速度那么重要。
|
||||
|
||||
当代码审查很慢时，会发生以下几件事：
|
||||
|
||||
* **整个团队的速度降低了。**是的，对审查没有快速响应的个人的确完成了其他工作。但是，对于团队其他人来说重要的新功能与缺陷修復将会被延迟数天、数周甚至数月，只因为每个 CL 正在等待审查和重新审查。
|
||||
* **开发者开始抗议代码审查流程。**如果审查者每隔几天只响应一次，但每次都要求对 CL 进行重大更改，那么开发者可能会变得沮丧。通常，开发者将表达对审查者过于“严格”的抱怨。如果审查者请求相同实质性更改（确实可以改善代码健康状况），但每次开发者进行更新时都会快速响应，则抱怨会逐渐消失。**大多数关于代码审查流程的投诉实际上是通过加快流程来解决的。**
|
||||
* **代码健康状况可能会受到影响。**如果审查速度很慢，则造成开发者提交不尽如人意的 CL 的压力会越来越大。审查太慢还会阻止代码清理、重构以及对现有 CL 的进一步改进。
|
||||
|
||||
## Code Review 应该有多快？ {#fast}
|
||||
|
||||
如果您没有处于重点任务的中，那么您应该在**收到代码审查后尽快开始**。
|
||||
|
||||
**一个工作日**是应该响应代码审查请求所需的最长时间（即第二天早上的第一件事）。
|
||||
|
||||
遵循这些指导意味着典型的 CL 应该在一天内进行多轮审查（如果需要）。
|
||||
|
||||
## 速度 vs. 中断 {#interruption}
|
||||
|
||||
有一种情况下个人速度胜过团队速度。**如果您正处于重点任务中，例如编写代码，请不要打断自己进行代码审查。**研究表明，开发人员在被打断后需要很长时间才能恢复到顺畅的开发流程中。因此，编写代码时打断自己实际上比让另一位开发人员等待代码审查的代价更加昂贵。
|
||||
|
||||
相反，在回复审查请求之前，请等待工作中断点。可能是当你的当前编码任务完成，午餐后，从会议返回，从厨房回来等等。
|
||||
|
||||
## 快速响应 {#responses}
|
||||
|
||||
当我们谈论代码审查的速度时，我们关注的是响应时间，而不是 CL 需要多长时间才能完成整个审查并提交。理想情况下，整个过程也应该是快速的，**快速的个人响应比整个过程快速发生更为重要**。
|
||||
|
||||
即使有时需要很久才能完成整个审查流程，但在整个过程中获得审查者的快速响应可以显着减轻开发人员对“慢速”代码审查感到的挫败感。
|
||||
|
||||
如果您太忙而无法对 CL 进行全面审查，您仍然可以发送快速回复，让开发人员知道您什么时候可以开始，或推荐其他能够更快回复的审查人员，或者[提供一些大体的初步评论](navigate.md)。 （注意：这并不意味着您应该中断编码，即使发送这样的响应，也要在工作中的合理断点处发出响应。）
|
||||
|
||||
重要的是，审查人员要花足够的时间进行审查，确信他们的“LGTM”意味着“此代码符合我们的[标准](standard.md)。”但是，理想情况下，个人反应仍然应该很[快](#fast)。
|
||||
|
||||
## 跨时区审查 {#tz}
|
||||
|
||||
在处理时区差异时，尝试在他们还在办公室时回复作者。 如果他们已经下班回家了，那么请确保在第二天回到办公室之前完成审查。
|
||||
|
||||
## 带评论的 LGTM {#lgtm-with-comments}
|
||||
|
||||
为了加快代码审查，在某些情况下，即使他们也在 CL 上留下未解决的评论，审查者也应该给予 LGTM/Approval，这可以是以下任何一种情况：
|
||||
|
||||
- 审查者确信开发人员将适当地处理所有审查者的剩余评论。
|
||||
- 其余的更改很小，不必由开发者完成。
|
||||
|
||||
如果不清楚的话，审查者应该指定他们想要哪些选项。
|
||||
|
||||
当开发者和审查者处于不同的时区时，带评论的 LGTM 尤其值得考虑，否则开发者将等待一整天才能获得 “LGTM，Approval”。
|
||||
|
||||
## 大型 CL {#large}
|
||||
|
||||
如果有人向您发送了代码审查太大，您不确定何时有时间查看，那么您应该要求开发者[将 CL 拆分为几个较小的 CL](../developer/small-cls.md) 而不是一次审查的一个巨大的 CL。这通常可行，对审查者非常有帮助，即使需要开发人员的额外工作。
|
||||
|
||||
如果 CL 无法分解为较小的 CL，并且您没有时间快速查看整个内容，那么至少要对 CL 的整体设计写一些评论并将其发送回开发人员以进行改进。作为审查者，您的目标之一应该在不牺牲代码健康状况的前提下，始终减少开发者能够快速采取某种进一步的操作的阻力。
|
||||
|
||||
## 代码审查随时间推移而改进 {#time}
|
||||
|
||||
如果您遵循这些准则，并且您对代码审查非常严格，那么您应该会发现整个代码审核流程会随着时间的推移而变得越来越快。开发者可以了解健康代码所需的内容，并向您发送从一开始就很棒的 CL，且需要的审查时间越来越短。审查者学会快速响应，而不是在审查过程中添加不必要的延迟。但是，从长远来看，**不要为了提高想象中的代码审查速度，而在[代码审查标准](standard.md)或质量方面妥协，实际上这样做对于长期来说不会有任何帮助。**
|
||||
|
||||
## 紧急情况
|
||||
|
||||
还有一些[紧急情况](../emergencies.md)，CL 必须非常快速地通过整个审查流程，并且质量准则将放宽。请查看[什么是紧急情况？]((../emergencies.md#what)) 中描述的哪些情况属于紧急情况，哪些情况不属于紧急情况。
|
||||
|
||||
下一篇：[如何撰写 Code Review 评论](comments.md)
|
||||
46
zh-cn/review/reviewer/standard.md
Normal file
46
zh-cn/review/reviewer/standard.md
Normal file
@ -0,0 +1,46 @@
|
||||
# Code Review 标准
|
||||
|
||||
代码审查的主要目的是确保逐步改善 Google 代码库的整体健康状况。代码审查的所有工具和流程都是为此而设计的。
|
||||
|
||||
为了实现此目标，必须做出一系列权衡。
|
||||
|
||||
首先，开发人员必须能够对任务进行**改进**。如果开发者从未向代码库提交过代码，那么代码库的改进也就无从谈起。此外，如果审核人员对代码吹毛求疵，那么开发人员以后也很难再做出改进。
|
||||
|
||||
另外，审查者有责任确保随着时间的推移，CL 的质量不会使代码库的整体健康状况下降。这可能很棘手，因为通常情况下，代码库健康状况会随着时间的而下降，特别是在对团队有严格的时间要求时，团队往往会采取捷径来达成他们的目标。
|
||||
|
||||
此外，审查者应对正在审核的代码负责并拥有所有权。审查者希望确保代码库保持一致、可维护及 [Code Review 要点](looking-for.md)中所提及的所有其他内容。
|
||||
|
||||
因此，我们将以下规则作为 Code Review 中期望的标准：
|
||||
|
||||
**一般来说，审核人员应该倾向于批准 CL，只要 CL 确实可以提高系统的整体代码健康状态，即使 CL 并不完美。**
|
||||
|
||||
这是所有 Code Review 指南中的**高级**原则。
|
||||
|
||||
当然，也有一些限制。例如，如果 CL 添加了审查者认为系统中不需要的功能，那么即使代码设计良好，审查者依然可以拒绝批准它。
|
||||
|
||||
此处有一个关键点就是没有“完美”的代码，只有**更好的**代码。审查者不该要求开发者在批准程序前仔细清理、润色 CL 每个角落。相反，审查者应该在变更的重要性与取得进展之间取得平衡。审查者不应该追求完美，而应是追求持续改进。不要因为一个 CL不是“完美的”，就将可以提高系统的可维护性、可读性和可理解性的 CL 延迟数天或数周才批准。
|
||||
|
||||
审核者应该随时在可以改善的地方留下审核评论，但如果评论不是很重要，请在评论语句前加上“Nit：”之类的内容，让开发者知道这条评论是用来指出可以润色的地方，而他们可以选择是否忽略。
|
||||
|
||||
注意：本文档中没有任何内容证明检查 CL 肯定会使系统的整体代码健康状况恶化。您会做这中事情应该只有在[紧急情况](../emergencies.md)时。
|
||||
|
||||
## 指导
|
||||
|
||||
代码审查具有向开发人员传授语言、框架或通用软件设计原则新内容的重要功能。留下评论可以帮助开发人员学习新东西，这总归是很好的。分享知识是随着长年累月改善系统代码健康状况的一部分。请记住，如果您的评论纯粹是教育性的，且对于本文档中描述的标准并不重要，请在其前面添加“Nit：”或以其他方式表明作者不必在此 CL 中解决它。
|
||||
|
||||
## 原则 {#principles}
|
||||
|
||||
* 基于技术事实和数据否决意见和个人偏好。
|
||||
* 关于代码风格问题，[风格指南](http://google.github.io/styleguide/)是绝对权威。任何不在风格指南中的纯粹风格点（例如空白等）都是个人偏好的问题。代码风格应该与风格指南中的一致。如果没有以前的风格，请接受作者的风格。
|
||||
* **软件设计方面几乎不是纯粹的风格或个人偏好问题。**软件设计基于基本原则且应该权衡这些原则，而不仅仅是个人意见。有时候会有多种有效的选择。如果作者可以证明（通过数据或基于可靠的工程原理）该方法同样有效，那么审查者应该接受作者的偏好。否则，就要取决于软件设计的标准原则。
|
||||
* 如果没有其他适用规则，则审查者可以要求作者与当前代码库中的内容保持一致，只要不恶化系统的整体代码健康状况即可。
|
||||
|
||||
## 解决冲突 {#conflicts}
|
||||
|
||||
如果在代码审查过程中有任何冲突，第一步应该始终是开发人员和审查者根据本文档中的 [CL 开发者指南](../developer/)和[审查者指南](index.md)达成共识。
|
||||
|
||||
当达成共识变得特别困难时，审阅者和开发者可以进行面对面的会议，或者有 VC 参与调停，而不仅仅是试着通过代码审查评论来解决冲突。 （但是，如果您这样做了，请确保在 CL 的评论中记录讨论结果，以供将来的读者使用。）
|
||||
|
||||
如果这样还不能解决问题，那么解决该问题最常用方法是将问题升级。通常是将问题升级为更广泛的团队讨论，有一个 TL 权衡，要求维护人员对代码作出决定，或要求工程经理的帮助。 **不要因为 CL 的开发者和审查者不能达成一致，就让 CL 在那里卡壳。**
|
||||
|
||||
下一篇：[Code Review 要点](looking-for.md)
|
||||
Loading…
Reference in New Issue
Block a user