把几十个 Python 项目的 merge request 从头到尾过一遍之后我对代码审查的态度经历了一个反转它不只是一道“检查风格是否符合规范”的关卡更是一个人把自己的判断力注入项目的好机会。这篇文章不打算罗列教科书式的规范条目而是从实际 review 里沉淀下来的一套 Python 代码审查实战检查清单——从最基本的编码规范一路看到系统架构每个检查点都讲清楚为什么要看、怎么看、以及我踩过什么样的坑。适合刚接手 MR 评审的同学也适合想在提交前自己做一遍自检的开发者。1. 代码审查到底在看什么先分清四层目标1.1 规范、正确性、设计与演进我最早做 review 的时候满脑子都是“有没有违反 PEP 8”“变量名是不是规范”结果把时间都花在了挑格式上真正有问题的逻辑反而被放过去了。后来我把 review 的目标拆成四层每一层的审查方式和语气都不一样。规范层代码能不能被人类顺畅地读懂命名是否诚实函数是否短到可以一眼看完。正确性层边界条件、异常路径、资源释放、并发状态在真实运行中会不会出问题。设计层模块划分是否合理依赖方向是否清晰新代码是强化了既有结构还是绕过了它。演进层这次改动三个月后会不会变成债接口是否向后兼容别人是否敢在这个基础上继续改。这四层的关系可以用下面这张表来理解层次关注点典型问题规范层可读性、命名、结构函数太长、命名误导、逻辑藏在注释里正确性边界、异常、资源、竞态空值崩溃、资源泄漏、重试不幂等设计层模块划分、依赖方向循环 import、控制器里塞业务、工具模块反向依赖演进层兼容性、扩展成本改签名、动返回结构、临时补丁没人认领每次 review 之前先问自己这次提交属于哪一层大多数提交同时涉及多个层次但你的注意力分配不一样。一个脚本实验只要规范层过得去就行核心支付链路上的改动则必须把正确性和演进层都审到。换句话说代码审查不是走流程而是先用“这层代码会活多久”来判断投入的精力。1.2 Diff 阅读和全量阅读的取舍很多人打开 MR 只看 diff逐行看新增代码就结束了。我承认 diff 是高效手段但只靠 diff 会漏掉一类重要信息改动和周边代码的交互方式。我的做法是改动量在 200 行以内把涉及的文件全量读一遍超过 300 行先要求提交者把 MR 拆小拆不开的至少给出“核心改动”和“配套改动”的说明。全量阅读能发现那种“新增函数看似通用其实和某个旧逻辑重复”的问题这类问题在 diff 里几乎看不出来。另一个经验是如果提交者连 commit message 都写不清“为什么改”那大概率连他自己都没想清楚。review 从 commit message 开始能省掉后面大半的沟通成本。1.3 读代码的顺序先接口再实现最后测试我推荐一套固定的阅读顺序先看公开函数的签名和类型注解再读实现里的异常分支最后看测试有没有覆盖关键路径。先看签名能快速判断设计意图比如get_user(user_id: int) - User | None比get_user(user_id)能传递的信息多得多看实现时重点盯“如果输入非法会怎样”而不是“正常输入能不能跑通”最后看测试不是为了看覆盖率数字而是确认提交者是否真的考虑过异常场景。这个顺序能阻止你陷入局部细节。很多时候review 到一半突然发现接口设计本身就是错的那后面几十行格式问题已经不值得再看了。2. 规范层PEP 8 之外真正影响可读性的细节2.1 命名是低成本的架构决策PEP 8 规定了命名风格但没规定命名质量。我在 review 里看到最多的不是“命名不符合规范”而是“命名根本没表达含义”。# 反例任何人都猜不到 process_data 具体做什么 def process_data(data): ... # 推荐函数名直接回答“做什么” def normalize_phone_number(raw: str) - str: ...data、item、temp、result这类变量名不是不能用而是出现频率太高时读代码的人就得反复跳到定义处才能确认含义。每跳一次认知负荷就高一截。review 时看到新的变量名我会问自己一个新同事不看上下文能不能在 3 秒内说出这个变量存的是什么如果不行这个命名就需要改。2.2 类型注解从文档注释到可执行约束Python 是动态语言但团队项目不应该把“动态”当作不做类型标注的理由。类型注解最大的价值不是好看而是让错误的可能性变窄同时给调用方一份不需要翻源码的契约。# 不推荐 def get_user(user_id): return db.query(fSELECT * FROM users WHERE id {user_id}) # 推荐 def get_user(user_id: int) - User | None: return db.query_one(User, iduser_id)review 时看到# type: ignore我不会直接放过去而是按“这里藏着一个解释不清的问题”来处理。它可能是第三方库的 isue也可能是类型设计本身有问题。对于 Python 3.10 以下的团队int | None要写成Optional[int]这点在 review 时要根据项目版本保持一致。注解也不是越多越好。内部函数和私有方法可以少注解公开接口必须完整尤其是返回类型。返回类型缺失调用方拿到什么都只能靠猜这是规范层里最隐蔽的成本。2.3 函数长度与认知负荷“一个函数最好不超过 50 行”这类建议太绝对我更喜欢用“一眼定律”打开函数视线扫一遍能不能看出它总共在干几件事。如果一个函数做到了三件事解析输入、调外部服务、写缓存那么即使只有 30 行也应该拆。相反一个线性执行的纯计算函数长一点也没太大问题因为它的逻辑是连续的。真正危险的是多层缩进、循环套条件、条件里再调函数这种结构让每一行的状态都依赖前面的状态读起来像是在做思维迷宫。review 时看到超过 50 行且多层嵌套的函数第一反应不是“给我拆成更短”而是“它同时在干几件事”。让提交者把职责拆开往往比单纯要求缩短更有用。2.4 自动化工具能做的别让人眼做格式、未使用的变量、import 顺序、明显的语法问题这些全部应该交给工具。我的人工注意力只留给工具判断不了的问题语义、边界、依赖、演进。一个可复制的组合是用自动格式化工具统一代码风格用静态检查工具扫常见问题用类型检查工具验证注解最后用统一配置的 pre-commit 在提交前拦截。团队里最想避免的是每个开发者本地配置不同导致 MR 里全是格式噪音真正的逻辑问题反而被淹没。检查项交给工具必须人眼缩进、行宽、import 排序是否未使用变量、未定义名称是否类型不匹配是否命名是否误导否是注释是否与代码同步过时否是函数职责是否单一否是3. 正确性与异常路径最容易放走的隐蔽 Bug3.1 空值、边界和“看起来不可能”的输入Review 里最常漏掉的 bug往往出现在“正常路径没有问题极端输入直接崩溃”的地方。列表分片、字典取值、链式调用都是重灾区。# 看起来没问题但 items 为空时会发生什么 batch_size len(items) // 3 for chunk in [items[i:i batch_size] for i in range(0, len(items), batch_size)]: process(chunk)当items为空时batch_size是 0range(0, 0, 0)会直接抛ValueError。这个例子说明review 时看到列表运算至少要在心里做一遍“空列表、单元素列表、n 不能被整除”的推演。我自己在 review 订单回调逻辑时遇到过类似问题生产环境里数据量偶尔为 0测试数据从没覆盖上线后第一分钟就报警。字典的链式取值也是同样的逻辑result data.get(key, {}).get(value, 0)如果data[key]存在但值是None这一行照样抛AttributeError。review 时看到链式取值养成习惯问一句中间节点可能为 None 吗3.2 异常粒度什么时候 catch什么时候让它抛出去except Exception是 Python 项目里最让人头疼的写法之一。它看起来安全实际上把所有异常都吞掉了包括代码里真正的 bug。# 反例失败时完全不知道发生了什么 try: response requests.post(url, jsonpayload, timeout5) except Exception: pass # 推荐区分异常类型并保留原始上下文 try: response requests.post(url, jsonpayload, timeout5) except requests.Timeout as exc: raise ExternalServiceTimeout(payment service timeout) from exc except requests.RequestException as exc: raise ExternalServiceError(payment service unavailable) from excraise ... from exc不是形式主义它能把底层异常链完整保留排查问题时少走很多弯路。review 时看到裸except: pass基本可以直接打回。如果确实只需要兜底日志至少要用logger.exception(...)记录完整堆栈而不是让异常静默消失。3.3 资源与上下文with 之外的隐藏状态Python 程序员都知道with open(...)可以安全处理文件但资源管理的坑远不止文件。数据库连接、线程锁、临时目录、信号量、缓存客户端都需要明确的生命周期。# 不推荐acquire 和 release 隔着 20 行中间任何 return 都会漏释放 lock.acquire() try: ... finally: lock.release() # 推荐用上下文管理器收敛生命周期 with lock: ...review 时看到acquire()和release()相距很远必须警惕。这类代码一旦中间出现提前 return 或者新异常锁就永远放不掉了。更好的方式是把资源生命周期收进with语句或显式的上下文管理器里让“进入和退出”成对出现。另外一个容易被忽略的资源是缓存和连接池。比如每请求创建新的数据库连接而不是复用连接池这在低并发时没事流量上来就直接打满连接数。3.4 按“发生中断后怎么办”来读正确性 review 的最高标准不是正常流程是否能跑通而是每一步“发生中断后系统是否还能自洽”。举个例子处理支付回调时是先改订单状态还是先记账如果改完状态、记账之前进程崩了重试会发生什么订单会不会被重复记账如果调用外部接口超时重试逻辑会不会把同样的请求发两次这类问题在 happy path 里看不见但生产环境里几乎天天发生。review 时看到写数据库、发消息、调外部服务的代码我会要求提交者说明“重试一次会发生什么”。如果对方答不上来说明这个改动还没想清楚。幂等键、去重表、状态机限制都是这类问题的常见解法但更重要的是先把问题问出来。4. 性能与并发Review 时如何定位潜在瓶颈4.1 复杂度预判先看循环嵌套和数据规模性能 review 不是靠猜而是靠量级判断。看到嵌套循环时我会先确认数据规模外层 10 条、内层 1000 条和内外都是 10 万条处理方式完全不同。# O(n*m)每个元素都在列表里做一次线性查找 for user_id in user_ids: if user_id in all_user_ids: ... # O(nm)先用集合建立索引 known_ids set(all_user_ids) for user_id in user_ids: if user_id in known_ids: ...把列表in查找换成集合或字典是 Python 项目里性价比最高的性能改进。但 review 时也要克制如果数据规模只有几百这种优化对用户毫无感知反而让代码多几行。性能问题的优先级应该由“实际量级”决定而不是由“听起来很慢”决定。4.2 数据库查询和 IO 重灾区会延续Python Web 服务里最常见的性能问题是 N1 查询外层循环查询主记录循环里再一次次查关联数据。review 时看到循环里执行数据库查询直接标记。# N1 查询 for order in orders: items.append(get_items_for_order(order.id)) # 批量查询 items get_items_for_orders([o.id for o in orders])同样的道理适用于循环里发 HTTP 请求、循环里写文件、循环里调第三方 SDK。每一次 IO 都有网络延迟和连接开销批量合并往往能带来数量级的提升。review 时还要看事务边界长事务里做外部调用会让数据库连接长时间占用连接池一旦耗尽整个服务都会受影响。4.3 多线程与异步可重入、竞态与阻塞多线程代码在 review 里最难发现的是竞态问题。比如一个看似简单的计数器counter 0 def worker(): global counter for _ in range(100): counter 1counter 1不是原子操作线程交替执行时会出现“读-算-写”中间被切换的情况。review 时看到共享状态被多线程读写要么加锁要么用queue或concurrent.futures的线程安全结构。异步代码也有自己的坑。在async函数里调用同步阻塞 IO会直接卡住整个事件循环导致其他请求一起变慢。review 时看到await函数里混着time.sleep或同步requests应该提示改成异步版本或者放到线程池里执行。4.4 没有 profile 不下定论性能 review 最容易犯的错误是凭直觉要求“优化”。有一次我 review 一个数据校验功能建议把某处的list换成set理由是从复杂度上看更快。后来提交者实际测试数据量只有几百条改动前后耗时差异在误差范围内反而因为引入集合的哈希约束代码可读性变差了。从那以后我给自己定了一条纪律review 时可以指出明显的复杂度问题但涉及微优化必须给出可验证的收益。通常先问“这里的数据量级是多少”“有没有压测数据”再决定要不要动。5. 安全与依赖代码审查中的风险扫描5.1 注入与路径操作把不可信输入当敌人Python 里最容易出现的注入是 SQL 注入和命令注入。review 时看到字符串拼接 SQL无论提交者怎么解释都先假定是有问题的。# 反例用户输入直接拼进 SQL query fSELECT * FROM users WHERE name {name} # 正例参数化查询 query SELECT * FROM users WHERE name ?文件路径也一样。用户传入的文件名如果包含../直接拼进os.path.join(base_dir, filename)就能越界读到其他目录。review 时看到用用户输入构造路径至少要用Path.resolve()校验最终路径是否仍然位于允许的目录内。命令注入则藏在subprocess.run(fls {path}, shellTrue)这类写法里。能不用shellTrue就不用改用参数列表传参是更安全的方式。5.2 敏感信息密钥、Token 与日志泄露很多密钥事故不是发生在配置中心而是发生在日志里。review 时看到logger.info(request failed: %s, request_body)要考虑request_body里是否包含密码、Token、手机号一类信息。一个很常见的场景是开发环境调试时为了方便把整个请求体打到日志里上线的时候忘了删。我的检查习惯是全文搜索几个关键词password、token、secret、api_key看它们有没有出现在日志输出、错误消息、URL 参数和数据库字段里。如果确实需要记录请求体至少要做脱敏处理比如把密码字段替换成***。5.3 依赖锁定与第三方风险requirements.txt里写package1.0等于把版本命运交给未来。review 时看到依赖没有锁定版本或者锁定版本但不锁定传递依赖我会要求改成固定版本加锁文件的方式保证任何人在任何时候拉代码装出来的环境是一致的。另一个常被忽略的问题是直接从一个 Git 仓库pip install git...安装依赖。这不一定是错的但必须固定到具体 commit 或者 tag否则第三方仓库的每次推送都会改变你的构建结果。定期扫描已安装依赖的已知漏洞也应该进入团队的例行任务。5.4 服务端校验与权限前端校验只是用户体验服务端校验才是安全边界。review 时看到新接口没有校验必填字段、长度、格式直接打回。尤其要注意接口是否真的做了权限判断而不是只依赖网关过滤。同事曾经在网关层做了 auth 校验业务层就默认所有请求都合法直到有人发现一个内部接口可以被普通用户直接调用改了好几天才补完。6. 架构层面这次改动是否在给系统挖坑6.1 依赖方向与分层让数据流单向流动代码审查要看的最后一个大层次是这次改动对整体结构的影响。我最常遇到的架构问题是循环 import 和分层混乱。一个典型的信号是视图函数里写业务逻辑、业务函数里拼 SQL、而工具模块反过来 import 业务模块。数据流一旦开始绕圈任何改动都有可能踩到隐藏的耦合。review 时如果发现自己画不出“这次改动从哪里来、要往哪里去”的依赖关系就应该打回去要求重构。推荐的做法是让依赖方向保持单向控制层调服务层服务层调数据访问层工具模块只提供无业务含义的通用能力不反向依赖业务。分层不是教条而是让未来改任何一层的成本都可控。6.2 接口设计与向后兼容改函数签名、改字典 key、改返回类型意味着所有调用方都要跟着变。review 时看到这类改动第一件事是在仓库里搜索所有调用点确认影响范围。# 原来返回 dict调用方用 config[timeout] 获取值 def get_config() - dict: ... # 改成返回 dataclass调用方原来的写法全部失效 def get_config() - AppConfig: ...如果这是内部接口可以保留旧函数并加 deprecated 警告先让调用方迁移再在下一个版本移除。如果是对外 HTTP API优先考虑新增版本而不是原地修改。一次“顺手”的接口变更可能让下游系统凌晨炸掉review 的价值就在这里——把看不见的风险提前暴露。6.3 可测试性Review 测试和 Review 代码一样重要代码审查不能只看业务代码测试代码同样在审查范围内。一个改动如果没有对应测试等于在提交一个承诺而这个承诺没人验证。我 review 测试时关注三点有没有覆盖核心状态转换、异常分支有没有断言、测试是在验证行为还是在验证实现细节。把大量 mock 堆起来的测试很容易变成“测试只验证了 mock 本身”。依赖注入是提升可测试性最直接的手段与其在测试里 patch 全局单例不如让业务函数接受外部依赖作为参数提交者也更容易写出自然覆盖边界的用例。6.4 演进成本临时方案必须被记录和认领技术债不是不能有但必须被记录和认领。review 时看到# TODO、临时补丁、异常分支爆炸我会问三个问题这个临时方案会活多久谁负责在到期时把它换掉如果不换未来维护的人需要知道哪些隐性知识才能改对如果三个问题一个都答不上来那这次改动就不是在解决问题而是在把问题延后。代码审查的最终目的不是让当前 MR 看起来完美而是让整个系统在三个月、一年后依然有人敢改、改得动。7. 落地经验把检查清单跑起来7.1 一张可以直接复制的最小检查清单我把自己实际使用的检查项压缩成一张表可以直接放进团队 MR 模板也可以作为自己提交前的自检列表层次必查项不通过时的常见信号规范命名能自解释函数短到可以一眼看完data、tmp、func2泛滥正确性空值、边界、异常链、资源释放都有处理except: pass、acquire和release隔着几十行性能循环内没有 N1 查询复杂度与数据量匹配循环里发 HTTP、嵌套in查找安全用户输入不拼 SQL/路径/命令密钥不进日志f-string 拼 SQL、日志打印整个 request body架构依赖单向、接口向后兼容、行为可测试循环 import、改函数签名没搜调用点测试核心行为有覆盖异常分支有断言只测 happy path、mock 掉所有外部依赖依赖锁文件存在依赖版本更新有记录requirements 无版本、从 Git 直接拉取未固定 commit这张表不是一次 review 就要全部走完的而是按改动类型选择。普通功能提交重点看规范、正确性和测试核心服务改动才需要把性能、安全和架构全部过一遍。7.2 和 CI、Git 工作流结合人工 review 最怕的是把时间花在机器能判断的问题上。静态检查、类型检查、测试、依赖漏洞扫描都放进 CI让提交者先在本地和流水线里解决掉“机器问题”人工 review 才有空间讨论“人类问题”。在实际落地时我会建议团队做几件小事用统一的 pre-commit 配置锁住提交格式CI 里不允许跳过测试大 MR 自动打标签提醒 reviewer 注意阅读成本MR 模板里强制填写“改动背景”和“测试说明”。这些措施的共性是把标准写进流程而不是寄希望于每个人记得住。7.3 Reviewer 怎么和提交者沟通最后说沟通因为 review 的技术再熟练沟通失败了也白搭。我的几个原则是把“建议”和“必须改”明确分开避免所有意见混在一起让提交者失去重点用提问代替断言看到奇怪写法先问“这里为什么这样写”而不是直接说“你写错了”能给出具体修改示例的尽量给出示例减少来回成本。我还有一个长期坚持的小习惯每次 review 至少写一条 positive comment不是客套而是让提交者知道什么样的写法应该继续保持。代码审查如果只剩下挑错大家会越来越怕提交代码而一个好的 review 文化应该让提交者觉得这是在帮他把代码变得更好。我现在每次打开一个 MR先按这个顺序快速过一遍签名、异常、资源、调用点、测试。剩下逐行细看的都是机器替我筛完之后值得人类判断的东西。希望这份清单也能帮你少踩几个坑。 Python代码审查实战清单：从规范到架构的核心检查点
把几十个 Python 项目的 merge request 从头到尾过一遍之后我对代码审查的态度经历了一个反转它不只是一道“检查风格是否符合规范”的关卡更是一个人把自己的判断力注入项目的好机会。这篇文章不打算罗列教科书式的规范条目而是从实际 review 里沉淀下来的一套 Python 代码审查实战检查清单——从最基本的编码规范一路看到系统架构每个检查点都讲清楚为什么要看、怎么看、以及我踩过什么样的坑。适合刚接手 MR 评审的同学也适合想在提交前自己做一遍自检的开发者。1. 代码审查到底在看什么先分清四层目标1.1 规范、正确性、设计与演进我最早做 review 的时候满脑子都是“有没有违反 PEP 8”“变量名是不是规范”结果把时间都花在了挑格式上真正有问题的逻辑反而被放过去了。后来我把 review 的目标拆成四层每一层的审查方式和语气都不一样。规范层代码能不能被人类顺畅地读懂命名是否诚实函数是否短到可以一眼看完。正确性层边界条件、异常路径、资源释放、并发状态在真实运行中会不会出问题。设计层模块划分是否合理依赖方向是否清晰新代码是强化了既有结构还是绕过了它。演进层这次改动三个月后会不会变成债接口是否向后兼容别人是否敢在这个基础上继续改。这四层的关系可以用下面这张表来理解层次关注点典型问题规范层可读性、命名、结构函数太长、命名误导、逻辑藏在注释里正确性边界、异常、资源、竞态空值崩溃、资源泄漏、重试不幂等设计层模块划分、依赖方向循环 import、控制器里塞业务、工具模块反向依赖演进层兼容性、扩展成本改签名、动返回结构、临时补丁没人认领每次 review 之前先问自己这次提交属于哪一层大多数提交同时涉及多个层次但你的注意力分配不一样。一个脚本实验只要规范层过得去就行核心支付链路上的改动则必须把正确性和演进层都审到。换句话说代码审查不是走流程而是先用“这层代码会活多久”来判断投入的精力。1.2 Diff 阅读和全量阅读的取舍很多人打开 MR 只看 diff逐行看新增代码就结束了。我承认 diff 是高效手段但只靠 diff 会漏掉一类重要信息改动和周边代码的交互方式。我的做法是改动量在 200 行以内把涉及的文件全量读一遍超过 300 行先要求提交者把 MR 拆小拆不开的至少给出“核心改动”和“配套改动”的说明。全量阅读能发现那种“新增函数看似通用其实和某个旧逻辑重复”的问题这类问题在 diff 里几乎看不出来。另一个经验是如果提交者连 commit message 都写不清“为什么改”那大概率连他自己都没想清楚。review 从 commit message 开始能省掉后面大半的沟通成本。1.3 读代码的顺序先接口再实现最后测试我推荐一套固定的阅读顺序先看公开函数的签名和类型注解再读实现里的异常分支最后看测试有没有覆盖关键路径。先看签名能快速判断设计意图比如get_user(user_id: int) - User | None比get_user(user_id)能传递的信息多得多看实现时重点盯“如果输入非法会怎样”而不是“正常输入能不能跑通”最后看测试不是为了看覆盖率数字而是确认提交者是否真的考虑过异常场景。这个顺序能阻止你陷入局部细节。很多时候review 到一半突然发现接口设计本身就是错的那后面几十行格式问题已经不值得再看了。2. 规范层PEP 8 之外真正影响可读性的细节2.1 命名是低成本的架构决策PEP 8 规定了命名风格但没规定命名质量。我在 review 里看到最多的不是“命名不符合规范”而是“命名根本没表达含义”。# 反例任何人都猜不到 process_data 具体做什么 def process_data(data): ... # 推荐函数名直接回答“做什么” def normalize_phone_number(raw: str) - str: ...data、item、temp、result这类变量名不是不能用而是出现频率太高时读代码的人就得反复跳到定义处才能确认含义。每跳一次认知负荷就高一截。review 时看到新的变量名我会问自己一个新同事不看上下文能不能在 3 秒内说出这个变量存的是什么如果不行这个命名就需要改。2.2 类型注解从文档注释到可执行约束Python 是动态语言但团队项目不应该把“动态”当作不做类型标注的理由。类型注解最大的价值不是好看而是让错误的可能性变窄同时给调用方一份不需要翻源码的契约。# 不推荐 def get_user(user_id): return db.query(fSELECT * FROM users WHERE id {user_id}) # 推荐 def get_user(user_id: int) - User | None: return db.query_one(User, iduser_id)review 时看到# type: ignore我不会直接放过去而是按“这里藏着一个解释不清的问题”来处理。它可能是第三方库的 isue也可能是类型设计本身有问题。对于 Python 3.10 以下的团队int | None要写成Optional[int]这点在 review 时要根据项目版本保持一致。注解也不是越多越好。内部函数和私有方法可以少注解公开接口必须完整尤其是返回类型。返回类型缺失调用方拿到什么都只能靠猜这是规范层里最隐蔽的成本。2.3 函数长度与认知负荷“一个函数最好不超过 50 行”这类建议太绝对我更喜欢用“一眼定律”打开函数视线扫一遍能不能看出它总共在干几件事。如果一个函数做到了三件事解析输入、调外部服务、写缓存那么即使只有 30 行也应该拆。相反一个线性执行的纯计算函数长一点也没太大问题因为它的逻辑是连续的。真正危险的是多层缩进、循环套条件、条件里再调函数这种结构让每一行的状态都依赖前面的状态读起来像是在做思维迷宫。review 时看到超过 50 行且多层嵌套的函数第一反应不是“给我拆成更短”而是“它同时在干几件事”。让提交者把职责拆开往往比单纯要求缩短更有用。2.4 自动化工具能做的别让人眼做格式、未使用的变量、import 顺序、明显的语法问题这些全部应该交给工具。我的人工注意力只留给工具判断不了的问题语义、边界、依赖、演进。一个可复制的组合是用自动格式化工具统一代码风格用静态检查工具扫常见问题用类型检查工具验证注解最后用统一配置的 pre-commit 在提交前拦截。团队里最想避免的是每个开发者本地配置不同导致 MR 里全是格式噪音真正的逻辑问题反而被淹没。检查项交给工具必须人眼缩进、行宽、import 排序是否未使用变量、未定义名称是否类型不匹配是否命名是否误导否是注释是否与代码同步过时否是函数职责是否单一否是3. 正确性与异常路径最容易放走的隐蔽 Bug3.1 空值、边界和“看起来不可能”的输入Review 里最常漏掉的 bug往往出现在“正常路径没有问题极端输入直接崩溃”的地方。列表分片、字典取值、链式调用都是重灾区。# 看起来没问题但 items 为空时会发生什么 batch_size len(items) // 3 for chunk in [items[i:i batch_size] for i in range(0, len(items), batch_size)]: process(chunk)当items为空时batch_size是 0range(0, 0, 0)会直接抛ValueError。这个例子说明review 时看到列表运算至少要在心里做一遍“空列表、单元素列表、n 不能被整除”的推演。我自己在 review 订单回调逻辑时遇到过类似问题生产环境里数据量偶尔为 0测试数据从没覆盖上线后第一分钟就报警。字典的链式取值也是同样的逻辑result data.get(key, {}).get(value, 0)如果data[key]存在但值是None这一行照样抛AttributeError。review 时看到链式取值养成习惯问一句中间节点可能为 None 吗3.2 异常粒度什么时候 catch什么时候让它抛出去except Exception是 Python 项目里最让人头疼的写法之一。它看起来安全实际上把所有异常都吞掉了包括代码里真正的 bug。# 反例失败时完全不知道发生了什么 try: response requests.post(url, jsonpayload, timeout5) except Exception: pass # 推荐区分异常类型并保留原始上下文 try: response requests.post(url, jsonpayload, timeout5) except requests.Timeout as exc: raise ExternalServiceTimeout(payment service timeout) from exc except requests.RequestException as exc: raise ExternalServiceError(payment service unavailable) from excraise ... from exc不是形式主义它能把底层异常链完整保留排查问题时少走很多弯路。review 时看到裸except: pass基本可以直接打回。如果确实只需要兜底日志至少要用logger.exception(...)记录完整堆栈而不是让异常静默消失。3.3 资源与上下文with 之外的隐藏状态Python 程序员都知道with open(...)可以安全处理文件但资源管理的坑远不止文件。数据库连接、线程锁、临时目录、信号量、缓存客户端都需要明确的生命周期。# 不推荐acquire 和 release 隔着 20 行中间任何 return 都会漏释放 lock.acquire() try: ... finally: lock.release() # 推荐用上下文管理器收敛生命周期 with lock: ...review 时看到acquire()和release()相距很远必须警惕。这类代码一旦中间出现提前 return 或者新异常锁就永远放不掉了。更好的方式是把资源生命周期收进with语句或显式的上下文管理器里让“进入和退出”成对出现。另外一个容易被忽略的资源是缓存和连接池。比如每请求创建新的数据库连接而不是复用连接池这在低并发时没事流量上来就直接打满连接数。3.4 按“发生中断后怎么办”来读正确性 review 的最高标准不是正常流程是否能跑通而是每一步“发生中断后系统是否还能自洽”。举个例子处理支付回调时是先改订单状态还是先记账如果改完状态、记账之前进程崩了重试会发生什么订单会不会被重复记账如果调用外部接口超时重试逻辑会不会把同样的请求发两次这类问题在 happy path 里看不见但生产环境里几乎天天发生。review 时看到写数据库、发消息、调外部服务的代码我会要求提交者说明“重试一次会发生什么”。如果对方答不上来说明这个改动还没想清楚。幂等键、去重表、状态机限制都是这类问题的常见解法但更重要的是先把问题问出来。4. 性能与并发Review 时如何定位潜在瓶颈4.1 复杂度预判先看循环嵌套和数据规模性能 review 不是靠猜而是靠量级判断。看到嵌套循环时我会先确认数据规模外层 10 条、内层 1000 条和内外都是 10 万条处理方式完全不同。# O(n*m)每个元素都在列表里做一次线性查找 for user_id in user_ids: if user_id in all_user_ids: ... # O(nm)先用集合建立索引 known_ids set(all_user_ids) for user_id in user_ids: if user_id in known_ids: ...把列表in查找换成集合或字典是 Python 项目里性价比最高的性能改进。但 review 时也要克制如果数据规模只有几百这种优化对用户毫无感知反而让代码多几行。性能问题的优先级应该由“实际量级”决定而不是由“听起来很慢”决定。4.2 数据库查询和 IO 重灾区会延续Python Web 服务里最常见的性能问题是 N1 查询外层循环查询主记录循环里再一次次查关联数据。review 时看到循环里执行数据库查询直接标记。# N1 查询 for order in orders: items.append(get_items_for_order(order.id)) # 批量查询 items get_items_for_orders([o.id for o in orders])同样的道理适用于循环里发 HTTP 请求、循环里写文件、循环里调第三方 SDK。每一次 IO 都有网络延迟和连接开销批量合并往往能带来数量级的提升。review 时还要看事务边界长事务里做外部调用会让数据库连接长时间占用连接池一旦耗尽整个服务都会受影响。4.3 多线程与异步可重入、竞态与阻塞多线程代码在 review 里最难发现的是竞态问题。比如一个看似简单的计数器counter 0 def worker(): global counter for _ in range(100): counter 1counter 1不是原子操作线程交替执行时会出现“读-算-写”中间被切换的情况。review 时看到共享状态被多线程读写要么加锁要么用queue或concurrent.futures的线程安全结构。异步代码也有自己的坑。在async函数里调用同步阻塞 IO会直接卡住整个事件循环导致其他请求一起变慢。review 时看到await函数里混着time.sleep或同步requests应该提示改成异步版本或者放到线程池里执行。4.4 没有 profile 不下定论性能 review 最容易犯的错误是凭直觉要求“优化”。有一次我 review 一个数据校验功能建议把某处的list换成set理由是从复杂度上看更快。后来提交者实际测试数据量只有几百条改动前后耗时差异在误差范围内反而因为引入集合的哈希约束代码可读性变差了。从那以后我给自己定了一条纪律review 时可以指出明显的复杂度问题但涉及微优化必须给出可验证的收益。通常先问“这里的数据量级是多少”“有没有压测数据”再决定要不要动。5. 安全与依赖代码审查中的风险扫描5.1 注入与路径操作把不可信输入当敌人Python 里最容易出现的注入是 SQL 注入和命令注入。review 时看到字符串拼接 SQL无论提交者怎么解释都先假定是有问题的。# 反例用户输入直接拼进 SQL query fSELECT * FROM users WHERE name {name} # 正例参数化查询 query SELECT * FROM users WHERE name ?文件路径也一样。用户传入的文件名如果包含../直接拼进os.path.join(base_dir, filename)就能越界读到其他目录。review 时看到用用户输入构造路径至少要用Path.resolve()校验最终路径是否仍然位于允许的目录内。命令注入则藏在subprocess.run(fls {path}, shellTrue)这类写法里。能不用shellTrue就不用改用参数列表传参是更安全的方式。5.2 敏感信息密钥、Token 与日志泄露很多密钥事故不是发生在配置中心而是发生在日志里。review 时看到logger.info(request failed: %s, request_body)要考虑request_body里是否包含密码、Token、手机号一类信息。一个很常见的场景是开发环境调试时为了方便把整个请求体打到日志里上线的时候忘了删。我的检查习惯是全文搜索几个关键词password、token、secret、api_key看它们有没有出现在日志输出、错误消息、URL 参数和数据库字段里。如果确实需要记录请求体至少要做脱敏处理比如把密码字段替换成***。5.3 依赖锁定与第三方风险requirements.txt里写package1.0等于把版本命运交给未来。review 时看到依赖没有锁定版本或者锁定版本但不锁定传递依赖我会要求改成固定版本加锁文件的方式保证任何人在任何时候拉代码装出来的环境是一致的。另一个常被忽略的问题是直接从一个 Git 仓库pip install git...安装依赖。这不一定是错的但必须固定到具体 commit 或者 tag否则第三方仓库的每次推送都会改变你的构建结果。定期扫描已安装依赖的已知漏洞也应该进入团队的例行任务。5.4 服务端校验与权限前端校验只是用户体验服务端校验才是安全边界。review 时看到新接口没有校验必填字段、长度、格式直接打回。尤其要注意接口是否真的做了权限判断而不是只依赖网关过滤。同事曾经在网关层做了 auth 校验业务层就默认所有请求都合法直到有人发现一个内部接口可以被普通用户直接调用改了好几天才补完。6. 架构层面这次改动是否在给系统挖坑6.1 依赖方向与分层让数据流单向流动代码审查要看的最后一个大层次是这次改动对整体结构的影响。我最常遇到的架构问题是循环 import 和分层混乱。一个典型的信号是视图函数里写业务逻辑、业务函数里拼 SQL、而工具模块反过来 import 业务模块。数据流一旦开始绕圈任何改动都有可能踩到隐藏的耦合。review 时如果发现自己画不出“这次改动从哪里来、要往哪里去”的依赖关系就应该打回去要求重构。推荐的做法是让依赖方向保持单向控制层调服务层服务层调数据访问层工具模块只提供无业务含义的通用能力不反向依赖业务。分层不是教条而是让未来改任何一层的成本都可控。6.2 接口设计与向后兼容改函数签名、改字典 key、改返回类型意味着所有调用方都要跟着变。review 时看到这类改动第一件事是在仓库里搜索所有调用点确认影响范围。# 原来返回 dict调用方用 config[timeout] 获取值 def get_config() - dict: ... # 改成返回 dataclass调用方原来的写法全部失效 def get_config() - AppConfig: ...如果这是内部接口可以保留旧函数并加 deprecated 警告先让调用方迁移再在下一个版本移除。如果是对外 HTTP API优先考虑新增版本而不是原地修改。一次“顺手”的接口变更可能让下游系统凌晨炸掉review 的价值就在这里——把看不见的风险提前暴露。6.3 可测试性Review 测试和 Review 代码一样重要代码审查不能只看业务代码测试代码同样在审查范围内。一个改动如果没有对应测试等于在提交一个承诺而这个承诺没人验证。我 review 测试时关注三点有没有覆盖核心状态转换、异常分支有没有断言、测试是在验证行为还是在验证实现细节。把大量 mock 堆起来的测试很容易变成“测试只验证了 mock 本身”。依赖注入是提升可测试性最直接的手段与其在测试里 patch 全局单例不如让业务函数接受外部依赖作为参数提交者也更容易写出自然覆盖边界的用例。6.4 演进成本临时方案必须被记录和认领技术债不是不能有但必须被记录和认领。review 时看到# TODO、临时补丁、异常分支爆炸我会问三个问题这个临时方案会活多久谁负责在到期时把它换掉如果不换未来维护的人需要知道哪些隐性知识才能改对如果三个问题一个都答不上来那这次改动就不是在解决问题而是在把问题延后。代码审查的最终目的不是让当前 MR 看起来完美而是让整个系统在三个月、一年后依然有人敢改、改得动。7. 落地经验把检查清单跑起来7.1 一张可以直接复制的最小检查清单我把自己实际使用的检查项压缩成一张表可以直接放进团队 MR 模板也可以作为自己提交前的自检列表层次必查项不通过时的常见信号规范命名能自解释函数短到可以一眼看完data、tmp、func2泛滥正确性空值、边界、异常链、资源释放都有处理except: pass、acquire和release隔着几十行性能循环内没有 N1 查询复杂度与数据量匹配循环里发 HTTP、嵌套in查找安全用户输入不拼 SQL/路径/命令密钥不进日志f-string 拼 SQL、日志打印整个 request body架构依赖单向、接口向后兼容、行为可测试循环 import、改函数签名没搜调用点测试核心行为有覆盖异常分支有断言只测 happy path、mock 掉所有外部依赖依赖锁文件存在依赖版本更新有记录requirements 无版本、从 Git 直接拉取未固定 commit这张表不是一次 review 就要全部走完的而是按改动类型选择。普通功能提交重点看规范、正确性和测试核心服务改动才需要把性能、安全和架构全部过一遍。7.2 和 CI、Git 工作流结合人工 review 最怕的是把时间花在机器能判断的问题上。静态检查、类型检查、测试、依赖漏洞扫描都放进 CI让提交者先在本地和流水线里解决掉“机器问题”人工 review 才有空间讨论“人类问题”。在实际落地时我会建议团队做几件小事用统一的 pre-commit 配置锁住提交格式CI 里不允许跳过测试大 MR 自动打标签提醒 reviewer 注意阅读成本MR 模板里强制填写“改动背景”和“测试说明”。这些措施的共性是把标准写进流程而不是寄希望于每个人记得住。7.3 Reviewer 怎么和提交者沟通最后说沟通因为 review 的技术再熟练沟通失败了也白搭。我的几个原则是把“建议”和“必须改”明确分开避免所有意见混在一起让提交者失去重点用提问代替断言看到奇怪写法先问“这里为什么这样写”而不是直接说“你写错了”能给出具体修改示例的尽量给出示例减少来回成本。我还有一个长期坚持的小习惯每次 review 至少写一条 positive comment不是客套而是让提交者知道什么样的写法应该继续保持。代码审查如果只剩下挑错大家会越来越怕提交代码而一个好的 review 文化应该让提交者觉得这是在帮他把代码变得更好。我现在每次打开一个 MR先按这个顺序快速过一遍签名、异常、资源、调用点、测试。剩下逐行细看的都是机器替我筛完之后值得人类判断的东西。希望这份清单也能帮你少踩几个坑。