4

私は、 RobertC.Martinによって書かれたCleanCode-A Handbook of Agile Software Craftsmanshipという本を読んでいます彼の本の中で、彼は優れたJavaコードを書くための役立つヒントをたくさん提供しています。

そして、それらのヒントの1つは次のとおりです。

ifステートメント、elseステートメント、forステートメントなどのブロックは、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");
        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");
}

つまり、基本的にコードのサイズは2倍になりましたが、役に立ちましたか?- 多分。

私の例のどのコードがいわゆるクリーンですか?私は何が間違っているのですか?これについてどう思いますか?

4

2 に答える 2

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回ではなく1回だけ実行します。
  2. 呼び出しはlist.add()2か所で重複していません。

ここで、何かを除外したい場合は、次の候補が適しています。

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

上記が複数の場所に表示された場合、私は間違いなくそれを行います。そうでなければ、おそらくそうではありません(少なくとも最初はそうではありません)。

于 2012-12-12T07:29:01.793 に答える
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 に答える