重构 这种代码有什么好的重构方法吗?

xiaoping_rubyist · 2017年01月12日 · 最后由 xiaoping_rubyist 回复于 2017年01月13日 · 7441 次阅读

这种代码有什么好的重构吗?

def _assign_network_from_db
  begin
    @nodes.each do |node_name, node|
      next if node["topology_resource_id"].nil?
      topology_resource            = TopologyResource.find(node["topology_resource_id"])
      @interfaces[node_name].each do |interface_name,interface|
        iface =   topology_resource.topology_resource_interfaces.find_by_name interface_name
        if !iface.nil?
          if !iface.topology_vlan_id.nil?

            # Find the vlan details from DB and set them
            topology_vlan     = TopologyVlan.find(iface.topology_vlan_id)
            vlan              = topology_vlan.vlan
            physnet           = topology_vlan.physnet
            external_network  = topology_vlan.external_network
            if interface["ip"].nil?
              interface["ip"]               = iface.ip
            end
            if interface["network"].nil?
              interface["network"]          = iface.network
            end
            if interface["share_network"].nil?
              interface["vlan"]             = vlan
            end
            interface["physnet"]          = physnet
            interface["external_network"] = external_network
            interface["switch_id"]        = iface.switch_id
            interface["switch_port"]      = iface.switch_port
            topology_vlan.status = 'INUSE'
            topology_vlan.save!

          elsif !iface.nil? && !iface.ip.nil?
            if interface["ip"].nil?
              interface["ip"] = iface.ip
            end
            if interface["network"].nil?
              interface["network"] = iface.network
            end
            interface["skip_network"] = true
          end
        elsif !interface_name.match(/X\d:\d+/i).nil?
          # deal with the subinterface(like: X1:1) of UTM by Rex
          iface_parent_name = interface_name.split(":")[0]
          iface_parent = topology_resource.topology_resource_interfaces.find_by_name iface_parent_name

          vlan_result       =  _get_private_vlan
          vlan              = vlan_result["vlan"]
          physnet           = vlan_result["physnet"]
          external_network  = vlan_result["external_network"]

          interface["physnet"]          = physnet
          interface["external_network"] = external_network
          interface["switch_id"]        = iface_parent.switch_id
          interface["switch_port"]      = iface_parent.switch_port   

          if interface["share_network"].nil? then
            interface["vlan"]             = vlan
          end
        end

        if !interface["remote_node"].nil? && interface["routed_connection"].nil? then
          remote_node_name      = interface["remote_node"]
          remote_interface_name = interface["remote_interface"]
          interface["network"]  = nil if interface["network"].to_s.match(/\d/).nil?
          @interfaces[remote_node_name][remote_interface_name]["network"]          = interface["network"] 
          @interfaces[remote_node_name][remote_interface_name]["vlan"]             = vlan
          @interfaces[remote_node_name][remote_interface_name]["physnet"]          = physnet
          @interfaces[remote_node_name][remote_interface_name]["external_network"] = external_network
        end
      end
    end
  rescue => e
    self.status    = "error"
    self.message   = "Error occured while assigning network details from db. #{e}"
    self.backtrace = e.backtrace
    return
  end
end


可以把所有的 if 都拆出来写成一个方法。。恩。。。这么长看着太蛋疼了。

看着这代码一点也不 ruby,可是水平有限,想不出好的重构方法。大神们给点建议也好。谢谢。

#1 楼 @hging 嗯。这个办法我倒是想过了,但是感觉也不是特别理想。

  1. 下定决心要重构。要是没有时间没有决心就算了,其实也没有关系,能跑就挺好的了。
  2. 把所有这个方法下的代码移到一个单独文件,service object 也好,function 也好(其实最好)。原 class 用@nodes@interfaces呼叫这个。
  3. 把之前的测试移过来,保证通过。要是没有就得写,还得写详细一点。
  4. 随便玩,直到满意为止。

其实你有好些参数是只用一遍的,可以进行预处理不让逻辑进入循环内,比如 node, node_name 这些

另外,循环内不要查询,你这里查询数=nodes.size*interfaces.size*2,太多了,如果可以直接用 in 那不妨用一下。下面代码仅供参考..

def _assign_network_from_db
  @nodes
    .lazy
    .map do |node_name, node|
      next if node["topology_resource_id"].nil? 
      [node["topology_resource_id"], @interfaces[node_name]]
    end
    .reject(&:nil?)
    .each do |id, interface|
      ifaces = TopologyResourceInterface.where(topology_resource_id: id, name: interface.map(&:first))
      # ....
    end
end

def deal_with_interface_name
  #...
end

def deal_with_iface_parent_name
  #...
end

#4 楼 @billy 测试确实也没有,ruby 水平也不过关,心好累!

https://ruby-china.org/topics/31956 https://ruby-china.org/topics/32024 他写了两篇文章,可以先看一下。 不过,重构首先要技术过关,你自己也觉得 ruby 水平一般,就尽量先别动。能跑的代码总比出问题的代码好。哈哈。

尽量在没有 side effects 的时候不要去使用 each,可以把数据想象成通过一个一个的管道,看看 Enumerable module 的文档,例如 map 把数据进行了变换,reject , delete_if 对数据进行了过滤,reduce 将数据进行了归约,用组织数据的方式来思考代码的结构。

9 楼 已删除

#7 楼 @tesla_lee 还有一个 300 行+的 if else。其实我对写着代码的人还挺佩服的,能写这么长的 if else 说明对逻辑一清二楚。不过对于维护的人来说确实很残忍。

可以先去看下《ruby 元编程》。类似

if interface["ip"].nil?
  interface["ip"] = iface.ip
end

可以重构为

interface["ip"] ||= iface.ip

这类是比较基础的元编程。 一些可以在逻辑上进行重构:

next if node["topology_resource_id"].nil?

=>

@nodes.select{|n| n["topology_resource_id"].present?}.each do |node_name, node|

有些地方个人觉得很奇怪:

if !iface.nil?

=>

if iface.present?

而类似

topology_resource            = TopologyResource.find(node["topology_resource_id"])
.....
iface =   topology_resource.topology_resource_interfaces.find_by_name interface_name

的地方是有可能报错的。

physnet           = vlan_result["physnet"]          
external_network  = vlan_result["external_network"]

interface["physnet"]          = physnet
interface["external_network"] = external_network

中间变量显得没什么意义

代码有很多围绕一个层级型数据结构的 set/get 的操作,以及相应的 if .. else ..

建议围绕核心数据面向对象为 Node, Interface 等类(如果有逻辑)或者 struct (如果无逻辑)。

http://wiki.c2.com/?PrimitiveObsession

要想看完你的这个方法,需要鼓起好大的勇气!容我先去喝杯 82 年的莫吉托压压惊……

  1. 一个方法不要包含超过五个表达式。
  2. 如果某些表达式太长,可以单独成一个方法。
  3. 再给方法一个较为语义化的名字。

一般说到重构,都是提升代码的可读性,重新组织下代码,不会改变逻辑的。

#13 楼 @hiveer 其实我是想按照#12 @knwang的思路改的,奈何心有余力不足,先增强可读性吧,有时间再来考虑别的。一个 2000 多行的 lib 全部几乎都是这种,一个 200 多行的 if else 真是看的老眼昏花。感谢,同时感谢#5 楼 @saiga 贡献的代码与思路。学习了。

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