代码审查翻车现场：一个裸辞半年老码农的血泪复盘
去年十月，我从某一线大厂“战略性撤退”，开启了一段长达半年的Gap生活。白天撸猫、晚上打游戏，中间偶尔刷刷LeetCode，顺便把《重构》和《代码整洁之道》这两本书翻得快散架了——不是装文艺，是真的焦虑。远程办公久了，人容易陷入“自嗨式编码”：没人review，没人吐槽，连提交信息都敢写“fix bug（again）”。直到上个月重新投简历、面试、入职一家中型科技公司，我才意识到：脱离团队协作的代码，再优雅也是孤芳自赏。
而真正让我“破防”的，是入职第三周参与的一次代码审查（Code Review）。本以为凭借前东家那套“高内聚低耦合+类型安全+100%测试覆盖”的黄金标准能轻松过关，结果被同事一条评论问懵：“你这函数参数为啥要传五个？还全是any？”
我当时盯着屏幕，耳机里放着Lo-fi beats，手里的咖啡差点洒在机械键盘上——原来我已经太久没被人“挑刺”了。
今天这篇技术分享，就复盘几个我在近期项目里踩过的代码审查大坑，有些是老毛病复发，有些是新环境水土不服。希望能帮到正在卷CR（Code Review）的你。
你以为的“清晰”，其实是“谜语”
我们项目用的是TypeScript + React + NestJS 全栈。上周五晚上，我接到一个紧急需求：给后台管理系统加个“批量导出用户行为日志”的功能，下周一上线——又是熟悉的Deadline压迫感。
我吭哧吭哧写了200行代码，封装了一个 exportUserActivityLogs 函数，参数列表如下：
function exportUserActivityLogs(
userIds: string[],
startDate: string,
endDate: string,
includeIP: boolean,
format: 'csv' | 'json'
) { /* ... */ }
自我感觉良好：类型明确、职责单一、命名规范。提交PR后，信心满满等LGTM（Looks Good To Me）。
结果第二天早上，Team Lead 直接在PR里@我：“兄弟，这函数调用起来像在解谜。五个参数，顺序不能错，boolean参数意义模糊，format还得看注释才知道有啥选项。”
我愣了一下，心想：这不就是标准做法吗？但转念一想——在大厂时我们有严格的文档和IDE提示，但小团队里大家更依赖直觉和可读性。
后来我改成了对象参数：
interface ExportOptions {
userIds: string[];
dateRange: { start: string; end: string };
includeClientIP?: boolean; // 默认false
outputFormat?: 'csv' | 'json'; // 默认'csv'
}
function exportUserActivityLogs(options: ExportOptions) { /* ... */ }
不仅调用更清晰：
exportUserActivityLogs({
userIds: ['u1', 'u2'],
dateRange: { start: '2024-01-01', end: '2024-01-31' },
includeClientIP: true
});
还顺手加了默认值，避免调用方传一堆undefined。这次CR终于过了。
教训：可读性不是“我能看懂”，而是“别人不用动脑就能用对”。
别让“过度抽象”变成“过度表演”
另一个翻车点，是我试图秀一把“设计模式”。
业务里有个场景：不同渠道（APP、Web、小程序）上报的日志格式略有差异，需要做字段映射。我一拍大腿：这不就是策略模式的经典用例吗？
于是搞了个 LogParserFactory + 多个 ParserStrategy，每个策略类50行，加上接口定义、工厂注册……总共写了180行。
PR一交，后端同事直接评论：“就三个渠道，if-else不香吗？你这代码我光找入口就得花五分钟。”
我试图辩解：“以后扩展方便啊！”
他回：“产品经理说今年不会再加新渠道了。”
那一刻，我仿佛看到了《重构》第5章那句话：“过早优化是万恶之源”——马丁·福勒诚不我欺。
最后我删掉了所有策略类，改成一个简单的switch：
function parseLog(raw: any, channel: 'app' | 'web' | 'mini') {
switch (channel) {
case 'app':
return { userId: raw.uid, action: raw.event, ts: raw.timestamp };
case 'web':
return { userId: raw.user_id, action: raw.action_type, ts: raw.time };
// ...
}
}
代码从180行缩到30行，逻辑一目了然。
有时候，“简单”才是最高级的抽象。
测试覆盖率≠质量，别自欺欺人
说到测试，我以前在大厂可是“TDD信徒”，单元测试覆盖率必须90%+。这次我也写了测试，覆盖率92%，心里美滋滋。
但CR时测试同学指出一个问题：我测了函数正常路径，却没测异常情况。比如日期格式错误、用户ID为空等边界条件。
// 我原来的测试
it('should return parsed logs', () => {
const result = parseLog({ uid: '123', event: 'click' }, 'app');
expect(result.userId).toBe('123');
});
但他问：“如果传了个非法日期呢？会不会把服务搞崩？”
我一查，果然！我的解析函数在遇到无效日期时会抛出未捕获异常，导致整个导出任务失败——线上事故预定。
后来补上了异常测试：
it('should throw when date is invalid', () => {
expect(() => parseLog({ ... }, 'invalid-date')).toThrow();
});
并加了try-catch和日志上报。
测试不是为了凑覆盖率数字，而是为了兜住那些“万一”。
工具链差异：别把前公司的习惯强加给新团队
最后一个坑，纯属“文化冲突”。
我在前公司用的是基于Gerrit的CR流程，每行代码都要逐字review，comments必须resolve才能merge。所以这次我也按同样标准写PR description：详细说明改动点、关联需求、影响范围，甚至画了数据流图。
结果同事一脸懵：“你这PR写得跟RFC似的……我们一般就写‘fix export bug’就行。”
后来才知道，他们用的是GitHub PR + Slack通知，讲究快速迭代，PR描述越短越好，细节口头沟通。我那一长串文档反而让他们觉得“太重了”。
于是我调整策略：PR标题写清楚意图（如“feat: support batch export user logs”），正文只留关键变更点，其他细节在站会或Slack里同步。效率反而更高。
每个团队都有自己的节奏，别用过去的尺子量现在的地。
写在最后：CR不是审判，是共同成长
这次重新找工作，最大的感悟是：代码从来不是一个人的事。即使在家远程办公，戴着耳机听着音乐敲代码，最终还是要交付给团队、交付给用户。
那些曾经觉得“吹毛求疵”的CR意见，现在回头看，都是在帮我避开更大的坑。正如《代码整洁之道》里说的：“写出机器能读懂的代码很容易，写出人能读懂的代码才难。”
如果你也在经历CR被“虐”的阶段，别灰心。每一次被指出的问题，都是你离“靠谱工程师”更近一步的证明。
对了，我现在每天开工前还是会放首歌，但不再只为自己写代码了。
毕竟，好代码，是写给人看的，顺便让机器执行。
附：代码审查避坑自查清单（个人经验版）
| 检查项 | 反面案例 | 正确姿势 |
|---|---|---|
| 参数设计 | 5个以上位置参数，含boolean开关 | 使用配置对象，带默认值 |
| 抽象程度 | 为未来可能性提前设计复杂架构 | 先满足当前需求，YAGNI原则 |
| 异常处理 | 只测happy path | 覆盖边界、错误、超时等异常流 |
| PR描述 | 过于简略 or 过度文档化 | 清晰标题 + 关键变更点 + 影响范围 |
| 团队适配 | 强行套用前公司流程 | 观察并融入现有协作方式 |
共勉。
评论 0