Opened 12 years ago

Closed 12 years ago

#145 closed enhancement (fixed)

Update Parameters to properly implement compareTo and equals

Reported by: Peter Powers Owned by: Peter Powers
Priority: major Milestone: OpenSHA 1.2
Component: sha Version:
Keywords: Cc:


It appears that the initial compareTo implementations were done to support equals and often only return 0 or -1. There are situations in which one would want to sort groups of parameters for presentation. Need to implement compareTo such that sorting is by parameter name, then value, with null params, names, or values always returning -1 and sinking in any list (unless both two params both have null values). This should be done using the generics Comparable<T> interface. For lists of with type ParameterAPI<?>, compareTo in a specific Param will throw ClassCastExceptions? so these need to be caught and passed up to abstract parents that will compare on name only.

Change History (8)

comment:1 Changed 12 years ago by Peter Powers

First pass at implementing generic Comparable. Currently maintains previous evaluation but needs further work. Some equals implementations adjusted for compactness.

in [7517] and [7518]

Last edited 12 years ago by Peter Powers (previous) (diff)

comment:2 Changed 12 years ago by Peter Powers

Unit test errors have provided insight on this issue. (Note: the Parameter class name makes talking about generic parameterization a bit confusing)

Equivalency via equals(obj) for parameters is now implemented as follows in AbstractParameter?:

  • compare object
  • check if Parameter
  • check specific Parameter implementation class
  • check name
  • check value, handling null

This should suffice for all implementations, however, any class that is used as a generic parameterization E in Parameter<E> should have a robust implementation of equals to ensure proper evaluation of value.equals(p.getValue()) in AbstractParameter?.equals(obj).

As stated originally, equals should NOT defer to compareTo(Parameter<E>).

In the case of compareTo(), AbstractParameter? provides a comparison on name. This, too should suffice for most if not all comparison situations. In the event that a Collection<Parameter> or ParameterList? needs sorting, such sorting will likely be done for presentation purposes only where name will be the important key. If additional sorting on value is required, it could be implemented by a subclass, but would need to defer to the parent implementation in the event of a ClassCastException? (this could occur when sorting a List of unknown parameterized type e.g. List<Parameter<?>>)

To properly compare values in AbstractParameter?, the Parameter<E> Type E would have to be <E extends Comparable<E>> or better yet <E extends Comparable<? super E>> (to handle child classes). This in turn requires that all possible parameterization types implement Comparable (e.g. LocationLists?, Regions etc...). It makes no sense to go to such lengths at this time.

Note that for Parameters compareTo is not consistent with equals as suggested in the documentation of the Comparable<E> interface. This is ok, however, Parameters should NOT be used as keys in any collections that relies on hashing or other comparisons to check or sort contents (e.g. Set, Maps etc...)

Last edited 12 years ago by Peter Powers (previous) (diff)

comment:3 Changed 12 years ago by Peter Powers

Moving beyond the Parameters themselves, compareTo should not be used in any equivalency test for objects that wrap Parameters. As an example, Site currently uses param.compareTo(param) to check if the values of internal site params are the same. Should use param.equals(param) instead.

comment:4 Changed 12 years ago by Peter Powers

Site equals(obj) rewritten in [7933]

comment:5 Changed 12 years ago by Peter Powers

Changes to equals and compareTo for all commons params in [7934]

No parameter implementation handles equals or compareTo; all work is done by AbstractParameter?.

comment:6 Changed 12 years ago by Kevin Milner

is this ready to be closed? any unit tests for the equals method?

comment:7 Changed 12 years ago by Peter Powers

Tests for equals complete in [7972] and [7974].

Tests for compareTo revealed that cruisecontrol jvm doesn't like runtime type ambiguity when sorting a List<Parameter<?>>. Although we're updating the cc JRE, I also changed Parameter to implement Comparable<?> instead of Comparable<T>. Seems ok for now; however, any Parameter implementations that override compareTo(param) may have to cast the param.value to the runtime type. Changes made in [7979].

comment:8 Changed 12 years ago by Peter Powers

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.