On 5/27/07, Rony G. Flatscher <Rony.Flatscher@wu-wien.ac.at> wrote:
Hi there,

while looking at the possibilities that sort allows for I stumbled over a few observations which I think are omissions. Not being sure I rather list them here, but would enter them as bugs (please note that at the moment I cannot create ooRexx, hence I do not want to check in anything that I cannot test):
  • "CaselessColumnComparator", "ColumnComparator" and "InvertingComparator" should mixinclass "Comparator" instead of "Object",

These are not really intended as mixin classes, nor do I see them as particularly useful as mixin classes.  The sorting framework does not require that the comparator object be an implementation of the comparator class, only that it implement the compare method. 

  • the "Descending*Comparator" classes *could* be removed, as using "InvertingComparator" would allow for that; if you want to keep them (perhaps making it easier for the programmers to sort descendingly), then I would suggest that "DescendingCaselessComparator" is mixinclassing "DescendingComparator" (to match "CaselessComparator" which mixinclasses "Comparator"),
Yes, it could be removed, however, it serves as a useful convenience class when you're just relying on the objects compareTo method to give the ordering.  This is handy to have, and I don't really see any justification for removing it.

  • in the "*ColumnComparator" I noticed that the "length" argument is mandatory: shouldn't that remain optional, in which case the remainder of the string is taken for comparison purposes? Also, while at it, how about enhancing it such that multiple columns could/can be compared?
No, it cannot.  The ColumnComparator should be symmetric in it's results regardless of the order of the arguments.  Defaulting the length to the length of one or the other argument can result in inconsistent results if the strings being compared happen to be of different lengths.  The length needs to be specified.

So this would probably mean that:
  • class "ColumnComparator":
    • method "compare" is missing
Hmmm, I wonder how that happened...it was certainly working correctly at the symposium.  Please open a bug report.
    • should mixinclass "Comparator"
I disagree.


  • class "CaselessColumnComparator":
    • should mixinclass "Comparator"
I disagree.

  • class "InvertingComparator":
    • in method "compare": "USE STRICT ARG left, rightis missing
Where's the bug report.


    • should mixinclass "Comparator"

I disagree.

To be able to fully use the classification tree for classifying purposes, the classes "Comparator" and "DescendingComparator" should both have either the same parent, or "DescendingComparator" should specialize "Comparator" or both classes should be marked with a class that indicates "comparator" functionality. (An alternative could be to use a package concept, where one would be able to classify by putting classes belonging together into the same package; but then one would need an ability to check whether a class got defined to belong to a certain package.)


I'm not convinced this is a useful classification for these.  There is not requirement that any comparator object implement either of these....the object just needs to implement the compare method to function.




This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
Oorexx-devel mailing list