2
\$\begingroup\$

Our company uses an old version of JAXB so it does not allow generics. Other than that, I am using recursive calls because Rows can have subrows and I want to find out if any of the rows for the given column id i has any values.

I am interested to know if I got the recursion right.

private static boolean anyCellHasValue(CommonRowType row, int i) {

    CustomCellType cell = (CustomCellType)row.getCustomRow().getCell().get(i);

    if(!CELL_EMPTY.equals(cell.getType())) {
        return true ;
    }

    if (row.getChildren() != null && row.getChildren().getRowData() != null && 
        !row.getChildren().getRowData().isEmpty()) {
        for (int k = 0; k < row.getChildren().getRowData().size(); k++) {
            if(anyCellHasValue((CommonRowType) row.getChildren().getRowData().get(k), i)) {
                return true;
            }
        }
    }

    return false;
}
\$\endgroup\$
4
\$\begingroup\$

This site is more about the readability and structure of code rather than actual correctness. I will comment on that and maybe a more readable program will help you find any potential errors?

I would recommend that you:

  • Extract complex if statements to their own methods describing the test
  • Use the new style java iteration instead of the old for loop
  • Reduce the block nesting level by returning early when you know you have a negative match
  • Use good naming for all variables. In this case the variable i desperately needs a better name!

This should produce (sans renaming of i) code that look something like this: (not compiled or tested in any way :))

private static boolean anyCellHasValue(CommonRowType row, int i) {
    if (isNonEmptyCellType(row, i)) {
        return true;
    }

    if (!hasChildren(row)) {
        return false;
    }

    for (CommonRowType childRow : row.getChildren().getRowData()) {
        if (anyCellHasValue(childRow, i)) {
            return true;
        }
    }

    return false;
}

private static boolean isNonEmptyCellType(CommonRowType row, int i) {
    CustomCellType cell = (CustomCellType)row.getCustomRow().getCell().get(i);
    return !CELL_EMPTY.equals(cell.getType()));
}

private static boolean hasChildren(CommonRowType row) {
    return row.getChildren() != null 
            && row.getChildren().getRowData() != null 
            && !row.getChildren().getRowData().isEmpty();
}
\$\endgroup\$
2
\$\begingroup\$

I'd improve @kyck-ling's hasChildren method a little bit:

private static boolean hasChildren(CommonRowType row) {
    final Children children = row.getChildren();
    if (children == null) {
        return false;
    }

    final RowData rowData = children.getRowData();
    if (rowData == null) {
        return false;
    }
    if (rowData.isEmpty()) {
        return false;
    }
    return true;
}

Now it uses guard clauses and a few local variables to remove some duplication like row.getChildren().getRowData(). Anyway, it still violates the Law of Demeter. A better solution is to create a hasChildren method in the Children class. See also: Feature envy smell.

Another interesting point that that it seems that the children.getRowData() returns a collection which could be null. It reminds me the Item 43: Return empty arrays or collections, not nulls chapter of Effective Java, 2nd Edition.

\$\endgroup\$
  • \$\begingroup\$ maybe replace the last two if-clauses with return rowData != null && !rowData.isEmpty(); \$\endgroup\$ – kyck-ling Jul 13 '12 at 21:26
  • 1
    \$\begingroup\$ @kyck-ling I'm not sure about replacing those. I tend to find double negatives quite hard to follow without doing a double take. Perhaps !(rowData == null || rowData.isEmpty()) or put it into it's own method? \$\endgroup\$ – dreza Jul 13 '12 at 22:59
  • \$\begingroup\$ @dreza its not a double negative but sure, there are other ways to write it. My main point was that seven rows of code could be one row. \$\endgroup\$ – kyck-ling Jul 14 '12 at 6:35
  • \$\begingroup\$ @kyck-ling: Source code storage costs almost nothing but developer time is expensive, so I'd choose the cheaper one, which is easier to understand, therefore easier (cheaper) to maintain and less error-prone. \$\endgroup\$ – palacsint Jul 14 '12 at 23:09
  • \$\begingroup\$ @palacsint I'm not talking about storage but readability (and thus developer time) same as you. But I guess this boils down to personal choice more than anything else. \$\endgroup\$ – kyck-ling Jul 15 '12 at 0:13

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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