代码之丑（四）——代码找茬游戏

    转载按：这是我在InfoQ上看到的一个关于代码书写规范的一系列的讨论，文后的评论，有些比较有意思的，我也一并摘抄下来了，大家有什么看法、想法也可以在这里继续讨论。

---

这是一个找茬的游戏，下面三段代码的差别在哪：

if (1 == insertFlag) {
    retList.insert(i, newCatalog);
  } else {
    retList.add(newCatalog);
  }
if (1 == insertFlag) {
    retList.insert(m, newCatalog);
  } else {
    retList.add(newCatalog);
  }
if (1 == insertFlag) {
    retList.insert(j, newPrivNode);
  } else {
    retList.add(newPrivNode);
  }


答案时间：除了用到变量之外，完全相同。我想说的是，这是我从一个文件的一次diff中看到的。

不妨设想一下修改这些代码时的情形：费尽九牛二虎之力，我终于找到该在哪改动代码，然后改了。作为一个有职业操守的程序员，我知道别的地方也需要类 似的修改。 于是，趁人不备，把刚做修改拷贝了一份，放到另外需要修改的地方。修改了几个变量，编译通过了。世界应该就此清净，至少问题解决了。

好吧！虽然这个程序员有职业操守的程序员，却缺少了些职业技能，至少在挥舞着“拷贝粘贴”的锤子时，他没有嗅到散发出的臭味。

只要意识到坏味道，修改是件很容易的事，提出一个新函数即可：

void AddNode(List& retList, int insertFlag, int pos, Node& node) {
    if (1 == insertFlag) {
      retList.insert(pos, Node);
    } else {
      retList.add(node);
    }
  }


于是，原来那三段代码变成了三个调用：

AddNode(retList, insertFlag, i, newCatalog);
AddNode(retList, insertFlag, m, newCatalog);
AddNode(retList, insertFlag, j, newPrivNode);


当然，这种修改只是一个局部的微调，如果有更多的上下文信息，我们可以做得更好。

重复，是最为常见的坏味道。上面这种重复实际上是非常容易发现的，也是很容易修改。但所有这一切的前提是，发现坏味道。长时间生活在这种代码里面， 我们会对坏味道失去嗅觉。更可怕的是，一个初来乍到的嗅觉尚灵敏的人意识到这个问题，那些失去嗅觉的人却告诫他，别乱动，这挺好。

趁嗅觉尚在，请坚持代码正义。

---

以下是原出处的评论（摘抄）：

2010年11月25日 下午7时5分 发表人 Hou Bowei

这就是传说中的DRY（don't repeat yourself)原则吧，哈哈。

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月25日 下午7时53分 发表人 xishun feng

我个人觉得，一个函数必须的参数如果超过三个，那就很难读了。

这是作者改写后的代码：

void AddNode(List& retList, int insertFlag, int pos, Node& node) {
    if (1 == insertFlag) {
      retList.insert(pos, Node);
    } else {
      retList.add(node);
    }
}

如果不怕麻烦，还可以改写成这样：

private class ListWithSpecialAddRule extends List {
    int insertFlag;

    public PositionAddList(int insertFlag) {
        this.insertFlag = insertFlag;
    }

    public void add(int pos, Node& node) {
        if (1 != insertFlag) {
          pos = this.size();
        }

        this.insert(pos, Node);
    }
}

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月26日 上午3时44分 发表人 高 翌翔

原来的代码只说明了“怎么做”，至于代码的目的是什么（即“做什么”）要读者自己领悟。

修改后的代码仅仅是一种形式化的转换！

封装的方法 AddNode 仅仅是一种对代码本身的注释，而并没有从业务角度说明“做什么”；
此外，AddNode 的功能并不单一，其中包含了插入或添加的选择逻辑，而方法名却只反应了添加功能，
这使得方法名变得含混不清，不能一目了然。

总之，这种机械地 Extract Method，不值得提倡！

方法命名是一件非常慎重而严肃的工作，信手拈来的方法名所散发的臭味比重复代码更令人作呕！

在尚未找到恰当的方法名之前，我宁愿保留重复代码！

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月26日 上午3时57分 发表人 高 翌翔

to xishun feng

首先，为了缩短参数而使用继承实在太草率；
其次，一个方法拆分为两个紧密耦合的方法，只是增加了复杂度而已，费力不讨好！

长参数确实不值得提倡，但绝不能通过机械地拆分来缩短参数！

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月26日 上午6时59分 发表人 毕 成栋

已经不错了，这是我每期看到现在，唯一“找不出茬”的文章。
不过method name确实臭了点。可以改一个addProjNode或者addBomNode之类的名字。
另外if (1 == insertFlag) 这种“魔术数字”也可以去掉。

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月28日 上午6时30分 发表人 chen Joseph

不认为为了函数参数数量而去封装成类，这样可读性差了，而且复杂度增加。因为你得考虑组合或者继承，而且增加的代码量，应用程序也变大了，为了一个简单的function，最终甚至影响到应用程序大小或者性能，得不偿失。
函 数参数数量不能说明可读性，可能好点的去掉多参数的方式是其实是把上文中的pos和node做成一个参数，pair就可以, 因为pos和node其实是依赖性参数，另外List& retList不作为参数传递(当然这个要依赖于上下文),如：

List& retList;
typedef pair<int, Node&> NodePair;

void AddNode(int insertFlag, NodePair &np)
{
if (1 == insertFlag) {
retList.insert(np.first, np.second);
} else {
retList.add(np.second);
}
}</int,></pos,>

－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－－

2010年11月29日 上午9时5分 发表人 lei chen

写2个方法：
1，AddNode(...)
2,InsertNode(...)
然后去调用，这样方法这则单一，便于以后修改，而且省去if语句，代码清晰度更高