日常代码重构笔记

这里存放一些平时看代码时觉得需要重构的地方并记录下来(语言可能不止一种)，供大家参考|讨论。

1. 数据容器一致性操作|函数单一职能

功能需求：对多个节点布局时，如果节点排布超出屏幕则将预定节点调整至一个可滑动容器中(scrollUI)；
数据容器：1）originalNodeLst : 存放原始节点；2）showedNodeLst ：显示出来的第一层节点，视窗节点布局时使用该数据容器，当节点未被移至scrollUI时，该容器与originalNodeLst保持一致；3）nodeMap：节点名称与节点对象的映射，用于基于名字的节点检索；
重构前代码：

function T:getScrollUI()
    local cs = cc.size(0,0)
    local scrollUI = createScrollUI() -- 每次都创建一个新的
    for k,v in pairs(originalNodeLst) do
        local hasGot = false -- 标记是否已经找到需要移动到scroll中的原始节点
        for k1,v1 in pairs(preDefNodeName) do
            if not hasGot then
                if v == nodeMap[v1] then
                    hasGot = true
                    moveToScrollUI(v,scrollUI) -- 将原始节点移至scrollUI
                end
            end
        end
    end

    if self.scrollUI then -- self.scrollUI额外保存了scrollUI
        self.scrollUI:removeFromParent() -- 移除旧的scrollUI
        self.scrollUI = nil
    end

    self.scrollUI = scrollUI
    self.scrollUI:setContentSize(self.scrollUI:getInnerContainerSize())
    self.scrollUIParent:addChild(self.scrollUI)
    return self.scrollUI
end

function T:adaptLayout()
    local viewContSize = self:getOriViewSize() -- 原始视窗大小
    if viewContSize.height > maxViewHeight then -- 原始视窗大小超出最大限定范围，将一些节点搬移至scrollview中
        local subHeight = viewContSize.height - maxViewHeight -- 需要缩短的高度
        local scrollUI = self:getScrollUI()
        local scrollUIContSize = scrollUI:getContentSize()
        scrollUIContSize.height = scrollUIContSize.height - subHeight -- 需要缩短的高度体现在scrollUI中
        scrollUI:setContentSize(cc.size(scrollUIContSize.width,scrollUIContSize.height))

        self.showedNodeLst = nil -- 该容器存放视窗上显示的一级节点(原始节点中被移至scrollUI的为二级节点)
        local flag = false -- 是否已将scrollUI更新至容器中
        for k,v in pairs(originalNodeLst) do
            if self:isInScrollUI(v) then
                flag = true
                table.insert(self.showedNodeLst,scrollUI)
            else
                table.insert(self.showedNodeLst,v)
            end
        end
    end
    doLayout()
end

代码存在一下几个问题：
1）数据容器更新的不一致性：程序使用self.scrollUI单独保存了对象，而该对象在adaptLayout()中又被加入到了showedNodeLst数据容器中，那么后续析构该对象时总要坚固两部分，一不留神就产生了野指针；
2）函数职能不单一：adaptLayout()的工作是做界面布局，但上述程序还进行了数据容器更新的操作，很明显，这一操作应该在数据发生变化时完成，而不应该延迟到UI界面布局阶段；
3）冗余的标识性变量：上述代码用标志性变量干预了循环体执行流程，看起来不直观且易出错，可将其独立成简易的函数体以消除这些变量；
4）避免对象的重复构造：每次调用adaptLayout()时总会创建一个新的scrollUI，这样做效率不高，可将其保存至nodeMap映射容器中；
重构后的代码如下：

function getScrollUI()
    local scrollUI = nodeMap["scrollUI"]
    if scrollUI then return scrollUI end

    local isNeedMove = function ( node ) -- 封装成一个独立函数 避免使用标记变量控制函数流程
        for k,v in pairs(preDefNodeName) do
            if node == self.nodeMap[v] then
                return true
            end
        end
        return false
    end

    scrollUI = createScrollUI()
    local scrollUIShowedIdx = nil -- 记录scrollUI将存放在showedNodeLst中的位置
    for k,v in pairs(originalNodeLst) do
        if isNeedMove then
            local idx = table.findkey(showedNodeLst,v)
            table.remove(showedNodeLst,idx)
            scrollUIShowedIdx = scrollUIShowedIdx and scrollUIShowedIdx < idx and scrollUIShowedIdx or idx
            moveToScrollUI(v,scrollUI)
        end
    end
    scrollUI:setContentSize(scrollUI:getInnerContainerSize())
    nodeMap["scrollUI"] = scrollUI
    table.insert(showedNodeLst,scrollUIShowedIdx,scrollUI)
    self.scrollUIParent:addChild(scrollUI)
end
function T:adaptLayout() -- 职能分离，布局就只做布局的事
    local viewContSize = self:getOriViewSize() -- 原始视窗大小
    if viewContSize.height > maxViewHeight then -- 原始视窗大小超出最大限定范围，将一些节点搬移至scrollview中
        local subHeight = viewContSize.height - maxViewHeight -- 需要缩短的高度
        local scrollUI = self:getScrollUI()
        local scrollUIContSize = scrollUI:getContentSize()
        scrollUIContSize.height = scrollUIContSize.height - subHeight -- 需要缩短的高度体现在scrollUI中
        scrollUI:setContentSize(cc.size(scrollUIContSize.width,scrollUIContSize.height))
    end
    doLayout()
end

