Skip to content

Instantly share code, notes, and snippets.

@bruce-shi

bruce-shi/code-review.md

Last active September 21, 2016 02:33
Show Gist options
  • Save bruce-shi/9fc5581f8b1ccdeb720da5560d28a1db to your computer and use it in GitHub Desktop.
Save bruce-shi/9fc5581f8b1ccdeb720da5560d28a1db to your computer and use it in GitHub Desktop.
Download ZIP
Raw
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）

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

  • 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不要慌，写的不好可以互相请教学习，共同进步

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment