0
\$\begingroup\$

I was refactoring an ugly piece of code, and I'm unsure if there is a better way of doing this, and whether I'm overdoing it with streams.

Basically I get a bunch of columns, which should be grouped by the name of the sheet they belong to, so that I can return a map of sheet names to a set of column names.

BTW, I can't change the return type (I know it should be a MultiMap).

@Override
public Map<String, Set<String>> getReferencedColumnNames(String columnName) {
    return _formulaSheetSchema.column(columnName).get().formula()
            .computeReferencedColumns(getSheetName()).stream()
            .collect(groupingBy(
                    ColumnRef::sheetName,
                    LinkedHashMap::new,
                    collectingAndThen(toSet(), set ->
                            set.stream().map(ColumnRef::columnName).collect(toSet()))));
}
\$\endgroup\$
  • \$\begingroup\$ This question is incomplete. To help reviewers give you better answers, please add sufficient context to your question. The more you tell us about what your code does and what the purpose of doing that is, the easier it will be for reviewers to help you. Questions should include a description of what the code does \$\endgroup\$ – Simon Forsberg Jun 29 '19 at 8:18
1
\$\begingroup\$

I'd format this code slightly differently:

return _formulaSheetSchema.column(columnName).get()
  .formula()
  .computeReferencedColums(getSheetName()).stream()
  .collect(groupingBy(ColumnRef::sheetName,
    LinkedHashMap::new,
    collectingAndThen(toSet(), set -> set.stream()
      .map(ColumnRef::columnName).collect(toSet())));

Also the downstream operation you use here is wasteful, because it collects the set and then restreams it. Instead use the mapping collector:

  .collect(groupingBy(ColumnRef::sheetName,
    mapping(ColumnRef::columnName, toSet()));

Sidenote: I personally prefer using 2-space indents across the board, but this formatting works with 4-space indents just as well...

Additionally the _-prefix for instance variables is something I dislike, but so long as you're consistent about it, that should be fine.

I also dislike that you're not checking whether the column actually exists, before accessing it's formula.

\$\endgroup\$
  • \$\begingroup\$ Thank you, that looks better, the collectingAndThen felt really clunky. The get() is safe in that context (there are sheet types without formulas, but the code is actually in a formula sheet). \$\endgroup\$ – Landei Jun 22 '19 at 21:28

Not the answer you're looking for? Browse other questions tagged or ask your own question.