4、代码审查内容
Note: Always make sure to take into account The Standard of Code Review 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.
多数情况下，我们希望开发人员在提交代码审核前能对要代码做足够的测试。然而，作为评审人，你仍需考虑边界情况、看下是否有并发问题、试着从用户的角度去思考，以此来确保不会存在那种仅通过阅读代码就能发现的bug。
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.
如果有需要，评审人也可以去验证提交上来的代码的正确性，如果某次提审的代码变更涉及到影响用户的直观体验（例如UI变了），那么此时评审人去检查该部分的代码是很重要的。如果只是去阅读这些代码，评审人很难想象这些变更会对用户造成什么样的影响。对于这类变更，如果评审人不方便在提审的代码变更里自己做测试，可以让开发人员给一个该功能的样例。
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.”
该次代码变更是否过于复杂了？可以从不同层面去对其进行检查：某些代码行是不是太复杂了？某些函数是不是太复杂了？某些类是不是太复杂了？这里的“太复杂”的定义通常是：无法被阅读代码的人快速理解。 它也可以定义为：开发人员如果尝试调用或者修改该代码，将很容易引入bug。
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.
检查开发人员是否已为该次代码变更做了相应的单元测试、集成测试或者端到端测试。通常情况下，测试应该被加入到变更记录里作为产品代码的一部分，除非这个变更是为了处理某次紧急情况
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?
当生产代码发生变更时，这些测试代码是否还能运行？这些测试代码发生变更时，是否会产生误报？每个测试是否都做了简单但有用的断言（assertions）？测试案例是否恰当地分开在不同的方法里了？
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.
有时也可以看下代码里现有的注释。可能有个现在可以移除的TODO，有条建议不要进行此变更的注释，等等。
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 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.
在Google内部，我们为所用到的所有主流语言甚至是多数非主流语言都制定了相应的规范规约。要确保该次代码变更遵从了恰当的代码规范规约。
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.
如果你想改善某些不在规约规范里的点，那么在留言评论时可以在前面加上“Nit：”，以此来让开发人员知道这个只是你个人认为的可以改进的地方，而不是强制性的。不能仅因为个人的规范风格喜好的原因而阻断了代码变更的提交通过。
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.
代码变更的作者不应将主要内容为规范变更的代码与其他变更代码混合在一起。这么做会让人难以看出该次代码变更的变更内容是什么，还会使合并和回滚变得复杂，并且引发别的问题。举个例子，如果作者想格式化整个文件，那么应该把格式化作为一次变更提交，然后再将功能性的变更作为一次变更提交。
Consistency（一致性）
What if the existing code is inconsistent with the style guide? Per our code review principles, the style guide is the absolute authority: if something is required by the style guide, the CL should follow the guidelines.
如果现有的代码与规范规约不一致怎么办？根据我们的代码审查原则，规范规约是绝对的权威：如果是规范规约所要求的，那么代码变更就应该遵从。
In some cases, the style guide makes recommendations rather than declaring requirements. In these cases, it’s a judgment call whether the new code should be consistent with the recommendations or the surrounding code. Bias towards following the style guide unless the local inconsistency would be too confusing.
在某些情况下，规范规约只是做出了推荐做法而不是做强制要求。在这些情况下，需要判断新代码是否要与推荐做法或者现有其它代码保持一致。如果这个不一致性争议性较强，则应该遵从规范规约。
If no other rule applies, the author should maintain consistency with the existing code. Either way, encourage the author to file a bug and add a TODO for cleaning up existing code.
如果没有其它适用规则，那么作者应该与现有代码保持一致。无论是哪种方式，都应鼓励作者记录下问题并添加一个TODO，以此来清理现有代码。
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.
如果某次代码变更改变了用户构建、测试、交互或者发布代码的方式，那么审查人员也要检查是否同步更新了相关的文档，包括README文档、g3doc页面（g3doc为google内部的文档管理平台）以及任何生成的参考文档。如果该次代码变更删除或者弃用了某些代码，则还要看对应的文档是否也删除了。如果缺失了文档，则应要求作者去补充。
Every Line（每一行）
In the general case, 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.
审查人员在审查过程中如果发觉阅读这些代码很困难，并且使得审查进度慢了下来，那么审查人员应该让开发人员知道这点，等开发人员解释说明清楚后再去审查。在Google，我们雇佣了很多很棒的软件工程师，而审查人员也是他们中的一员。如果审查人员无法理解某段代码，那么其他的工程师也很可能无法理解。所以，当审查人员在让开发人员解释说明某段代码时，也是在帮助以后的开发人员理解这段代码。
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 privacy, security, concurrency, accessibility, internationalization, etc.
如果你理解该部分代码但是又觉得好像不够资格来去做这部分代码审查，那么要确保能有个足够资格的人来去审核它，尤其是面对诸如隐私，安全，并发，可访问性，国际化等复杂问题时。
Exceptions（例外情况）
What if it doesn’t make sense for you to review every line? For example, you are one of multiple reviewers on a CL and may be asked:
如果审查每一行对你来说没有意义怎么办？例如，你是评审人之一然后可能会被要求：
- To review only certain files that are part of a larger change.
- 对于某次大的变更提交，你只需审查某些指定的文件。
- To review only certain aspects of the CL, such as the high-level design, privacy or security implications, etc.
- 只需审查该次变更提交的某些层面，例如高层次的设计，隐私或者安全影响等。
In these cases, note in a comment which parts you reviewed. Prefer giving LGTM with comments.
在这些场景下，在评论里标注你审核了哪些内容。建议可以在评论里给一个“LGTM”（Look Good To Me的缩写，对应到Gerrit就是+1）。
If you instead wish to grant LGTM after confirming that other reviewers have reviewed other parts of the CL, note this explicitly in a comment to set expectations. Aim to respond quickly once the CL has reached the desired state.
如果在确认其他审查人员已经审查了其它部分的情况下，你仍然希望给一个LGTM，那么请在评论里明确写下你的期望建议。目的是为了当有代码变更提交上来时，能做到快速响应。
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.
看一下当前代码变更的上下文对于代码审查会很有帮助。通常情况下，代码审查工具只会展示变更了的那几行代码。有时你需要看下整个文件来确保该次变更是有意义的。例如，在看上下文之前，你可能会觉得就只是加了4行新代码而已，但如果你看下整个文件，你会发现这4行代码是包含在一个50行代码的函数里的，这时候这个函数就必须分割成几个更小的函数了。
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（好的地方）
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.
在做代码审查时，应该审查以下几点：
- 代码是设计良好的。
- 用户（包括终端用户和使用这部分代码的开发人员）是可以放心使用这部分功能的。
- 任何UI上的改动都是有意义的并且看起来还不错。
- 所有的多线程编程都应该是安全的。
- 代码已经足够简洁。
- 不会过度设计，不会实现还用不到的功能。
- 代码里包含着恰当的单元测试。
- 测试代码也是设计良好的。
- 代码里所有用到的命名都做到了见名知意。
- 注释是清晰而且有用的，同时主要是解释了“为什么”而不是“什么”。
- 代码有恰当的文档。
- 代码符合我们的开发规范。
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.
当你被指定去审核代码时，要切实审查了每一行代码，看看上下文，确保你是在帮助提高代码质量，同时对于开发人员做得好的地方，还要对其进行表扬。