Should a block inside a foreach statement be only one line?

Question

I am reading a book called Clean Code, A Guide to Flexible Programming Skills, written by Robert C. Martin, and in his book he gives many useful tips on how to write good Java code.

And one of these tips:

Blocks inside if, else statements, for statements, etc. must be one line long. Probably this line should be a challenge function. This not only restricts the closure of a function, but also adds a documentary value, because a function called inside a block can have a beautiful descriptive name

For me it was a very strange hint, because from this code:

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; } 

Using this principle, I got the following:

 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"); } 

So basically the code doubled in size. Was this helpful? - May be.

What code is called clean in my example? What am I doing wrong? What do you think about it?

Answer 1

Honestly, I think the advice is stupid because it is so extreme.

Personally, if I had to do anything with your function, I would change it like this:

 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; } 

Benefits:

• You only view the map once, not twice.
• The call to list.add() not duplicated in two places.

Now, if you want to change something, the following will be a good candidate:

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

I would definitely do this if the above appeared in more than one place. Otherwise, maybe not (at least not initially).

Answer 2

I think it makes sense to simplify the condition itself. Than the contents of the if block, i.e.

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

becomes

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