Uploaded image for project: 'OpenMRS Core'
  1. OpenMRS Core
  2. TRUNK-3652

Natural ordering of Patient State is not consistent with equals




      The compareTo for PatientState is defined to sort by date, so two different patient states with the same start and end date will be considered equal by the compareTo method (ie, compareTo will return 0).

      This led to TRUNK-3645 (though this bug has been fixed without addressing the compareTo issue).

      Ideally, we should change the natural ordering so that is it consistent with equals. One solution would be to add an additional comparison based on uuid if the dates of two patient states are equal. (Other suggestions of the best way to implement the natural ordering can be seen in the comments on TRUNK-3645.) HOWEVER, as the compareTo method was originally designed to do a date comparison, changing the implementation of the function may break other functionality, specifically the PatientState validator (take a look at how compareTo is used there).

      Therefore, I see a few potential solutions:

      1) Do nothing, live with the fact that the Patient State comparator is not consistent with equals

      2) Change the compareTo method so that it IS consistent with equals, but make sure we test (and correct as necessary) all usages of the compareTo method

      3) Remove the compareTo method entirely and create a new PatientStateByDateComparator and use this comparator instead throughout the code where comparisons are being made.

      I have a slight leaning toward 3, because of the fact that we should consider usages of compareTo outside of core that may break based on changes to the comparator. The thing I like about option 3 over option 2 is that while 2 will lead to a compilation errors, option 3 may lead to more insidious, unnoticed errors.

      It also appears that most of our domain objects don't have compareTo method.

      Some background from javadocs on Comparable:

      "This interface imposes a total ordering on the objects of each class that implements it. This ordering is referred to as the class's natural ordering, and the class's compareTo method is referred to as its natural comparison method.

      Lists (and arrays) of objects that implement this interface can be sorted automatically by Collections.sort (and Arrays.sort). Objects that implement this interface can be used as keys in a sorted map or elements in a sorted set, without the need to specify a comparator.

      The natural ordering for a class C is said to be consistent with equals if and only if (e1.compareTo((Object)e2) == 0) has the same boolean value as e1.equals((Object)e2) for every e1 and e2 of class C. Note that null is not an instance of any class, and e.compareTo(null) should throw a NullPointerException even though e.equals(null) returns false.

      It is strongly recommended (though not required) that natural orderings be consistent with equals. This is so because sorted sets (and sorted maps) without explicit comparators behave "strangely" when they are used with elements (or keys) whose natural ordering is inconsistent with equals. In particular, such a sorted set (or sorted map) violates the general contract for set (or map), which is defined in terms of the equals method."

        Gliffy Diagrams


            Issue Links



                k.joseph Kaweesi Joseph
                mogoodrich Mark Goodrich
                1 Vote for this issue
                6 Start watching this issue



                    Time Tracking

                    Original Estimate - 4 hours
                    Time Spent - 1 hour Remaining Estimate - 3 hours
                    Time Spent - 1 hour Remaining Estimate - 3 hours