4

我正在阅读Robert C. Martin编写的名为Clean Code -A Handbook of Agile Software Craftsmanship的书，在他的书中他提供了许多有用的技巧来编写好的 Java 代码。

其中一个提示是：

if 语句、else 语句、for 语句等中的块应为一行。可能那行应该是一个函数调用。这不仅使封闭函数保持小，而且还增加了文档价值，因为在块内调用的函数可以有一个很好的描述性名称

对我来说，这是一个非常奇怪的提示，因为从这段代码中：

public Map<String, List<Issue>> mapComponentToIssueList(List<Issue> issues) {
    Map<String, List<Issue>> map = new HashMap<String, List<Issue>>();

    for (Issue issue : issues) {
        String componentName = issue.getComponents().iterator().next().getString("name");
        if (map.containsKey(componentName)) {
            map.get(componentName).add(issue);
        } else {
            List<Issue> list = new ArrayList<Issue>();
            list.add(issue);
            map.put(componentName, list);
        }
    }
    return map;

}

使用这个原则，我得到了这个：

public Map<String, List<Issue>> mapComponentToIssueList(List<Issue> issues) {
    Map<String, List<Issue>> componentNameIssueListMap = new HashMap<String, List<Issue>>();
    for (Issue issue : issues) {
        populateMapWithComponenNamesAndIssueLists(componentNameIssueListMap, issue);
    }
    return componentNameIssueListMap;
}

private void populateMapWithComponenNamesAndIssueLists(Map<String, List<Issue>> componentNameIssueListMap, Issue issue) {
    String componentName = getFirstComponentName(issue);
    if (componentNameIssueListMap.containsKey(componentName)) {
        componentNameIssueListMap.get(componentName).add(issue);
    } else {
        putIssueListWithNewKeyToMap(componentNameIssueListMap, issue, componentName);
    }
}

private void putIssueListWithNewKeyToMap(Map<String, List<Issue>> componentNameIssueListMap, Issue issue, String componentName) {
    List<Issue> list = new ArrayList<Issue>();
    list.add(issue);
    componentNameIssueListMap.put(componentName, list);
}

private String getFirstComponentName(Issue issue) {
    return issue.getComponents().iterator().next().getString("name");
}

所以基本上代码的大小翻了一番。有用吗？- 也许。

我的示例中的什么代码是所谓的clean？我究竟做错了什么？对此你们怎么看？

4

2 回答 2

1

我认为简化条件本身更有意义。比 if 块的内容，即

public void method(){
...
  if( mycondition1 && mycondition2 && mycondition3 && mycondition4 && mycondition5 && mycondition6 && mycondition7 && mycondition8 ) {
   dosomething();
  }
...
}

变成

public void method(){
...
  if( conditionsAreTrue() ) {
   dosomething();
  }
...
}

boolean conditionsAreTrue(){
return  mycondition1 && mycondition2 && mycondition3 && mycondition4 && mycondition5 && mycondition6 && mycondition7 && mycondition8;
}
于 2012-12-12T07:30:36.833 回答
1

坦率地说，我认为这个小费很愚蠢，因为它太极端了。

就个人而言，如果我要对您的功能做任何事情，我会像这样更改它：

public Map<String, List<Issue>> mapComponentToIssueList(List<Issue> issues) {
    Map<String, List<Issue>> map = new HashMap<String, List<Issue>>();

    for (Issue issue : issues) {
        String componentName = issue.getComponents().iterator().next().getString("name");
        List<Issue> list = map.get(componentName);
        if (list == null) {
            list = new ArrayList<Issue>();
            map.put(componentName, list);
        }
        list.add(issue);
    }
    return map;
}

好处是：

  1. 您只需进行一次地图查找，而不是两次。
  2. 呼叫不会在list.add()两个地方重复。

现在，如果您想排除某些因素，以下将是一个不错的选择：

        List<Issue> list = map.get(componentName);
        if (list == null) {
            list = new ArrayList<Issue>();
            map.put(componentName, list);
        }

如果以上内容出现在多个地方，我肯定会这样做。否则，可能不会（至少最初不是）。

于 2012-12-12T07:29:01.793 回答