Findings from Sonar results

Hi there,

I have run Sonar runner against current OpenLMIS codebase (2.0-TZ branch), and came up to some findings and thoughts that I’d like to share to this group. (Thanks to Josh for setting Sonar gradle task up.)

Sonar Configuration

  • sonar.binaries property is not set, which causes one of the “Blocker” issues saying “”@RequiredParam" is not available at runtime and cannot be seen with reflection."

  • Generally speaking, the default Sonar Java profile makes sense. It seems to be a good idea using it as a start point and tweaking it.
    Tech Debts

  • Should we exclude test code or using a different profile for test code when analysing tech debts?

  • Another “Blocker” issue is about RnrColumn class, which overrode equals() method but not hashCode().

  • A most common cluster of tech debt is about error handling. More specifically, there are hundreds of places we eat up exceptions without logging or re-throwing them. Some of the cases worth further discussion. For example, here is a method from OrderQuantityAdjustmentProductRepository class:

public void insert(OrderQuantityAdjustmentProduct adjustmentProduct) {
try {
orderQuantityAdjustmentProductMapper.insert(adjustmentProduct);
} catch (DuplicateKeyException duplicateKeyException) {
throw new DataException(“error.duplicate.product.code”);
} catch (DataIntegrityViolationException dataIntegrityViolationException) {
String errorMessage = dataIntegrityViolationException.getMessage().toLowerCase();
if (errorMessage.contains(“foreign key”) || errorMessage.contains(“violates not-null constraint”)) {
throw new DataException(“error.reference.data.missing”);
} else {
throw new DataException(“error.incorrect.length”);
}
}
}

It is arguable that DataException could also wrap the original exception so that it can provide better traceability. Therefore, it is questionable if we should loosen the rules regarding error handling.

  • There are a lot of warnings regarding “hard-coded password”, which are not real problems. We might need to disable this rule.

  • There are a lot of warnings regarding public static fields are not final. I personally think it is a good practice to always declare public static fields as final. Meanwhile, many of this cluster happens in test code. So again, we shall consider how we want to treat test code.

  • There are some other “Critical” tech debts noteworthy. I think we can deal with them once we have all these big clusters handled.
    Duplication

  • ODKSubmissionService class has many duplicated lines in its methods (saveProofOfDeliverySurveyData, saveZNZSurveyData, saveStockStatusSurveyData, saveODKSubmissionData).

  • There are other duplications found, for example in OrderFillRateQueryBuilder class.
    Complexity

  • The complexity indicator points to some big classes and long methods, such as ODKZNZSurveySubmissionSAXHandler. They could be potential targets of refactoring.

  • Packages with highest complexity are test packages (org.openlmis.pageobjects and org.openlmis.functional). Although quality of test code should be treated differently than product code, there are some big classes (over 1000 lines) should be considered taking too much responsibility.

Thoughts?

···

Jeff Xiong
ThoughtWorks
+86 186 826 53819