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

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

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

共收到 28 条回复
146

在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一族的方法来做这个。

586

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

292

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

292

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

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

586

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

292

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

96

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

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

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

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

1

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

594

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

292

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

586

#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

1765

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

1765

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

292

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

1765

#8楼 @ery 说得好啊

96

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

594

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

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

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


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

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

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

8

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

146

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

96

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

292

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

1926

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

1959

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

96

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

2456

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

96

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

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