imsweb / mph

Java implementation of the Multiple Primary and Histology Coding Rules.
Other
4 stars 2 forks source link

Clean up Sonarqube warnings #92

Closed depryf closed 1 year ago

depryf commented 1 year ago

I enabled SonarQube on the project and it found some small issues.

But there is on one that is flagged as a "bug" (even if it isn't) and I would like to that one to be fixed.

The issue is in GroupUtility and creating dates with LocalDate.of() without using the resulting value. There are better ways to use the LocalDate class for validating and/or comparing dates.

For example, here is a different implementation of the "compareDxDate" method:

    public static int compareDxDate(MphInput input1, MphInput input2) {
        int year1 = NumberUtils.isDigits(input1.getDateOfDiagnosisYear()) ? Integer.parseInt(input1.getDateOfDiagnosisYear()) : 9999;
        int year2 = NumberUtils.isDigits(input2.getDateOfDiagnosisYear()) ? Integer.parseInt(input2.getDateOfDiagnosisYear()) : 9999;
        int month1 = NumberUtils.isDigits(input1.getDateOfDiagnosisMonth()) ? Integer.parseInt(input1.getDateOfDiagnosisMonth()) : 99;
        int month2 = NumberUtils.isDigits(input2.getDateOfDiagnosisMonth()) ? Integer.parseInt(input2.getDateOfDiagnosisMonth()) : 99;
        int day1 = NumberUtils.isDigits(input1.getDateOfDiagnosisDay()) ? Integer.parseInt(input1.getDateOfDiagnosisDay()) : 99;
        int day2 = NumberUtils.isDigits(input2.getDateOfDiagnosisDay()) ? Integer.parseInt(input2.getDateOfDiagnosisDay()) : 99;

        // if year is missing or in the future, return unknown
        int currYear = LocalDate.now().getYear();
        if (year1 == 9999 || year2 == 9999 || year1 > currYear || year2 > currYear)
            return MphConstants.COMPARE_DX_UNKNOWN;
        else if (year1 > year2)
            return MphConstants.COMPARE_DX_FIRST_LATEST;
        else if (year2 > year1)
            return MphConstants.COMPARE_DX_SECOND_LATEST;

        // if month is missing or invalid, return unknown
        if (month1 < 1 || month1 > 12 || month2 < 1 || month2 > 12)
            return MphConstants.COMPARE_DX_UNKNOWN;
        else if (month1 > month2)
            return MphConstants.COMPARE_DX_FIRST_LATEST;
        else if (month2 > month1)
            return MphConstants.COMPARE_DX_SECOND_LATEST;

        // if day is missing or invalid, return unknown
        if (day1 < 1 || day1 > LocalDate.of(year1, month1, 1).lengthOfMonth() || day2 < 1 || day2 > LocalDate.of(year2, month2, 1).lengthOfMonth())
            return MphConstants.COMPARE_DX_UNKNOWN;
        else if (day1 > day2)
            return MphConstants.COMPARE_DX_FIRST_LATEST;
        else if (day2 > day1)
            return MphConstants.COMPARE_DX_SECOND_LATEST;

        return MphConstants.COMPARE_DX_EQUAL;
    }

This implementation doesn't create date objects and doesn't rely on exceptions to check the validity of the parameters.

I think the same methods can be used in the other methods.

The other things that is flagged is that those first 6 lines of code are repeated in many methods. It would be better to create an inner class that encapsulates the three parts of the dates as "int" and take the input as a param. Or if every method needs two dates, then encapsulates the 6 parts into the class (year1, month1, day1, year2, month2, day2) and pass the two input objects to the constructor.