重构 这种代码有什么好的重构方法吗？
这种代码有什么好的重构吗？
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
billy
#3
2017年01月12日
- 下定决心要重构。要是没有时间没有决心就算了，其实也没有关系，能跑就挺好的了。
- 把所有这个方法下的代码移到一个单独文件，service object 也好，function 也好（其实最好）。原 class 用
@nodes和@interfaces呼叫这个。 - 把之前的测试移过来，保证通过。要是没有就得写，还得写详细一点。
- 随便玩，直到满意为止。
saiga
#4
2017年01月12日
其实你有好些参数是只用一遍的，可以进行预处理不让逻辑进入循环内，比如 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
https://ruby-china.org/topics/31956 https://ruby-china.org/topics/32024 他写了两篇文章，可以先看一下。 不过，重构首先要技术过关，你自己也觉得 ruby 水平一般，就尽量先别动。能跑的代码总比出问题的代码好。哈哈。
尽量在没有 side effects 的时候不要去使用 each，可以把数据想象成通过一个一个的管道，看看 Enumerable module 的文档，例如 map 把数据进行了变换，reject , delete_if 对数据进行了过滤，reduce 将数据进行了归约，用组织数据的方式来思考代码的结构。
#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
中间变量显得没什么意义
knwang
#11
2017年01月13日
代码有很多围绕一个层级型数据结构的 set/get 的操作，以及相应的 if .. else ..
建议围绕核心数据面向对象为 Node, Interface 等类（如果有逻辑）或者 struct (如果无逻辑）。
hiveer
#12
2017年01月13日
要想看完你的这个方法，需要鼓起好大的勇气！容我先去喝杯 82 年的莫吉托压压惊……
- 一个方法不要包含超过五个表达式。
- 如果某些表达式太长，可以单独成一个方法。
- 再给方法一个较为语义化的名字。
一般说到重构，都是提升代码的可读性，重新组织下代码，不会改变逻辑的。