代码审查查什么?
不应该做的
- 格式:在什么地方放置空格和断行符?使用制表符还是空格?大括号如何放置?
- 风格:变量、参数是否声明为final?函数变量的定义是否与调用代码或函数开始代码太相似?
- 命名:域、常量、变量、参数、类的名称是否符合标准?命名是否太短?
- 覆盖测试:这段代码是否进行了测试?
应该做的
基本原则
设计
- 新代码是否与整体架构匹配?
- 代码是否遵循SOLID原则、领域驱动设计以及团队喜爱的其它设计模式。
- 新代码采用哪些设计模式?这些模式合适吗?
- 如果代码库采用混合标准或设计风格,新代码是否符合当前风格?代码迁移是按正确的方向进行,还是效仿即将被淘汰的陈旧代码?
- 代码是否处于正确的位置?例如,如果代码执行与顺序有关,它是否能按顺序执行?
- 新代码能否复用部分已存在代码?新代码能否给已存在代码提供复用部分?
- 新代码是否包含冗余代码?如果包含,是应该重构成更加可复用部分,还是在这个阶段能接受这种冗余?
- 代码是否超出设计标准?这种复用构造现在是不是不需要?团队如何根据YAGNI权衡复用?
可读性和可维护性
- 命名(字段、变量、参数、函数以及类)能否反映它们代表的事物?
- 通过阅读代码,我能否理解代码的目的?
- 我能否理解测试的目的?
- 测试是否覆盖了绝大部分情况?是否覆盖常见情况和异常情况?是否存在没考虑到的情况?
- 异常错误消息是否易于理解?
- 难以理解的代码段是否进行了备注、评论或者使用易懂的测试案例进行覆盖(符合团队的偏好)?
功能
- 代码的实际工作是否符合预期?如果有自动化测试来确保代码的正确,测试能否测出代码满足约定要求?
- 代码看上去是否含有细微错误,比如使用错误变量进行检查,或者把 or 误用为 and?
其他
- 代码中是否存在潜在的安全问题?
- 是否需要满足规范要求?
- 对于没有覆盖自动化性能测试的代码段,新代码是否引入了不可避免的性能问题,比如不必要的数据调用或远程服务?
- 作者是否需要创建公共文档,或者修改现有的帮助文档。
- 展示给用户的消息是否检查无误?
- 是否存在导致产品崩溃的明显错误?代码是否会意外指向测试数据库,或者是否有应该替换成真正服务的硬编码存根代码?
测试
- 新的或修改的代码有测试吗?
- 测试有覆盖到代码中令人困惑或者复杂的部分吗?
- 我能理解这些测试么?
- 这些测试需要满足什么要求?
- 我可以考虑没有被现有测试覆盖到的用例吗?
- 这些测试是否有说明代码的限制条件?
- 审查代码中的测试类型、测试级别正确吗?
- 有没有针对安全性的测试?
- 审查者也可以写测试
性能
- 这部分功能有硬性的性能需求吗?
- 如果有,有测试去检查吗?
- 功能修改或新功能对已有性能测试结果有不良影响吗?
- 代码审查中没有硬性的性能需求要怎么做?
- 数据库调用是否影响性能?
- 是否有不必要的网络调用?
- 移动程序、可穿戴程序是否过于频繁地调用后端?
- 代码通过锁来访问共享代码吗?这样会导致性能下降和死锁吗?
- 代码会造成内存泄漏吗?
- 应用程序的内存会无限增加吗?
- 代码有关闭连接或流吗?
- 资源池配置正确吗?
- 如果你检查包含反射的代码,问一下反射是不是一定需要?
- 如果超时,系统中的其他部分会受到什么影响?
- 代码中有使用多个线程来执行一个简单的操作吗?除了增加时间和复杂性外,却没有带来性能的提高?代码使用并行流机制,但是却没有从并行性中受益吗?
- 多线程环境下的代码使用了正确的数据结构吗?
- 代码容易出现竞态条件吗?
- 代码中对锁的使用正确吗?
- 代码的性能测试有价值吗?
- 如果审查的代码使用了缓存,不正确的缓存项失效是否进行处理?
代码级优化
- 代码中是否使用了不必要的同步、锁?如果代码总是执行在单线程下,加锁只会带来额外开销。
- 代码中是否使用了不必要的线程安全数据结构?例如,可以用ArrayList 替换 Vector 吗?
- 代码中是否使用了在常见操作上性能很差的数据结构?例如,使用了链表结构,却经常搜索其中的一项。
- 代码是否使用了锁或者同步机制,而实际上可以用原子变量替代?
- 代码可以得益于延迟加载吗?
- if 语句或其他逻辑语句能通过短路机制进行优化吗?比如把最快的计算放在条件的开始?
- 有很多的字符串格式吗?可以更有效率吗?
- log 语句有使用字符串格式化吗?有用检查log级别的 if 语句包起来或者使用的日志提供者可以进行延迟操作?
数据结构
- 数据结构是否被正确的使用?
- 是否使用了反模式?
SOLID原则
SOLID代表:
- S – 单一职责原则:不应该有多种情况需要修改某个类的对象
- O – 开放封闭原则:软件实体应该对扩展开放,对修改封闭
- L – 里氏替换原则:函数使用基类的引用,必须能够在不知不觉的情况下使用派生类的对象
- I – 接口分离原则:多个客户端特定的接口比使用一个通用的接口要好
- D – 依赖倒置原则:依赖抽象。不要依赖于具体实现
一些代码异味可能意味着可能已经违反了一个或多个SOLID原则:
- 很长的if/else语句
- 强制转换成一个子类型
- 很多公共方法
- 实现的方法抛出UnsupportedOperationException异常
安全
- 了解第三方库
- 检查是否需要对新路径和服务进行身份验证?
- 您的数据是否需要加密?
- 密码、加密密钥、token等秘密数据是否被正确管理?
- 代码应该记录/审计行为吗?这样做是否正确?
- 代码是否会进行任何数据更改(例如添加/更新/删除)?它是否应该记录所做的更改,由谁以及何时进行?
- 此代码是否在某些性能关键路径上?是否应该在某种性能监控系统中记录启动时间和结束时间?
- 任何记录的消息的日志记录级别是否合适?