★ FEATURED ARTICLE 代码审查这件事说白了就是我们研发团队最后一道质量闸门。很多人觉得代码审查就是“找毛病”、“挑刺”其实真正做得好的人都知道这是整个团队对代码质量、架构演进方向的一次集体把关。而我今天要分享的是一套我从规范到架构、从本地到CI、从个人到团队逐步打磨出来的全方位检查清单这套清单帮我的团队把线上故障率压下去至少四成也让新同学融入团队的速度快了不少。如果你正在带团队、正在被评审或者要去评审别人的Python代码这篇文章应该能帮你省下不少功夫。Python项目做代码审查天然比其他语言要“虚”一些。它没有强类型系统帮你兜底没有编译器在合代码前拦住低级错误加上开发效率太高业务迭代太快技术债一不留神就越积越厚。正因为如此一份结构清晰的检查清单远比“凭经验和感觉走一遍”来得可靠。我自己的做法是把审查拆成“规范层、设计层、实现层、架构层、工程化层”五个维度逐项过这样既不漏、也不乱每次评审基本能控制在30到60分钟内效率比早年漫无目的地看高太多了。1. 为什么需要一套结构化的检查清单1.1 Python项目的特殊性动态类型带来的“隐性风险”我见过太多Java或者Go背景的同学转来做Python时第一反应是“这也太自由了”紧接着第二反应就是“这也能上线”。Python的动态特性确实给开发提了速但代价是非运行期几乎无法暴露大部分类型错误。热词列表里出现频率极高的“python入门”、“python安装教程”说明行业里涌入的新人非常多新同学写出来的代码往往语法没什么毛病但一跑到边界条件就炸。举个最典型的例子我在审查时经常看到这样的代码def get_user_info(user_id, db): user db.fetch_one(SELECT * FROM users WHERE id%s, user_id) return {name: user[name], email: user[email]}这里的隐患是如果db.fetch_one返回None下一行直接对None做下标操作就会抛TypeError。这种问题在静态类型语言里大概率会被编译器拦住在Python里只有跑到那条分支才炸。所以我在代码审查时第一项要查的往往不是规范而是数据流里的空值传播。这类问题在检查清单里必须单列成项不能只靠肉眼扫。1.2 检查清单为什么比“凭感觉”更高效很多人评审代码是靠“读”从头读到尾然后凭直觉给几条意见。这样不是不行但非常累而且容易漏。一次几百行甚至上千行的MR纯靠线性阅读到后半段人已经疲惫了注意力急剧下降真正要紧的问题反而看不出来。我自己的经验是把检查清单当成“评审的路由表”每到一个层次就只关心该层次的事情规范层的交给工具、架构层的单独抽时间看不要在细节和宏观之间来回横跳。打个比方代码审查就像房子验收。你不可能既趴在地上看地砖缝又同时抬头看楼板是不是平的。你得先看结构图再进房间挨个查水电最后才看油漆和瓷砖。检查清单本质上是给评审者规划了一条“先整体、后局部、再细节”的动线保证该看的都看到而且每看一步都有判断依据。1.3 我自己清单的演进过程早年间我做代码审查基本靠“师父带徒弟”的那套组长安排我看谁的代码我就打开diff闷头读。结果有一次线上出了一个大事故一个同学在写命令行工具时用了eval()处理用户输入我当时看到了但觉得“这行代码能跑”就没有指出来。后来这个工具被挂到内部的Web管理台上一个走歪的请求直接让整个服务崩了。那次事故之后我决定把脑子里那些零零碎碎的判断点全部落到纸上一条一条列一条一条查并且每半年复盘一次把新踩的坑补进去。所以今天这份清单每条背后要么是我自己踩过的坑要么是我在社区里看别人复盘后验证过的教训。2. 规范层检查最容易被忽视也最容易带偏团队2.1 PEP8与命名规范可读性是第一生产力规范层是审查的门面也是新同学最容易出问题的地方。PEP8本身不难难的是让整个团队真的执行一致。我在审查时重点看四个东西命名风格、导入顺序、行长度、空行使用。命名方面最常见的反例是“短得不知道在干嘛”的变量名def cal(a, b, c): t a * b r t / c return r这段代码换成有意义的命名之后是这样的def calculate_average_price(total_amount, item_count, discount_rate): discounted_total total_amount * discount_rate return discounted_total / item_count可能有人会觉得“这也能算问题”实际上在真实项目里变量名是代码的自文档一个函数如果让阅读者反复往定义处跳审查就该打回重写。PEP8里关于类名用CamelCase、函数和变量用snake_case、常量用UPPER_CASE的约定团队里必须作为硬性规则。没有讨论余地统一是唯一选项。导入顺序我一般交给工具管理isort或者ruff的I规则都能自动搞定手动在审查里纠结导入顺序没意义。但我会扫一眼有没有导入了但没使用的符号这种往往说明开发过程中删了代码没删干净属于卫生问题。2.2 类型提示与注释的边界注释写“为什么”而不是“是什么”类型提示在Python 3.5之后已经成为标配但在代码审查里我发现不少团队的代码库还是没有类型标注。更奇怪的是有些团队明明定了“必须加类型”但大家只是机械地在函数签名里加函数体内部的数据结构该模糊还是模糊。我审查时对注释的要求很明确注释回答“为什么”而不是“是什么”。下面这种注释基本等于没写# 遍历用户列表 for user in users: pass真正有用的注释是这个样子# 这里必须用set而不是list因为用户量上万之后list的in操作会拖垮整个接口 whitelisted_ids {item.id for item in items}类型提示的审查重点是函数的入参和返回值的类型是否真的描述了运行时行为特别是那些可能为None的参数。如果一个函数的参数可以是None类型标注里必须写清楚Optional[str]而不是直接写str。我见过太多项目因为这里偷懒导致mypy一开立刻几百个错误。2.3 Git提交规范commit信息也是代码审查的一部分热词里出现了“git提交规范”这确实是个高频关注点。代码审查如果只看代码本身不看提交历史等于只看照片不看体检报告。一个合格的提交信息至少要说明“这次改了什么、为什么改、影响范围是什么”。我比较推荐Conventional Commits那套约定虽然有点老生常谈但它对后续自动生成changelog和定位问题非常有帮助feat(api): 新增用户注册接口 fix(order): 修复订单金额溢出问题 refactor(utils): 重构日期格式化工具函数审查PR时我会看提交粒度。如果一个PR包含了好几个互不相干的功能或者类似“fix typo”和“update requirement”混在一起我会建议拆开。因为一旦上线后出了问题你需要在git log里快速定位哪次提交引入的如果提交信息一团浆糊回滚和排查都会变成灾难。3. 设计层检查代码可读性与可维护性的分水岭3.1 函数设计的五个信号什么时候该拆分设计层是代码审查最见功力的一层。我一般先看函数的形状从那几个信号来判断是不是需要动刀第一个信号是函数太长。我自己验收时有个相对宽泛的标准——一个函数超过50行大概率做了不止一件事。Python社区比较认“25行左右一个函数”的轻量风格虽然不绝对但它逼迫你把逻辑拆碎而拆碎之后很多隐藏问题就会浮出水面。第二个信号是参数太多。超过5个参数的函数调用方几乎一定记不住顺序和含义。我见过一个发送消息的函数参数从host到retry_times一共9个调用处看起来像天书。这种情况与其加参数不如定义一个SendOptions的dataclass把参数收敛起来。第三个信号是逻辑嵌套太深。if里面套for里面再套if这种代码读起来非常痛苦。我的建议是“早返回”把不满足条件的路径提前结束def process_order(order, inventory): if order is None: return {code: 400, msg: order is null} if not inventory.check(order.item_id): return {code: 400, msg: item out of stock} # 主体逻辑第四个信号是函数有副作用。如果一个函数既改了数据库又发了消息又写日志那它几乎不可能被复用也几乎不可能被测试。审查时看到这种“多功能合一体”的函数我的意见一般是拆成repository、notifier、logger三层。第五个信号是纯函数与非纯函数混在一起。纯函数好测、好推理非纯函数难测。能写成纯函数的部分就应该和IO部分分开这样单测只需要mock一个边界。3.2 DRY原则的正确打开方式不是所有重复都要消除重复代码在审查时经常被一刀切地否掉但我在后来实践中发现“DRY”也需要看场景。最简单的一种情况是两段代码长得像但其实它们的语义根本不同——比如一个是“按用户维度去重”一个是“按品维度去重”这时候强行抽公共函数会让代码变得抽象难懂参数越来越多最后谁也看不懂。我比较认同“三次法则”同样代码出现三次以上再考虑抽象一次两次的重复也许直接复制更清晰。审查的时候我会关注的是“重复的是逻辑还是巧合”。如果两段代码未来变化的节奏相同抽出来是正确的如果变化方向不同抽出来反而是提前的抽象。另外一个常见反例是“为了DRY而DRY”把两个完全不同的业务逻辑硬捏到一个函数里用一堆布尔参数控制分流。这种代码看起来消灭了重复实际上制造了更严重的耦合。我审查时会特别警惕这种“伪抽象”一旦发现就用一个简单的原则去反驳如果一个函数的调用方需要用注释来解释某个布尔参数的含义那这个函数本身就该拆。3.3 类和继承组合优于继承不是口号Python的MRO和多重继承确实强大但越强大的东西越容易玩脱。审查时我最怕看到的是三层以上的继承关系比如BaseHandler - BaseHttpHandler - BaseJsonHandler - UserHandler这种到第四层的时候你根本不知道self.request到底从哪个父类里来。我自己的原则是继承只用于“是一个”的关系如果两个类之间是“有一个”的关系就用组合。举个例子一个EmailService类不需要去继承SmtpClient它应该持有一个SmtpClient实例class EmailService: def __init__(self, smtp_client: SmtpClient): self._smtp_client smtp_client这样做的好处显而易见测试的时候可以轻松注入一个MockSmtpClient不需要动任何父类逻辑。审查时如果看到“为了复用几个方法就去继承一个类”我的建议几乎都是改成组合或者抽成模块级函数。类设计还有一个检查点一个类不能太大、职责不能太多。我常用的一个粗暴标准如果你给这个类写类注释时需要写超过两行才能说清楚“它是干什么的”那这个类大概率该拆了。拆分的判断也不复杂把类里的方法按“动词”分组每组对应一个职责那就是天然的拆分边界。4. 实现层检查数据流、并发与安全4.1 输入校验与异常处理Python里最容易翻车的两个点实现层的问题最难用工具查因为它需要结合业务理解。我审查时对输入校验有一个底线要求来自外部的一切数据都不可信。这里的“外部”包括HTTP请求参数、消息队列里的消息、配置文件里的值、甚至数据库里读取出来的字段。一个经典的坏味道是直接对用户的输入做下标引用比如user_input request.json[name]如果请求体里根本没有name这个键这一行就会抛KeyError。更稳妥的做法是user_input request.json.get(name) or 别小看这个差异线上很多500错误都是这么来的。审查时我会对每个“从外部拿数据”的接触点逐一排查看有没有做默认值、有没有做类型转换、有没有做范围校验。异常处理是另一个重灾区。我见过两种极端一种是全部裸奔任何异常都不管线上直接抛堆栈另一种是到处try...except Exception把什么错都吞了日志里什么都没有。这两种都不可取。我比较推崇的做法是能提前判断的边界条件用if挡不要用异常来控制流程真正的异常要分类处理——可预期的业务异常自己定义异常类不可预期的系统异常要记录完整堆栈。而且绝对不要在except里写pass哪怕留一行日志都比静默吞掉强百倍。4.2 资源管理与并发with语句、连接池与线程安全Python的资源管理核心就是with语句。文件、锁、数据库连接凡是实现了__enter__和__exit__的对象都应该用with来保证资源释放。审查时如果看到裸的open而不带close或者某个Lock.acquire()后面没有finally里的release()基本直接打回。并发这块要看的点更多。Python因为有GIL很多人误以为“线程安全不是问题”但GIL只保证单个字节码指令的原子性不保证多行代码组成的关键区安全。比如if self.count 0: self.count - 1这里的两行之间完全可能被线程切换打断导致self.count变成负数。审查并发代码时我一般会问三个问题共享状态是什么、锁覆盖的范围够不够、有没有使用queue.Queue或concurrent.futures这类更高级的抽象。热词里出现了“python协程”这也是近年的审查重点。协程代码的审查和线程不一样重点看有没有在异步函数里调用阻塞操作比如requests.get而不是httpx.AsyncClient因为一个阻塞调用就会卡掉整个事件循环。我自己在审查时遇到async def且有await requests.get这种组合会直接标红。4.3 性能与安全红线N1查询、日志脱敏与正则陷阱性能问题在代码审查阶段就能发现很多。最常见的莫过于数据库的N1查询循环里逐条访问数据库。比如for order in orders: user db.fetch_one(SELECT * FROM users WHERE id%s, order.user_id)如果orders有1000条这段代码就会发起1000次查询接口性能可想而知。审查意见很简单改成一次IN查询拿回所有用户再在内存里做映射。安全方面我重点看三块敏感信息日志、路径拼接、以及eval/exec的使用。日志脱敏这条看着小出事就是大事。很多团队在调试时把整个请求体打到日志里里面可能就带了用户手机号或者token。我审查时对日志里出现password、token、secret这类字段零容忍。路径拼接则是看有没有用os.path.join做规范化防止../穿越之类的问题。至于eval/exec我的意见是除非搞插件系统否则一律不允许出现在业务代码里。正则表达式本身不是坏事但用户可控的正则要小心ReDoS。像(a)这种嵌套量词的正则遇到恶意输入会把CPU吃满。审查时如果看到用户输入直接拼进re.compile()我会要求加一层长度和复杂度限制或者干脆改用fnmatch这类更安全的匹配方式。5. 架构层检查从模块职责到演进方向5.1 项目结构与依赖方向分层清晰才能走得远架构层是代码审查里“最贵”的一层因为它需要结合项目的全局视图来判断。我审查时先看整个仓库的目录结构在脑子里画出包之间的依赖关系图。Python项目最常见的架构问题是utils或common变成一个垃圾回收站什么都在里面放然后所有模块都依赖它最终导致任何改动都要大面积回归。我比较推崇的分层方式是api层处理输入输出service层处理业务逻辑repository层处理数据访问core层放领域模型。依赖方向必须是单向的——api依赖serviceservice依赖repository谁也不能反向依赖。审查时如果看到service里直接写了SQL或者core的领域模型里import了django的ORM模型我会明确指出来这破坏了分层的边界以后一定会成为扩展的阻碍。其实检查依赖方向有个很实用的技巧直接执行import-linter这类工具把架构约束写成规则CI里自动检查。我自己在团队里就配了一个简单的依赖规则文件任何人如果违反分层约定CI直接红。这比人工审查可靠得多因为人的注意力在该关注逻辑时不该浪费在边界检查上。5.2 接口契约与版本兼容内部API也要讲“兼容性”热词列表里有“微服务架构”、“分布式架构”这种场景下接口契约的审查比单体应用重要得多。很多人做内部服务接口时觉得“反正是我自己调改了就改了”结果一个字段改名导致十几个调用方一起挂。我的审查原则是任何跨服务接口、跨模块的公共函数都要考虑向后兼容。怎么审查兼容性几个关键点新增参数必须带默认值、返回值只能新增字段不能删除字段、枚举值只增不删、废弃的接口要加deprecated标记并保留一段过渡期。如果需要破坏性变更一定要先搜一下所有调用方确认没有在使用之后再改。热词里还有“基于RCP的汽车ZCU架构”、“ARM CMN架构深度解析”这种偏底层的概念但代码审查层面它们背后的逻辑是相通的——无论底层是芯片还是业务服务变化都是有边界的契约一旦定下来变更就要走流程。尤其是现在的分布式系统服务之间的接口就是整个架构的骨架骨架歪了肌肉再结实也没用。5.3 微服务和分布式场景下的审查重点超时、重试、幂等、链路追踪如果你的团队在搞微服务那么代码审查的关注点必须扩展出纯代码之外。我审查分布式相关的PR时逐个检查四个关键点第一有没有设置超时时间。服务间调用如果不设超时上游服务一旦hang住整个调用链就会被拖死。第二重试策略是否合理。重试不能无脑做像“下单”这种非幂等操作重试会导致重复扣款。所以重试前必须确认下游接口是否幂等。第三幂等实现是否到位。每次都生成一个新的request_id的接口除非依赖方实现了去重否则重试就是灾难。第四有没有透传链路追踪ID。没有trace_id的服务调用排查线上问题基本靠猜代码审查时如果发现出入参模型里没有trace字段我会要求加。这些检查项在单体应用时代几乎不存在但在微服务架构下它们决定了一次发布的成败。我在审查清单里专门把“分布式检查”独立成一个H2小节每个涉及跨服务调用的PR都会过一遍这张四连表。5.4 新范式项目的审查特点AI Agent与LLM API的代码审查热词里“AI agent主流架构”、“LLMAPI架构”、“moe架构”频繁出现说明现在很多团队已经在把大模型能力接入业务系统。但这类项目的代码审查传统清单根本不适用。我审查过几个Agent项目总结出三个额外的检查维度。第一工具函数的边界。Agent通常通过调用工具函数操作外部系统每个工具函数就是一道安全阀门。审查时要确认工具函数做了完整的参数校验、权限校验以及结果截断否则大模型一通乱调可能把生产库删了。第二提示词可控性。提示词本身应该被视为代码的一部分不能散落在业务代码里随便拼。我建议把提示词模板独立成版本化文件审查时看的是模板的变更记录。第三回退机制。大模型永远是概率性的它在某些输入下必然出错。代码里必须设计回退逻辑比如调用失败时返回缓存的兜底结果或者转交给人工处理流程。没有兜底的Agent代码本质上是定时炸弹。6. 自动化工具链与团队协作流程6.1 本地与CI的工具组合少一点拼刺刀多一点拼配置人工审查很宝贵但它的精力应该花在“设计”和“架构”这种机器看不懂的层面。规范层和低级错误应该全部交给工具在CI阶段拦下来。我目前团队里用的工具组合是工具作用配置建议rufflint与格式检查启用E、F、I、N等规则集替换flake8black自动格式化单行长度设88或100团队统一isort导入排序profile设为black避免和black冲突mypy类型检查先从不严格的disallow_untyped_defs开始逐步收紧pytest单元测试覆盖率先用--cov-fail-under80兜底import-linter依赖方向约束把架构分层规则写进配置ruff是我特别推荐的一个工具它用Rust写的比flake8快一个量级配置项也更丰富。实测下来在几万行的项目里跑一遍只需要几秒完全可以作为pre-commit钩子每次提交前自动跑。black的争议比较大但我个人的观点是格式化工具就是为了消灭争论的好不好看不重要重要的是所有人都一样。6.2 Pre-commit钩子与CI集成把审查前置到“提交前”代码审查最有价值的时间点是最早的“变更刚产生时”而不是推到CI之后。我们团队在根目录放了一个.pre-commit-config.yaml大概长这样repos: - repo: https://github.com/astral-sh/ruff-pre-commit rev: v0.4.0 hooks: - id: ruff args: [--fix, --exit-non-zero-on-fix] - repo: https://github.com/psf/black rev: 23.12.1 hooks: - id: black language_version: python3.11 - repo: https://github.com/pre-commit/mirrors-mypy rev: v1.8.0 hooks: - id: mypy这个配置意味着开发者本地提交代码时ruff和black会自动跑一遍并改写代码格式mypy会检查类型错误。如果本地不过压根儿提交不上去。这样一来到人工审查阶段代码已经经过了第一轮机器清洗审查者只需要关注逻辑和架构效率高非常多。CI阶段我们还会跑pytest和import-linter确保整个测试集是绿的依赖方向没有违反分层规则。我把这套配置放在团队的模板仓库里新项目直接复制几乎零成本落地。6.3 高效审查流程分层评审、评论分级与响应时限流程规范对代码审查体验的提升不亚于工具。我团队里的做法是第一PR要小。一个MR尽量控制在400行以内超过就要说明理由。几千行的MR没有人能认真看完最后一定是走过场。第二评论分级。我用三个前缀区分评论的紧急程度[nit]表示可改可不改的小问题、[suggestion]表示建议但非必须、[block]表示必须修改后才能合并。这样作者能快速判断哪些评论要处理、哪些可以留到后续迭代。第三响应时限。审查者的响应尽量在24小时内完成拖得越久上下文切换的成本越高作者也容易产生挫败感。我自己还有一个习惯评审前先跑一下代码把关键流程在本地执行一遍再回来看diff。很多逻辑问题在“看代码”时不容易发现但一跑起来就原形毕露。尤其是涉及时间、分页、并发之类的逻辑跑一下见效很快。7. 常见问题与排查技巧实录7.1 容易在审查中漏掉的“高危点”我在多年的审查中反复踩过一些坑挑几个最典型的说出来给大家提个醒。第一个是“改了一行但影响了整个调用链”。很多Python代码错误来自“签名没变、但语义变了”。比如有人把某个函数的返回值从list改成了generator调用方里如果用了len()立刻炸。这种问题在代码审查里非常难发现因为diff只显示被修改的行调用方要看一两层才能反应过来。我的建议是凡是改公共函数语义的PR必须有配套的类型标注更新和调用方搜索记录。第二个是“测试代码本身就是错的”。很多人审查业务代码时非常认真但看到测试文件就觉得“有测试就过了”。其实测试代码也是代码assert没有起作用、mock太粗暴、测试只是为了过覆盖率而写的空壳在项目里比比皆是。我在审查时如果发现一个测试从来都没有运行到真正的业务分支会直接打回。第三个是“配置文件和代码脱节”。比如一个模块的默认配置在config.py里改成了新值但线上部署的.env文件里还是老值或者某个middleware开关在测试环境开了、生产环境没开。这类问题不会出现在diff里但可能让整个功能在线上完全不生效。审查时留意配置相关文件的变更最好顺便看一眼部署脚本。7.2 评审时如何给出“不伤人”又能落地的反馈代码审查是一门沟通的艺术。早年我做审查时非常直接动不动就写“这个变量名太烂了”、“这逻辑完全不对”结果团队气氛降到冰点几个同学甚至一到提MR就紧张。后来我调整了策略效果立刻不一样。具体做法其实很简单把“你写错了”换成“这里可能有一个边界问题你看是不是这样处理更稳妥”。把“这个类设计得不行”换成“如果未来要扩展某个场景这个结构可能会不好加要不要考虑拆成两个”。本质上代码审查的目的是把代码改好不是把人打败。评论里多提供可执行的具体建议少做价值判断作者会更容易接受。我还建议团队内部做一个“审查评论模板”包括背景说明、问题位置、为什么这是问题、建议修改方案四个要素。这套模板强制审查者想清楚再评论也帮助作者快速理解背景比随手的“这里不好用”强太多。7.3 汇总一份可以直接抄的“审查速查表”最后把全文的检查点浓缩成一张速查表我团队的同学每次审查前都会打开它过一遍层面核心检查点页判标尺规范层命名、类型标注、导入、提交信息团队约定是否一致能否自动检查设计层函数长度、参数个数、分支深度、重复逻辑、继承关系是否被单一职责原则约束实现层边界输入、资源释放、并发安全、异常处理是否可预测、可追踪、不吞错架构层依赖方向、接口兼容、跨服务调用策略是否与系统方向一致不埋雷工程化层CI是否通过、测试是否有效、覆盖率是否达标机器已查的人工不重复查这个表格看起来简单但每一个格子背后都是大量线上事故换来的经验。代码审查不是表演不是为了显示审查者多厉害而是为了确保一段代码上线之后产品和用户不会在凌晨被一个低级错误打断。守住这道关比什么都实在。我个人这几年的体会是代码审查的本质不是“考试”而是团队的“集体复盘”。每一次评审既是在检查别人的代码也是在自己脑子里重演一遍问题。你真正认真去审查别人的代码自己写代码时就会不自觉地避开那些别人踩过的坑。所以别把它当任务把它当成投资——投下去的时间最终会以更少的故障、更顺的迭代和更踏实的发版收回来。祝各位在审查和被审查的路上都能少踩坑、多长进。
阅读完成 · 觉得有帮助？
