Google：如何做codereview？
导语：Google 前⼏天公开了⼀篇⾕歌的⼯程实践⽂档，内容跟 code review 相关，⾥⾯包含了 Google ⼯程师如何进⾏ code review 的内容，以及 code review 指南。笔者将其转译成中⽂，以便⼤家参考学习。
本⽂的名词解释：
cr: code review
cl: change list，指这次改动
reviewer: cr的那个review⼈
nit: 全称nitpick，意思是鸡蛋⾥挑⾻头
作者: 也就是本次CL的开发者，原⽂中是以author来称开发者的。
如果嫌⽂章太长，可以直接拖到⽂章最后看总结
cr的标准
cr(Code review)主要⽬的在于确保Google 的代码库代码质量越来越好。⽽所有相关的⼯具与流程皆是因应这个⽬的⽽⽣。为达到此⽬的，势必需要做出⼀连串的权衡与取舍。
⾸先，开发⼈员必须能够在⾃⼰负责的任务上有所进展。如果你从来没有向代码库提交改进过的代码，那么代码库就永远不会改进。另外，如果⼀个reviewer使cr都很难进⾏的话，那么开发⼈员就不愿意在将来进⾏改进。
另⼀⽅⾯，reviewer有责任确保每个change list（下称CL）的质量，保证其代码库的整体代码质量不会越来越差。这可能很棘⼿，因为通常会随着时间的推移，代码需要降级才能让代码运⾏起来，特别是当团队受到严重的时间限制时，⼤家觉得必须⾛捷径才能实现他们的⽬标。
此外，reviewer对他们正在review的代码拥有所有权和责任。他们希望确保代码保持⼀致、可维护，以及下⽂的“在cr中可以得到什么”中提到的内容。
因此，我们有以下规则，作为我们在cr中所期望的标准：
⼀般来说，当CL存在的时候，reviewer应该赞成它，此时它肯定会提⾼正在⼯作的系统的整体代码质量，即使这个CL并不完美。这是所有cr中的⾸要原则。当然，这是有局限性的。例如，如果⼀个CL添加了⼀个reviewer不希望在其系统中使⽤的功能，那么reviewer当然可以拒绝，即使代码设计得很好。
这⾥的⼀个关键点是，没有“完美”的代码，只有更好的代码。reviewer不应该要求作者在approve之前对⼀篇⽂章的每⼀⼩段进⾏润⾊。相反，reviewer应该权衡发展的需要和他们所建议的change的重要性。reviewer不应追求完美，⽽应追求持续改进。作为⼀个整体，提⾼系统的可维护性、可读性和可理解性的CL不应该因为它不是“完美的”⽽被延迟⼏天或⼏周。
reviewer应该随时可以留下评论，表达⼀些更好的东西，但如果不是很重要，可以在评论前加上“nit:”这样的前缀，让作者知道这只是⼀个他们可以选择忽略的修饰点。
指导
cr有⼀个重要的功能，教开发⼈员⼀些关于语⾔、框架或⼀般软件设计原则的新知识。留下有助于开发⼈员学习新知识的评论是可以的。随着时间的推移，共享知识是提⾼系统代码健康度的⼀部分。你只需要记住，如果你的评论纯粹是教育性的，但对达到本⽂档中描述的标准并不重要，请在其前⾯加上“nit:”，或者以其他⽅式表明作者不必在本CL中解决它。
原则
现实和数据推翻了个⼈喜好。在代码风格⽅⾯，风格指南（）是绝对的权威。任何不在style guide中的⼀些点（如空格等）都是个⼈偏好的问题。代码风格应该与现有的⼀致。如果项⽬没有以前的统⼀风格，那就接受作者的风格。
软件设计的各个⽅⾯从来都不仅仅是⼀个纯粹的代码风格问题或者是个⼈喜好问题。它们是以基本原则为基础的，应当以这些原则为依据，⽽不仅仅是以个⼈意见为依据，有时⼏乎都没有选择的。如果作者能够证明（通过数据或基于原理的⼀些事实）他的⽅法是同样有效的，那么reviewer应该接受作者的偏好。否则，代码风格选择取决于软件设计的标准原则。
如果没有其他规则适⽤，那么reviewer可以要求作者与当前代码库中的内容保持⼀致，只要这些代码不会恶化系统的整体代码健康状况。
解决冲突
在cr的任何冲突中，第⼀步应该始终是开发⼈员和reviewer根据本⽂和，尝试达成共识。
当达成共识变得特别困难时，reviewer和作者需要进⾏⾯对⾯会议，⽽不是仅仅试图通过cr的注释来解决冲突（不过，如果这样做，请确保将讨论结果记录在CL的评论中，以供将来的读者阅读）。
如果这不能解决问题，最常见的解决⽅法就是升级。通常情况下，升级的途径是进⾏更⼴泛的团队讨论，让team leader参与进来，请求代码维护⼈员做出决定，或者请求技术经理提供帮助。不要因为作者和审稿⼈不能达成⼀致意见⽽让⼀个其他⼈袖⼿旁观。
在cr中要看些什么
注意：在考虑每⼀点时，⼀定要考虑cr的标准。
怎样写代码 自己做编程设计
在cr中重要的是看CL的总体设计。CL中不同代码段的交互是否有意义？此更改属于你的业务代码库还是属于引进来的其他代码库？它是否与系统的其他部分很好地集成？现在是添加此功能的合适时机吗？
功能
这个CL做了开发者想要的吗？开发者对这些代码的设计初衷⽤户有好处吗？“⽤户”通常既是最终⽤户（当他们受到更改的影响时）⼜是开发⼈员（他们将来必须“使⽤”此代码）。
⼤多数情况下，我们希望开发⼈员在进⾏cr时能够对CL进⾏充分的测试，使其能够正常⼯作。但是，作为reviewer，仍然应该考虑边缘状况，寻问题，尝试像⽤户⼀样思考，并确保仅通过阅读代码就不会看到错误。
如果你愿意的话，你可以⾃⼰去验证CL。如果改动会直接带来的⽤户可见的影响，⽐如说ui改动，验证CL的变化是⾮常关键的。
在阅读代码时，很难理解某些更改会对⽤户产⽣怎样的影响。对于这样的更改，如果不⽅便⾃⼰测试，则可以让开发⼈员演⽰该功能(demo)。
另外，在cr期间考虑功能性特别重要的点是，cl中是否并发式编程，理论上可能导致死锁或竞争条件。这些类型的问题很难通过运⾏代码来发现，通常需要有⼈（开发⼈员和reviewer）仔细考虑，以确保不会引⼊问题（注意，这也是不使⽤平⾏式编程的⼀个很好的理由，在这种情况下，可能出现竞争条件或死锁，这会使代码检查或理解代码变得⾮常复杂）。
复杂性
⼀个CL是否复杂到超过预期的必须？针对任何层级的CL必须确认这⼏点：单⾏代码是否过于复杂？函数是否过于复杂？class是否过于复杂？“复杂”通常意味着该代码很难阅读，很有可能也意味着其他开发者在修改这段代码的时候引⼊其他bug。
其中⼀种复杂性就是过度设计(Over-engineering)造成的，开发者会让那段代码过度通⽤化，超过了原本所需，或者还新增了系统⽬前不需要的功能。reviewer应特别注意⼀下过度设计。⿎励开发者解决他们知道现在需要解决的问题，⽽不是推测将来可能需要解决的问题。当那些将来出现的问题出现的时候才开始解决它们，因为那时候你可以更加清晰看见问题的原样⼦。
Donald Knuth说过：过早的优化是万恶之源 (Premature optimization is the root of all evil)。
测试
请将单元测试、整合测试、端到端测试视为要求CL所做的适当变更。⼀般CL内除了⽣产环境的业务代码外，测试也应该要被加⼊其中。除⾮该CL是为了处理某个紧急事情⽽存在。
另外，也要确保测试是正确、合理、有⽤的。测试并⾮来测试它们本⾝，⼀般也极少为了测试⽽测试（如测试⼀下测试代码有没有问题⼜⾛了测试流程），因此我们要保证测试是有效的。
当代码真的有问题，测试是否会失败？如果被测试的程序发⽣改动时，测试是否会产⽣误报？每⼀个测试是否做出了简单⽽有⽤的断⾔？不同的测试⽅法之间的测试是否适当分开？
请记住，测试代码也是必须维护的代码，不要因为它们不在主要关注的范围内。
命名
开发者是否为了每⼀个东西都挑了⼀个适当的名字？⼀个好的命名意味着，通过名字就⾜以完整表达该东西的作⽤是什么或者要做什么。但是同时名字也不要长得难以阅读。
推荐参考⽂章：
注释
开发者是否⽤可理解的英⽂留下清晰的注释? 这些注释是否真的必要?
通常注释是解析这段代码为什么存在的时候是相当有⽤的，⽽不应该去解释某段代码正在做什么。如果代码本⾝不能解释清楚的话，意味着它更加需要简化了。当然也有例外，⽐如解释正规的表达式或者复杂的算法正在做什么的时候，注释解释这段代码正在做什么就相当有⽤。但对于⼤部分注释来说是⽤来说明那些不包含在程序本⾝但资讯，⽐如说为什么要这样⼦做的理由。
查看该CL之前的注释也很有帮助，或许有⼀个todo项⽬现在可以⼀处、⼀个评论建议为什么不要进⾏这种更改等等。
要注意的是，注释与class、module、function的⽂件不同。后三者要能够表达⼀段代码的⽬的、如何使⽤它、使⽤时⾏为。
风格
Google 对于主要语⾔都有提供风格指南（style guide），甚⾄包括⼤多数冷门语⾔，因此要确保CL遵循适当的指南上的说明。
如果你想改进CL中某些不包含在风格指南中的要点时，请在评论前加上Nit: ，让开发⼈员知道这是你认为可以改善代码的⼩问题且并⾮强制性的。但记住，不要仅根据个⼈风格偏好阻⽌提交CL。
开发者不应该在 CL内同时包含主要风格的改动与其他代码的修改，这样会导致难以看出CL到底做出什么改动。同时也会让合并和回滚更为复杂，并产⽣其他问题。例如，如果作者想要重新格式化代码的话，让他们将新格式在⼀个新CL⾥⾯重构。
⽂档
如果CL更改了构建、测试、交互、发布的时候，请检查是否也同时更新相关⽂档， 包括README，g3doc 页⾯和其他⽣成(generated)的参考⽂件。如果CL 删除或弃⽤ 了⼀些代码，请考虑是否应该删除对应⽂档，如果缺少⽂档时请询问。
每⼀⾏代码
仔细review分配给你的每⼀⾏代码。有些东西，⽐如资料⽂件(data files)、⽣成的代码(generated code)、⼤型数据结构(large data structures)，你可以稍微扫过。千万不要在扫过开发者写的 class、函数、代码区块 时，去假设它内部是没问题的。很显然的某些代码需要⽐其他代码更仔细的review。这是必须由你做出的判断，但⾄少你应该确定你理解所有代码在做些什么。
如果阅读代码过于复杂并且减慢review速度时，那么你再继续review前，要让开发者知道这件事，并等待他们为这段代码做出解释、说清楚。在Google 我们聘请许多优秀的软件⼯程师，⽽你也是其中的
⼀员。如果连你也⽆法理解的话，很可能其他⼈也不会。因此，你要求开发者去说清楚这段代码时，同时也在帮助未来的开发⼈员理解这些代码。
如果你能够理解，但觉得没有资格进⾏某部分的审核，请确保 reviewer中有⼀个适合(合格) 的⼈来review该部分。尤其是针对安全性、并发性、可访问性、国际化等复杂问题时。
上下⽂
在充⾜的上下⽂下查看CL通常很有帮助。⼀般来说，cr⼯具只会显⽰修改部分周围的⼏⾏代码⽽已。但有时你必须查看整个⽂件以确保改动是否合理。⽐⽅说，你可能只看到添加4⾏新代码，但实际上查看整个⽂件时会发现这4⾏是被加在有50⾏的代码⾥，此时需要将它拆解为更⼩的函数。
以整个系统的⾓度出发来思考CL也是很有⽤的。该CL是否改善整体系统的代码质量，亦或是会让整个系统更加复杂？是否缺少测试？千万不要接受会降低整体系统的代码质量的CL。因为⼤多数系统是由于许多⼩改动的积累⽽变得复杂，因此阻⽌新的改动引⼊复杂性(尽管很⼩)也⾮常重要。
优点
当你在CL⾥看到优点时记得告诉开发者，尤其是当他们⽤很棒的⽅式来解决你的评论时。cr通常只关注存在的错误，但它们也应该同时应该为良好实践提供⿎励与欣赏。这点在指导开发者时尤其重要：
与其告诉他们做错什么，还不如告诉他们做对了什么。
个⼈认为并⾮不要指出错误，⽽是多以⿎励来代替指出错误，让其他开发者更有动⼒想将事情做好。其实透过简单的⼀句话让对⽅知道哪⾥做得很好，未来他们会将继续保持下去，并为其他开发者带来的正⾯影响。
标明每个commit 修改什么，帮助reviewer快速了解情况
此时就不要吝啬你的称赞了!
总结
在cr时，请务必确保：
代码经过完善的设计
功能性对于使⽤者们是好的
对于任何UI改动要合理且好看
任何并⾏编程的实现是安全
代码不应该复杂超过原本所必须的
开发者不该实现⼀个现在不⽤⽽未来可能需要的功能
代码有适当的单元测试
测试经过完善的设计
开发者对于每样东西有使⽤清晰、明了的命名

版权声明：本站内容均来自互联网，仅供演示用，请勿用于商业和其他非法用途。如果侵犯了您的权益请与我们联系QQ:729038198，我们将在24小时内删除。