总结：我们重构这部分代码，要怎么思考呢？很重要的一点是：我们要时刻关注我们的操作对已经定义好的数据结构|容器的影响，因为很多的野指针正是因为数据更新与容器操作不一致行造成的。如上例，当对节点变更后，最好紧随着更新其容器内容。而且这些容器一般都要编写统一的初始化|销毁接口。比如上述功能，会有一个init()函数将originalNodeLst、showedNodeLst及nodeMap全部置空。实际上我们在实现代码时应当时刻保持对数据容器的敏感性，避免随意添加变量来保存容器内数据引用。如果发现容器无法支撑要实现的逻辑，建议重新调整逻辑来适应我们既有的容器结构以保证程序的稳定性，随意修改、增加容器都不是明智之举。

2. 解开缠绕的嵌套逻辑分支

考虑一种需求：当进入某地图场景时需要隐藏一些按钮或是其他元素，当离开该场景时恢复。实现时只需要在进入时做标记，不在相关地图时判断是否被标记，如果已标记表明之前进入过地图，此时需要做一些恢复性工作。
如果情况变得更复杂一些，从当前地图进入一个属于该地图的小房间，也需要做类似的动作，从小房间进入更小的房间同样也有些类似的操作要完成，那么程序实现起来会时怎么样的呢？如下显示：

function doSomething()
    local hasEnterMap = false -- 标记是否进入过场景 下面类似
    local hasEnterSamllMap = false
    if isInMap() then
        hasEnterMap = true
        doSomethingForMap() -- 比如隐藏一些sprite icon等
        if isInSamllMap() then
            hasEnterSamllMap = true
            doSomethingForSmallMap()
        else
            if hasEnterSamllMap then -- 不在小房间 有可能是因为从小房间跳回至大房间 因此需要做检查 
                hasEnterSamllMap = false
                canclForSmallMap()
            end
        end
    else
        if hasEnterMap then
            hasEnterMap = false
            canclForMap() -- 恢复doSomethingForFstLvl的一些行为 如将隐藏的元素重新显示出来
            if hasEnterSamllMap then -- 前面不是已经检查了吗？为什么此处还要检查？因为有可能直接从小房间传送出了当前所在地图 而没有经过当前大地图
                hasEnterSamllMap = false
                canclForSmallMap()
            end
        end
    end
end

按照上面的写法，如果在有些逻辑继续嵌套下去，代码的可维护性将非常低下。上面之所以嵌套逻辑，是因为满足想要满足isInSamllMap条件必定先要满足isInMap条件，这么考虑，上面的逻辑是合理的，但是从职能角度考虑就不合理了。大房间负责做的事情可以完全与小房间独立开来，独立开来的益处就是格子处理自己的进出逻辑，而不用管其他的情形。

function doSomething()
    local hasEnterMap = false -- 标记是否进入过场景 下面类似
    local hasEnterSamllMap = false

    if isInMap() then
        hasEnterMap = true
        doSomethingForMap()
    else
        if hasEnterMap then
            hasEnterMap = false
            canclForMap()
        end
    end

    if isInSamllMap() then
        hasEnterSamllMap = true
        doSomethingForSmallMap()
    else
        if hasEnterSamllMap then
            hasEnterSamllMap = false
            canclForSmallMap()
        end
    end
end

上面的代码需要注意的是：将判断分支移出之后，每次执行doSomething都要执行isInSamllMap操作而不是仅在满足isInMap之后，因此最好对判断做些优化，比如采用map结构加速判断。

3. 寻求更贴合逻辑本质的控制变量

有这样一个需求：当接收到服务器下发的某一物品掉落信息时，播放特定的掉落飞行动画，下面是一种实现方式。

function server_addItem(item) -- 接收到相关掉落信息
    showDropEffect(item)
end
function showDropEffect(item)
    addToArr(item) -- 加入至容器
    if getArrItemNum() == 1 then
        local aniUI = getUI("aniUIName")
        aniUI.repeatAction(play) -- 重复播放特效
    else
        -- 如果容器中的道具数目不为1，说明aniUI应该还在循环执行play函数 
        -- 因此不需要执行播放动作
    end
end
function play()
    local num = getArrItemNum()
    if num > 0 then
        local item = getOneArrItem()
        playOneAni(item) -- 播放一次动画
        removeOneArrItem(item)
    else
        -- 容器中所有item已全部播完特效
        local aniUI = getUI("aniUIName")
        aniUI.stopAction()
    end
end

上面实现方法关键之处在于：判断容器中item数目是否为1，如果为1则此时道具为第一个道具，需要开启循环播放特效模式
这种做法的问题在于：如果aniUI的循环动作被异常停止(如界面被强制关闭)，那么以后再掉落物品将不再播放特效，因为异常停止play函数后，容器中item数目始终大于1，启动循环播放动画的条件永远不会判过。
如何修改呢？实际上是否需要启动循环播放机制，本质在于当前的循环系统是否还在运作。如果未运作就开启，已运作则不做处理。重构后的代码如下：

function showDropEffect(item)
    addToArr(item) -- 加入至容器
    if not isPlaying then -- 循环机制已断 重新启动
        local aniUI = getUI("aniUIName")
        aniUI.repeatAction(play) -- 重复播放特效
    else
        -- 如果容器中的道具数目不为1，说明aniUI应该还在循环执行play函数 
        -- 因此不需要执行播放动作
    end
end
function play()
    local num = getArrItemNum()
    if num > 0 then
        isPlaying = true
        local item = getOneArrItem()
        playOneAni(item) -- 播放一次动画
        removeOneArrItem(item)
    else
        isPlaying = false
        -- 容器中所有item已全部播完特效
        local aniUI = getUI("aniUIName")
        aniUI.stopAction()
    end
end