重构 问个冷门点滴,重构那些事

diudiutang · 2012年04月26日 · 最后由 neocanable 回复于 2013年03月10日 · 4051 次阅读

像这种非常相似的代码怎么重构 大写字母为 model 中的常量 @basic = Ex::User::BASIC @trade = Ex::User::TRADE @latest = Ex::User::LATEST @like = Ex::User::LIKE @gene = Ex::User::GENE 大家有没有什么奇葩写法

在 Ruby1.9.x 写的:

module Ex
  module User
    BASIC  = 1
    TRADE  = 2
    LATEST = 3
    LIKE   = 4
    GENE   =5
  end
end

class MyClass
  def initialize
    Ex::User::constants.each do |constant|
      instance_variable_set(:"@#{constant.downcase}",
                            Ex::User.const_get(constant))
    end
  end
end

t = MyClass.new
p t.instance_variables # [:@basic, :@trade, :@latest, :@like, :@gene]





Ruby 1.8 的 Symbol 没有 upcase 和 downcase 方法,你可以先把 symbol 转成 string 再 downcase. 除了 instance_variable_set,也可以用 eval 一族的方法来做这个。

你可以不要用实例变量,用方法嘛,然后用 method_missing 来做

多谢@skandhas @edokeh 用实例变量目的在于 view 中好调用,而且 controller 中生成实例变量也不能先实例化之后再生成

貌似用方法的确比用实例变量简单点,写到 helper 里面也能实现

%w(BASIC TRADE LATEST LIKE GENE).each do |i| class_eval(%Q{ def #{i.downcase}; Ex::User::#{i}; end }) end

#4 楼 @diudiutang 可以不用 class_eval,这样就行了 Ex::User.const_get(i)

@edokeh 用 class_eval 是为了批量生成这些方法 以便调用 const_get 只能在继承的类中使用,rails 除非是自定义的 module,不然在 controller 和 helper 里面都会报没有初始化常量的错误

Ex::User::BASIC Ex::User::TRADE Ex::User::LATEST Ex::User::LIKE Ex::User::GENE

@basic  = Ex::User::BASIC
@trade  = Ex::User::TRADE
@latest = Ex::User::LATEST
@like   = Ex::User::LIKE
@gene   = Ex::User::GENE

楼主觉得 以上代码 相似度很高,所以想重构吗? 如果是的话,我想说, 没有必要重构,代码虽然相似度高,但是清晰易懂, 没有任何复杂的逻辑,连判断都没有, 没有重构价值,根本不值得重构, 如果非要重构,就变成了,为了重构而重构的形式主义。

重构的目的是,降低代码的维护成本, 如果因为重构而降低了可读性,反而提高了维护成本, 那么就本末倒置啦。

:basic, :trade, :latest, :like, :gene

楼主能否把 Ex::User::BASIC 初始化代码拿出来,让我们看看

@ery 恩,有道理,本意是有几个 controller 需要调用这个,写到 application_controller 里又感觉没必要,索性这样写到 helper 里面,的确可读性大大降低了,以后别人维护也不好维护

#6 楼 @diudiutang 不是啊,我试了是 OK 的哎

module Ex
  module User
    BASIC  = 1
    TRADE  = 2
    LATEST = 3
    LIKE   = 4
    GENE   =5
  end
end
class MyClass
  def method_missing(name)
    Ex::User.const_get(name.upcase)
  end
end
puts MyClass.new.trade

匿名 #13 · 2012年04月26日

def method_missing name,*params Ex::User.const_get :"#{name.upcase}" end

匿名 #14 · 2012年04月26日

问下各位多行代码怎么弄的....?

@edokeh 我是在 controller 和 halper 里面调用都会报为未初始化的错误 @ery BASIC,TRADE 这些初始值都是一些数组

匿名 #16 · 2012年04月26日

#8 楼 @ery 说得好啊

没人觉得把常量赋值给变量是比较奇怪的事情么?

#15 楼 @diudiutang 我想,如果是符号和字符的话,可以想点办法,数组的话,就算了。

目前,这段代码 已经很清晰啦, 如果,非要嫌弃代码相似度高的话(有点长), 那么,可以想个办法,缩短常量名,比如

@basic  = USER_BASIC
@trade  = USER_TRADE
@latest = USER_LATEST
@like   = USER_LIKE
@gene   = USER_GENE


我觉得,就可读性而言,已经到极限啦

用实例变量目的在于view中好调用

这理由不成立呀 常量更可以直接调用

#18 楼 @ery #17 楼 @yuan 同意 可维护性、可读性是目的 重构只是手段

#20 楼 @hooopo +1. 如果重构后,代码的可读性大大降低的话,那是不可取的。

@hooopo 实际上我比较好奇把一个常量赋值给一个变量背后的目的是什么。如果不是非这样写不可,那还好;如果代码非得这么写不可,问题的根源应该不在 lz 给出的代码当中。

@ery @yuan @skandhas @hooopo 多谢各位的宝贵意见,最后仔细想了想,这段代码就功能性,可读性来说完全可以不重构,可能是我有点代码强迫症,代码越少看着越舒服,这个需要努力改正啊!

匿名 #24 · 2012年04月27日

我会把这些玩意塞到一个 Hash 常量中

我觉得是不是要反正 model 里面,并且加上 validate? 可以参见下 anti pattern 书里面的例子。

#8 楼 @ery 同意! 其他一些情形下也是。代码不是文章,并不一定简洁就是好的。在好的 IDE 支持下,繁冗一点的代码读起来也不会碍事。

如果你同类型的常量很多,并且有可能继续增加,可以定义一个 const_missing 方法。如果你的常量就那么少数几个 (<5),建议直接定义常量,如果你的常量很多,但不会增加,可以使用一下 const_set 方法定义。

@ery 说的对,这个地方没必要重构,你将这些东西写到 methods 里面生成,反而后面来的人读起来更麻烦

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