DRY与简单性的平衡

这个事例说起来相当简单。不过小中见大，它大致体现了我和pair在DRY vs. 简单性上的差别，和那个“这样代码重用”里面的例子体现了同样的分歧。

目标是重构下面的测试代码：

public void test1() {
  Account acct = new Account();
  acct.setName("test");
  acct.setType(TypeEnum.Type1);
  acct.setActive(true);
  Result result = runSomeApi(acct);
  assertEquals("test", result.getName());
  assertTrue(result.isActive());
  ResultType[] arr = result.getTypes();
  assertEquals(1, arr.length);
  assertEquals("type1", arr[0].getName());
}

public void test2() {
  Account acct = new Account();
  acct.setName("test");
  acct.setType(TypeEnum.Type2);
  acct.setActive(true);
  Result result = runSomeApi(acct);
  assertEquals("test", result.getName());
  assertTrue(result.isActive());
  ResultType[] arr = result.getTypes();
  assertEquals(1, arr.length);
  assertEquals("type2", arr[0].getName());
}
public void testNull(){
  Account acct = new Account();
  acct.setName("test");
  acct.setType(null);
  acct.setActive(true);
  Result result = runSomeApi(acct);
  assertEquals("test", result.getName());
  assertTrue(result.isActive());
  ResultType[] arr = result.getTypes();
  assertEquals(2, arr.length);
  assertEquals("type1", arr[0].getName());
  assertEquals("type2", arr[1].getName());
}

首先通过发现重复，我和pair都同意下面的重构：

private String name = "test";
private getResult(TypeEnum type) {
  Account acct = new Account();
  acct.setName(name);
  acct.setType(type);
  acct.setActive(true);
  return runSomeApi(acct);
}
public void test1() {
  Result result = getResult(TypeEnum.Type1);
  assertEquals(name, result.getName());
  assertTrue(result.isActive());
  ResultType[] arr = result.getTypes();
  assertEquals(1, arr.length);
  assertEquals("type1", arr[0].getName());
}

public void test2() {
  Result result = getResult(TypeEnum.Type2);
  assertEquals("test", result.getName());
  assertTrue(result.isActive());
  ResultType[] arr = result.getTypes();
  assertEquals(1, arr.length);
  assertEquals("type2", arr[0].getName());
}
public void testNull(){
  Result result = getResult(null);
  assertEquals(name, result.getName());
  assertTrue(result.isActive());
  ResultType[] arr = result.getTypes();
  assertEquals(2, arr.length);
  assertEquals("type1", arr[0].getName());
  assertEquals("type2", arr[1].getName());
}

分歧来自于下一步，我认为重构到下面这样就够了：

private String name = "test";
private getResult(TypeEnum type) {
  Account acct = new Account();
  acct.setName(name);
  acct.setType(type);
  acct.setActive(true);
  return runSomeApi(acct);
}
private void assertInvariants(Result result) {
  assertEquals("test", result.getName());
  assertTrue(result.isActive());
}
private void verifyWhenTypeIsNotNull(String expectedType, TypeEnum type){
  Result result = getResult(type);
  assertInvariants(result);
  ResultType[] arr = result.getTypes();
  assertEquals(1, arr.length);
  assertEquals(expectedType, arr[0].getName());

}
public void test1() {
  verifyWhenTypeIsNotNull("type1" TypeEnum.Type1);
}

public void test2() {
  verifyWhenTypeIsNotNull("type2", TypeEnum.Type2);
}
public void testNull(){
  Result result = getResult(null);
  assertInvariants(result);
  ResultType[] arr = result.getTypes();
  assertEquals(2, arr.length);
  assertEquals("type1", arr[0].getName());
  assertEquals("type2", arr[1].getName());
}

而pair重构到这一步后，仍然觉得不够DRY，坚决推进下一步的重构：

private String name = "test";
private getResult(TypeEnum type) {
  Account acct = new Account();
  acct.setName(name);
  acct.setType(type);
  acct.setActive(true);
  return runSomeApi(acct);
}
private void verify(String[] expectedTypes, TypeEnum type){
  Result result = getResult(type);
  assertEquals("test", result.getName());
  assertTrue(result.isActive());
  ResultType[] arr = result.getTypes();
  assertEquals(expectedTypes.length, arr.length);
  for(int i=0; i<expectedTypes.length; i++) {
    assertEquals(expectedTypes[i], arr[i]);
  }
}
public void test1() {
  verify(new String[]{"type1"} TypeEnum.Type1);
}

public void test2() {
  verify(new String[]{"type2"}, TypeEnum.Type2);
}
public void testNull(){
  verify(new String[]{"type1", "type2"}, null);
}

我的观点：
最后这一步的重构，是以简单性换取非常细微的DRY，并不划算。最好的情况，也就是和重构前半斤八两，所以不值得投入时间来做。而重构前的代码更能灵活适应变化。

比如，经过分析，我们完全可以仅assertEquals(2, arr.length)而不用分别对每个元素进行assert了（同样的逻辑在别的测试中已经覆盖了，而且，测试代码决定"type1", "type2"的顺序也加大了测试和被测试代码的耦合，这个顺序本来是无所谓的。）这个变化在重构前很容易，只要从testNull()里面删掉两行assertEquals就可。而重构后的代码则需要更复杂的逻辑控制才能达到这个目标。



那么，你怎么看？

评论

2 楼 fixopen 2007-08-15  

我同意gigix，DRY是指对一件事情说并且只说一次。
程序里面如果发现某些代码是重复的，那么应该区分出来是两件事情还是一件事情，如果因为某些原因，这两件事情看起来似乎很像，但逻辑上仍然是两件事情，不要试图去消除这种重复，除非能在更高层次把这两件事情归并为一件事情。

1 楼 gigix 2007-02-16  

在我看来，问题的关键就在于“verify”这个名字。你抽取一个方法却发现很难给它起一个更漂亮的名字，这本身就说明这些代码只是偶然地类似，而不是有什么内在的业务逻辑。强行抽取这种偶然类似的代码，是对模块内聚性的破坏。这一步的调整不是重构，因为它没有让软件的质量提升。

所以我的建议是等一等：等到重复代码真正出现的时候、给未来的添加代码或者修改代码带来困扰的时候再重构。例如你们的这个例子，最后一步重构的动机实际上是想象出来的，它并没有带来任何困扰。