code-review.md

Code Review

---

Code Review 的好处都有啥

Code Review 的好处都有啥，谁说对了就给他

• 提高代码质量。
• 互相学习，增进友♂情。
• 帮助大家了解系统，万一跑路了，代码就没人能维护了。
• 避免一些东西被错误的使用，例如左神写了一个jquery的插件，结果被我用瞎了。

Code Review Check List

列表来自 Software-Quality-Assurance-Documentation-and-Reviews

我有特别的code review 技巧
--不愿透露姓名的梁艺峰同学

• 完整性检查（Completeness）

  • 你这个Card没有做完就拖到Code Review，被坑了啊！（主要还是靠feature review）
  • 代码上有没有实现所有功能
  • Test 是不是充足
  • 需要review的人更多的了解card的功能，现有的card在没有review check list 的时候，需要团队更多的沟通。（不懂还是要多问问）

• 一致性检查（Consistency）

  • 文案格式 包括一些用词和标点符号，wiki上有详细的说明
  • 代码风格 每个人的风格都比较独特，就像酒，越久越浓。但是缩进大家还是统一一下吧，一瓶酒酿成了醋就不好玩了。
  • 和现有系统一致性。比如遵守 styleguide。现有的一些不一致的东西，pundit的使用。
  • 一致性应该是大家都遵守的，不应该成为code review的重点
  • 最起码先过了 Rubocop，

• 正确性（Correctness)

  Ruby才是最好的语言

  • render partial的时候locals是不是完整
  • partial 里面的 partial path
  • 是不是在new的form里调用了一下奇奇怪怪的东西，比如funding.agency之类

  • Ruby (On Rails)的一些坑

    if 0,if [] ,if ''
Model.find_or_create_by(enum_attribute: :some_enum_value)
Exception VS StandardError
a = 1 if defined?(a)
a ||= b fails when a is false and b is true

• 可修改性，维护性（Modifiability）

  30天精通设计模式 -- Copy and Paste

  • 因为中央经常做决定，所以代码能很快修改也是很关键的
  • 程序设计是不是合理 ，不要写死，尽量避免代码的duplicate
  • KPI Service， Tracklog这一块的Query有很多逻辑是一样的，之前是重复的出现在 SQL Query里，每次修改都要做大量的检查，之后把一些常用的Query逻辑放在了一个helper method里，改动就少很多。

• 可预测性（Predictability）

  我们写代码，靠的不是运气

  • 没有考虑到的死循环，例如之前redirect many times，特别是在做登录验证的时候
  • ActiveRecord的callback要小心使用，也会导致死循环的发生。

• 健壮性（Robustness）

  • Postgres中注意处理divided by zero
    COALESCE(COUNT(blabla)), NULLIF(blabla, 0)
  • Ruby中 一些 nil值的处理
    nil.try(:blabla)
  • 一些 exception有没有正确的handle，如果没必要through出来，也要做log在sentry上记录

• 可理解性（Understandability）

  • 由于产品需求变化快，很多地方都是前人挖坑后人跳，为了不跳坑，大家开始尽量不去refactor之前的代码，这就导致很多地方变得逻辑复杂，越难越难理解，比如funding里的access逻辑：

           AgencyPermission.enable_apollo_agency_ids.include?(@funding.agency_id) &&
    (%w(execution closing).include?(@funding.status) ||
      (investor.vip? || (Tracklog.wechat_pushed_funding_ids(investor).include?(@funding.id)) && @funding.status == 'private_marketing')||
    (@funding.status == 'pursue' && Tracklog.wechat_pushed_funding_ids(investor).include(@funding.id))) &&
    !investor.funding_block_me_ids.include?(@funding.id) && @funding.access?(investor)
    

    这一段其实已经很难看懂了，之后有了新的规则也很容易改动错误。所以 Code Review的时候遇到自己已经看不懂的，互相督促改进一下。

• 测试 （Verifiability/Testability）

  • 代码是不是方便测试，比如现在的WechatService，有些逻辑放在低层，不容易Mock，Mock了外层的话，里面的逻辑就很容易忽略

• 性能 （Performance）

  • 避免 N+1 Query，一些很明显的 N+1 Query可以很好的避免，但是系统里还是有蛮多的N+1 Query的
  • 如果这段 SQL让你来写，你会怎么写来更好的提高 performance。

• 安全 （Security）

  • 安全也是功能的一部分，然而很容易被忽略
  • 最基本的一些access control要有
  • https://www.owasp.org/images/0/08/OWASP_SCP_Quick_Reference_Guide_v2.pdf
  • https://github.com/FallibleInc/security-guide-for-developers

一些可能会用到的特殊技巧

• Review前可以大概了解一下这个card在干嘛

• 最好被review的人能准备一个 review的checklist,当然code review 不能仅限与checklist

• 一些比较重要的地方，比如funding的access control，逻辑要double confirm

• 太大的card最好能分milestone来review，太大的card如果一下100个changes要review，效率低，很难看懂

需要注意的地方

• 应该有充足的test保证不出bug， 不能依赖 review的人

• 代码规范， 大家尽量统一

• 避免的极端

  • 大家review的时候不要太极端爆粗口质疑别人的能力

    我不是针对谁，我是说，在座的各位都是 **
    -- 断水流大师兄

  • 自己的代码被review不要慌，写的不好可以互相请教学习，共同进步

    我丑，但我就是要出来吓人