重构 干掉你代码中的坏味道

ane · 2016年08月05日 · 最后由 Fighting_3 回复于 2018年02月07日 · 11275 次阅读
本帖已被设为精华帖!

最近团队开始抓代码质量了,总结一下自己的经验,先看看坏代码有哪些特点:

“幸福的家庭都一样,不幸的家庭却各有不同”,这句话放到代码里也同样适用。接下来,我们聊一聊如何解决坏代码问题。
如果我问你,“你们是如何保证团队代码质量的”,你的回答可能是:“我们每次写完代码,都会花一些时间review一下。”

恩,做的确实不错,但是,做的还不够,除非你是门门考试都100的学霸,否则,借助一些工具还是比较稳妥的办法。

在这里简单介绍一个代码分析工具RubyCritic,这是一个专门针对Ruby的一个静态代码分析工具。其它语言的,也有相似功能的工具链,我就不做介绍了。

这是一个命令行工具,第一步就是添加到你的gem库中,当然,还可以使用guard自动化分析。(Ruby的世界,你懂得~) 第二step,在console运行【RubyCritic】命令,就像这样:

在命令的最后,会生成一个静态页面。长这个样子:

x轴代表改动频率,Y轴代表代码复杂程度

这是分析结果的overview,超过200的复杂度的,基本都是坏代码。

再看看code里的内容:

对不同文件按照改动频率、复杂度、重复度和坏味道4个维度进行综合评定代码质量等级(和美国考试的成绩打分规则一样)。

RubyCritic对代码分析的原理,其实就是分析一些,被它认为是坏代码的点。注意,我这里使用的措辞是“被它认为,所以,有时候,它不是绝对的正确。” 还可以查看具体的类文件中的代码质量问题。

更多的介绍,详见 https://github.com/whitesmith/rubycritic

下面,我们针对RubyCritic给我们的一些坏代码的点,有针对性的做些代码调整。

这里使用git diff 比较新旧版本的差异。operator原来是实例方法,代码行7,并且里面还有一个if结构体。started_time_and_node 原来是实例方法,代码行4,并且里面还不止一个if结构体。

笔者review的方式:

1.实例方法修改为类方法(减少混入方法,解耦合,减低负责度)
2.多使用Ruby原生链式操作(减少中间变量,更少的代码,对于脚本语言,就是更快的执行效率,而且很多原生方法是C语言实现。)
3.去掉结构体 (现代编程语言的结构体,让代码具有丰富的逻辑性和可读性,但是缺点就是cpu的额外开销。)

以上部分,属于语法层面的奇技淫巧。

第二部分,我们从设计角度分析一下。

它的代码行只有141行,方法也只有7个。但是评级却是C。再看看代码分析细节,这里就展现一小部分,简直就是惨不忍睹,不好意思全展现给大家看了。

没有人会一开始就这样写代码,这种坏代码,永远都是渐渐变馊的。不过笔记仔细想来,当年遇到过比着还馊1000倍的代码(1000倍都不算过分)。

这是笔者做的第一版重构结果。

这里使用了策略模式。Stats_hash不再是充当一个集合的作用,现在变成了一个环境类,将原来依赖if结构数据分装到不同的行为类中。

第二版的改动计划是,引入work-job的模式,并发执行4个job。

第三版改动计划就是利用回调方式,去掉与该类不相干的代码,将逻辑分装到行为类里。

好了,写到这里,基本的代码层面的优化思路就这些了,其它就是开支散叶的过程,这里就不冗余了。下一节,咱俩聊一聊性能优化的一些思路。

更多细节 --> 干掉你代码中的坏味道
共收到 25 条回复
huacnlee 将本帖设为了精华贴 08月05日 17:09
2楼 已删除

相比 Rubocop 只做语法层面的检测,RubyCritic则进一步做了代码质量层面的检测;这些东西最好在项目 Set up 阶段就建起了,并且纳入 CI。

确实不错

6楼 已删除

果然不错,之前一直用rubocop

赞一个

学习到啦,赞

# gem install rubycritic
Fetching: thread_safe-0.3.5.gem (100%)
Fetching: descendants_tracker-0.0.4.gem (100%)
Fetching: equalizer-0.0.11.gem (100%)
Fetching: coercible-1.0.0.gem (100%)
Fetching: ice_nine-0.11.2.gem (100%)
Fetching: axiom-types-0.1.1.gem (100%)
ERROR:  Error installing rubycritic:
    axiom-types requires Ruby version >= 1.9.3.

无福享受了

spec.add_runtime_dependency 'flay', '2.8.0'
spec.add_runtime_dependency 'flog', '4.4.0'
spec.add_runtime_dependency 'reek', '4.1.0'

感觉reek还是要配置一下,另外干脆把Rubocop也集成进去好了

真的很赞 🆒

#3楼 @qinfanpeng 有没有把 rubycritic 纳入 CI 的经验可以分享一下的?

传个图 PK 一下分数 😄

代码review真的很重要!

#10楼 @msg7086 居然 1.9.3 都不够

ERROR:  Error installing rubycritic:
        reek requires Ruby version >= 2.1.0

nice project,很直观,可以帮助提升代码质量.

个人认为DuplicateMethodCall这种类型的错误相当不靠谱,至少得区分一下左值右值吧。右值反复出现确实可以用一个临时变量存储避免重复计算,但左值及write型的方法能不出现DuplicateMethodCall?

#21楼 @excosy 毕竟你是人,它是代码,所以也不能全听它的,有选择性的改动;毕竟,大部分都还是不错的建议。

#15楼 @lithium4010 好屌,能分享下怎么得这么高的分就更好了

最近加了 reek ,只用了它做检查,因为检查整个项目问题太多。我们写了一个 rake task 来检查 feature branch 内改动的代码,准备写功能的时候顺手改一下。

# lib/tasks/reek.rake
namespace :reek do
  desc "Check code smells for changed files (based on staging)"
  task :changed, [:branch] do |t, args| # 加一个 branch 参数
    branch = args[:branch] || 'staging'

    # 获取改变的文件名,剔除掉被删的文件
    re = `git diff --name-only #{branch}`
    files = re.split("\n").delete_if { |name| !File.exist?(name) }

    if files.blank?
      puts "\nNo files changed\n"
      return
    end

    # 打印一下文件列表
    puts "\nReek changed files:"
    files.each do |file|
      puts "  #{file}"
    end
    puts

    # 调用 reek
    system "bundle exec reek #{files.join(' ')}"
  end
end

详细信息可以看 这里

赞,最近代码开始有点味道,正好重构用。

idvc j,vb xmd kl sa,dvjlkKzc mhbjghfv,kIUYZx off knajk gjhas zfguvcAHIUzfihcxjh asfsuhgivja

赞,接触ruby on rails 半年,刚才跑了一下分,让我发现了自己许多不好的习惯~

需要 登录 后方可回复, 如果你还没有账号请点击这里 注册