https://thenewstack.io/linkedin-code-review/
our engineers use both giving and receiving code reviews as opportunities to grow professionally. In fact, high-quality code reviews are an important part of LinkedIn’s promotion process because they provide objective evidence of engineering skill.
http://www.raychase.net/4772
https://segmentfault.com/a/1190000002575050
our engineers use both giving and receiving code reviews as opportunities to grow professionally. In fact, high-quality code reviews are an important part of LinkedIn’s promotion process because they provide objective evidence of engineering skill.
Do I Understand the “Why”?
To facilitate the best review possible and help your team scale, every code change submission should include a design overview that briefly explains the motivation behind the change. It is really hard to offer a high-quality code review when the rationale needs to be inferred from the code change itself. It is fair to ask and expect the submitter to explain their motivation before attempting the code review. This also encourages the submitter to have an explanation in their commit message, increasing the quality of code documentation.
Am I Giving Positive Feedback?
When a reviewer sees good stuff in the code, they should call it out and give positive feedback. This helps improve team dynamics, and often such positive feedback is contagious. As with all code review comments (more on this below), any positive feedback should be specific, explaining why that particular code is well-written.
Is My Code Review Comment Explained Well?
Whether the feedback is positive or negative, any code review comment should be self-explanatory. What might seem obvious to the reviewer can be unclear to an engineer who receives poorly-explained code review comments. When in doubt, it is better to over-explain than to provide terse feedback that yields more questions and the need for more back-and-forth communication. Explanations can be as simple as “reduces duplication,” “improves coverage,” or “makes code easier to test.” In addition to making reviewers’ comments clearer, these types of explanations also help reinforce the design principles that the team aspires to meet.
Do I Appreciate the Submitter’s Effort?
The best way to show appreciation is to put effort into your code review by giving high-quality feedback with decent explanations, acknowledging the good ideas (there are always good things in every code submission!), and using “thank you.”
Would This Review Comment Be Useful to Me?
Asking this question is an easy and powerful way to validate if the code review comment is necessary. At the end of the day, engineers should view code reviews as helpful development tools, and not sources of unimportant busywork. If you don’t think a particular review comment would be useful to you, then remove it. A classic example of unhelpful code review comments are ones related to code formatting. Code style and formatting should be validated by automated tools, not engineers.
Is the “Testing Done” Section Thorough Enough?
Some changes may require running manual testing if the integration test coverage is inadequate. In those cases, “testing done” should include information about the test scenarios and the outputs. When changes alter the output of the program, it is very useful to include the new output in the “testing done” section
Am I Too Pedantic in My Review?
Some code reviews have so many comments that important issues — ones that really need to be fixed — are lost among less important suggestions. Reviews that are too heavy on details for a given team can slow down the review cycle and cause friction for both the reviewers and reviewee. Having clear review expectations, example reviews, and a positive, inviting review culture are great ways to avoid lengthy, exhausting review cycles.
http://www.raychase.net/4772
因此“一致”这个词一定是在一定程度内的大体描述而已，并非越一致约好。其实，代码的创造本就是具备个性的。毫无疑问我们应当遵从规约，应当符合习惯，但是一旦试图过度使用它们去约束代码，不但难以落实，而且容易产生无趣、低效和矛盾的氛围。
再回到代码评审上。代码评审本身，以及基于评审意见的来回沟通，和代码本身比起来，要更难以，也更不应该要求一致。我见过许多代码评审的风格，有人喜欢挑小毛病，有人喜欢展开观点夸夸其谈，有人喜欢给实际例子来证明自己的看法……无论哪一种风格，我都不觉得有什么大的问题。但是，就我个人而言，我可以谈谈我自己的代码评审风格：
我会关注三种问题，需求和业务上的问题、代码结构的问题、代码风格格式的问题。
坚定自己的意见，但是委婉地表达
关于这一点，我也在努力地改进。可以提及的点很多，技巧也很多，但是老实说，冲突还是不可避免。在我经历的几乎所有的团队中，有时候会有老好人，但是基本上所有的老好人都缺少对于原则的坚持。沟通是门艺术，在代码评审中也一样体现。
- 最重要的一条，只针对代码，不针对人。这条很简单，都知道对事不对人的重要性，但是要非常小心不能违背。
- 对于大多数代码风格格式上的问题，我都会标注这个问题是一个picky或者nit的问题（“挑剔的问题”，这是我在Amazon的时候学到的方式）。这样的好处在于，明确告知对方，我虽然提出了这个问题，但是它没有什么大不了的，如果你坚持不改，我也不打算说服你。或者说，我对这个问题持有不同的看法，但是我也并不坚信我的提议更好。
- 使用也许、或许、可能、似乎这样表示不确定的语气词（包括使用虚拟语气）。这样的好处是，缓和自己观点表达的语气。比如：“这个地方重构一下，去掉这个循环，也许会更好”。
- 间接地表达质疑。比如说，看到对方用了一个参数3，但是你觉得不对，但又不很确定，可以这样说：“我有一个疑问，为什么这里要使用3而不是其他值？”对方可能会反应过来这个值选取得不够恰当。
- 放上例子、讨论的链接，以及其它一些辅助材料证明自己的观点，但是不要直接表述观点，让对方来确认这个观点。比如：“下面的讨论是关于这个逻辑的另一种实现方式，不知你觉得是否会稍简洁一些？”
- 先肯定，再否定。这个我想很多人一直都在用，先摆事实诚恳地说一些同意和正面的话，然后用however一转，说出相反的情况，这样也就在言论中比较了pros和cons，意味着这是经过trade-off得出的结论。
一些很常见的可以在代码评审中提及的问题
这样的问题其实有不少，多数和实现的技术无关，但是又很容易不小心略过。它们多数时候是问题，当然也有时候不是。
- 比如说，我最痛恨的之一，职责过于宽泛或者职责不清的类或模块。我无数次见过这样的类：Helper，或者Utils（注意，它们都没有模型或者模块前缀）。考虑项目的规模，大多数情况下，这样的类很容易变成一个越吹越大的气球，似乎什么东西都可以往里搁，是个十足的违反单一职责原则的糟糕设计。
- 比如说，在线程使用，容器使用，连接管理……中，缺乏上限控制的设计，在一些情况下导致资源使用过度膨胀。生产者和消费者的队列设计，一旦消费者挂掉或者跟不上，队列里越堆越多，没有拒绝机制。
- 再比如说，缺少分包、分层，所有的东西都叠在一起，十几个，甚至几十个类文件并列在同一个文件夹下面。
- 再再比如说，代码缺乏设计，流水账结构，面条型代码，或者简单铺陈几个过程式的方法，这种修改往往代价还不小，自然修改的落实率低，因而令提出问题的人也颇为头疼。
避免一次评审提及过多的问题
少数情况下，手里拿到一份实在是问题多到令人发指的代码，这时候需要扼制自己想骂人的冲动。先抓问题的主干，不要去挑那些细枝末节的毛病，因为很有可能，这些代码会被改得面目全非，或者重写。写一大堆评审意见，反而容易淹没最重要的问题。
说说容易，但是这个度有时候并不好掌握。我的习惯是，在明确代码要解决的问题以后，先快速走读一遍代码，判断我是应该粗略地抓主要矛盾呢，还是应该严格地挑刺呢。我也见过一些别的方式。
不一样的评审要求
我始终觉得，代码评审的要求不应该完全一致。不要过度追求公平。对于新项目代码，以及新员工写的代码，要适当严格一些。
新项目的代码是形成后续风格和质量的模板。我们也许都听说过“破窗户原理”，在一块干干净净的代码田地里，新耕种的代码才容易保持一致，也可以避免一些“为了和现有代码风格保持一致”而导致质量妥协的情况出现。
新员工代码的高质量要求则更多地在于对他们良好习惯养成的负责。软件工程师良好职业习惯的养成，代码可以说是最重要一环，而前三个月的影响举足轻重。
还不如不做的评审
有些情况下，代码评审是非常耗时费力的工作。特别是对业务背景不熟悉，对实现技术不熟悉，或者干脆是对方提交上来一大堆修改，阅读非常费力。我不知道哪一种是最难的，但是如果因为这些原因很难做到良好的评审，我会在评审中说明，或者放弃一些评审的工作，保证评审的质量。
代码评审是建立在团队中影响的好方式。一方面是业务逻辑，一方面是代码结构，我反对在两方面都难以给出足够清晰的评审意见的时候，提一堆风格方面的次要问题。否则很容易达成负面影响。
http://www.cnblogs.com/lazio10000/p/5352239.html提交Code Review之前要做什么？
- 准备或者提交相关需求文档以备审查者询问
- 编写符合规范的代码和合适的注释
- 考虑代码是否有重构的可能
- 单元测试全部通过，测试覆盖率达标
如何Code Review？
- 了解需求：这个提交是为了解决什么问题，是需求单、BUG修复、还是代码重构，
如果不明确，需要及时和代码作者沟通和讨论 - 检查代码业务逻辑是否符合需求
- 代码是否符合相关代码规范
- 确认是否有更好的方式方法重构代码
- 检查单元测试用例是否考虑全面
https://segmentfault.com/a/1190000002575050
Approach to writing a Pull Request
- Include the purpose of this Pull Request. For example:
This is a spike to explore…This simplifies the display of…This fixes handling of… - Consider providing an overview of why the work is taking place (with any relevant links); don’t assume familiarity with the history.
- Remember that anyone in the company could be reading this Pull Request, so the content and tone may inform people other than those taking part, now or later.
- Be explicit about what feedback you want, if any: a quick pair of
on the code, discussion on the technical approach, critique on design, a review of copy. - Be explicit about when you want feedback, if the Pull Request is work in progress, say so. A prefix of “[WIP]” in the title is a simple, common pattern to indicate that state.
- @mention individuals that you specifically want to involve in the discussion, and mention why. (“/cc @jesseplusplus for clarification on this logic”)
- @mention teams that you want to involve in the discussion, and mention why. (“/cc @github/security, any concerns with this approach?”)
Offering feedback
- Familiarize yourself with the context of the issue, and reasons why this Pull Request exists.
- If you disagree strongly, consider giving it a few minutes before responding; think before you react.
- Ask, don’t tell. (“What do you think about trying…?” rather than “Don’t do…”)
- Explain your reasons why code should be changed. (Not in line with the style guide? A personal preference?)
- Offer ways to simplify or improve code.
- Avoid using derogatory terms, like “stupid”, when referring to the work someone has produced.
- Be humble. (“I’m not sure, let’s try…”)
- Avoid hyperbole. (“NEVER do…”)
- Aim to develop professional skills, group knowledge and product quality, through group critique.
- Be aware of negative bias with online communication. (If content is neutral, we assume the tone is negative.) Can you use positive language as opposed to neutral?
- Use emoji to clarify tone. Compare “
Looks good 
” to “Looks good.”
Responding to feedback
- Consider leading with an expression of appreciation, especially when feedback has been mixed.
- Ask for clarification. ("I don’t understand, can you clarify?")
- Offer clarification, explain the decisions you made to reach a solution in question.
- Try to respond to every comment.
- Link to any follow up commits or Pull Requests. (“Good call! Done in 1682851”)
- If there is growing confusion or debate, ask yourself if the written word is still the best form of communication. Talk (virtually) face-to-face, then mutually consider posting a follow-up to summarize any offline discussion (useful for others who be following along, now or later).
These guidelines were inspired partly by Thoughtbot's code review guide.
