代码质量评审
代码质量评审
Section titled “代码质量评审”每个评审者在评审标准之外,还要用上这个代码质量视角。这是一套严格的标准,关注实现质量、可维护性、抽象质量和代码库的健康。
最要紧的是,对代码结构要有野心。不要只挑局部能清理的地方。主动去找「代码柔道」式的改法:行为不变,结构重新组织,让实现大幅变得更简单、更小、更直接、更优雅。
从这段基础要求开始:
对当前分支的改动做一次深入的代码质量审查。 重新想一想这些改动该怎样组织、怎样实现,在不影响行为的前提下实打实地提升代码质量。 改进抽象和模块划分,减少意大利面式的代码,让代码更简洁、更好读。 要有野心。如果有一条清楚的路能改进实现,哪怕要重组一部分代码库,也放手去做。 要极其彻底、严谨。量两遍,再下刀。
每个角度只讲一次。用上相关的那些。
-
对结构上的简化要有野心。 不要停在「这里可以再干净一点」。去找换个角度就能让整段分支、辅助函数、模式、条件判断或整一层消失的改法。要假定常常存在「代码柔道」式的一招:它更好地利用现有架构,让改动大幅变简单。如果能删掉复杂度,而不只是挪动它,就使劲推动这样做。
-
没有非常充分的理由,不要让一个 PR 把文件从不到 1000 行撑到超过 1000 行。 把这当成很强的坏味道。优先抽出辅助函数、子组件或模块。如果 diff 越过了这条线,问一句是不是应该先拆分代码。只有在结构上有令人信服的理由、而且结果文件仍然组织得很清楚时,才放行。
-
不要让现有代码长成意大利面。 对新加的临时条件判断、四处散落的特例、塞进无关流程里的一次性分支保持怀疑。把「随便哪儿冒出来的奇怪 if」当成设计问题,而不是风格上的小毛病。优先把这段逻辑放进专门的辅助函数、状态机或模块,而不是缠进现有的路径里。
-
倾向于把设计理干净,而不是代码能跑就收下。 如果行为可以不变、结构却能明显更干净,就推动更干净的版本。宁可选能减少活动部件的简化,也不要把同样的复杂度摊到别处的重构。
-
宁要直接、平淡、好维护的代码,不要取巧或带「魔法」的代码。 把脆弱的、临时拼凑的或「魔法」式的行为当成问题。对那些把简单的数据形状假设藏起来的通用机制保持怀疑。指出那些增加了间接层却没换来清晰度的薄抽象、原样包装或透传辅助函数。
-
类型和边界的干净程度影响可维护性时,要追问。 本可以有更清楚的类型边界时,质疑不必要的可选字段、
unknown、any或到处是类型转换的代码。宁要明确的类型模型,不要形状松散的临时对象。如果某个分支靠一个悄无声息的兜底来掩盖不清楚的 invariant,问一句这个边界是不是该写明确。 -
逻辑放在它该在的那一层,复用现有的辅助函数。 指出功能逻辑漏进了共享路径,或者实现细节透过 API 漏了出来。优先用现有的标准工具,不要另写一次性的定制版本。把代码推到对的包、服务或模块里,不要让偏离变成常态。
-
更干净的结构显而易见时,把不必要的串行编排和非原子的更新当作设计上的坏味道。 互相独立的工作无缘无故地串行执行,问一句是不是该并行。相关的几处更新可能只完成一半、留下半截状态时,推动更原子的结构。不要过分纠结微优化,但可以避免、又让代码更脆弱的编排复杂度,要指出来。
输出时的优先顺序
Section titled “输出时的优先顺序”先讲结构上的代码质量退步和错过的简化,再讲意大利面式代码和分支复杂度,然后是边界、类型和文件大小的问题,最后是较小的模块划分和可读性问题。
不要因为行为看起来正确就批准。下面这些情况默认挡住合并,除非作者能说出充分理由:一招「代码柔道」就能删掉大量附带复杂度,PR 却留着它。把一个文件从不到 1000 行撑到超过 1000 行。加了临时分支,把现有流程缠在一起。把功能判断散落在共享代码里。加了不必要的抽象、包装或靠大量类型转换维持的约定;或者已有明确的标准归宿,却重复了现有的辅助函数,或把逻辑放错了层。只要还没达到批准的门槛,就留下明确、能照着改的反馈,推动更干净的拆分。
直接、严肃,对质量要求高。不要无礼,但也不要把重大的可维护性问题说软成温和的建议。代码让代码库更乱了,就直说。实现错过了一个明显能大幅简化的机会,也直说。真正的问题出在结构上时,不要满足于「这里要不改个名」。
