How can I improve this comparator?

Question

I have several Comparator - one for Date s, one for decimal places, one for percent, etc.

At first, my decimal comparator looked like this:

 class NumericComparator implements Comparator<String> { @Override public int compare(String s1, String s2) { final Double i1 = Double.parseDouble(s1); final Double i2 = Double.parseDouble(s2); return i1.compareTo(i2); } } 

Life was simple. Of course, this does not apply to the case when strings are not processed. So I improved compare() :

 class NumericComparator implements Comparator<String> { @Override public int compare(String s1, String s2) { final Double i1; final Double i2; try { i1 = Double.parseDouble(s1); } catch (NumberFormatException e) { try { i2 = Double.parseDouble(s2); } catch (NumberFormatException e2) { return 0; } return -1; } try { i2 = Double.parseDouble(s2); } catch (NumberFormatException e) { return 1; } return i1.compareTo(i2); } } 

Life was better. The tests have become harder. However, my code browser noted: "What about null s?"

Ok, now I have to repeat the above with a NullPointerException or add the method body with:

 if (s1 == null) { if (s2 == null) { return 0; } else { return -1; } } else if (s2 == null) { return 1; } 

This method is huge. Worst of all, I need to repeat this pattern with three other classes that compare different types of strings and can raise three other exceptions when parsing.

I am not a Java expert. Is there a cleaner, tidier solution than - sigh - copy and paste? Do I have to trade correctness because of the lack of complexity, if it is documented?

---

Update:. Some have suggested that this is not a Comparator job to handle null values. Since the sort results are displayed to users, I want zeros to be sorted sequentially.

Answer 1

Here is how I would improve the comparator:

First, an exctract method for converting a value. This is repeated, several attempts ... catches are always ugly → it is better to have as little as possible.

 private Double getDouble(String number) { try { return Double.parseDouble(number); } catch(NumberFormatException e) { return null; } } 

Then write down simple rules to show how you want the comparator stream to be.

 if i1==null && i2!=null return -1 if i1==null && i2==null return 0 if i1!=null && i2==null return 1 if i1!=null && i2!=null return comparison 

Finally, do a terrible mess to the actual comparator in order to raise a few WTF: s in the code overview (or, like others, say “Implement Comparator”):

 class NumericComparator implements Comparator<String> { public int compare(String s1, String s2) { final Double i1 = getDouble(s1); final Double i2 = getDouble(s2); return (i1 == null) ? (i2 == null) ? 0 : -1 : (i2 == null) ? 1 : i1.compareTo(i2); } private Double getDouble(String number) { try { return Double.parseDouble(number); } catch(NumberFormatException e) { return null; } } } 

... yes, that branching nested triple. If someone complains about this, say what others have said here: Handling zeros is not a comparator job.

Answer 2

You are implementing Comparator<String> . String , including compareTo throw a NullPointerException if zero is passed to it, so you should too. Similarly, Comparator throws a ClassCastException if the argument types do not allow them to be compared. I would recommend you implement these inherited behaviors.

 class NumericComparator implements Comparator<String> { public int compare(String s1, String s2) { final Double i1; final Double i2; if(s1 == null) { throw new NullPointerException("s1 is null"); // String behavior } try { i1 = Double.parseDouble(s1) } catch (NumberFormatException e) { throw new ClassCastException("s1 incorrect format"); // Comparator behavior } if(s2 == null) { throw new NullPointerException("s2 is null"); // String behavior } try { i2 = Double.parseDouble(s1) } catch (NumberFormatException e) { throw new ClassCastException("s2 incorrect format"); // Comparator behavior } return i1.compareTo(i2); } } 

You can almost restore the original elegance of the extraction method for type checking and conversion.

 class NumericComparator implements Comparator<String> { public int compare(String s1, String s2) { final Double i1; final Double i2; i1 = parseStringAsDouble(s1, "s1"); i2 = parseStringAsDouble(s2, "s2"); return i1.compareTo(i2); } private double parseStringAsDouble(String s, String name) { Double i; if(s == null) { throw new NullPointerException(name + " is null"); // String behavior } try { i = Double.parseDouble(s1) } catch (NumberFormatException e) { throw new ClassCastException(name + " incorrect format"); // Comparator behavior } return i; } } 

If you are not particularly concerned with exception messages, you may lose the "name" parameter. I am sure that you can lose the extra line here or the word there by applying small tricks.

You say you need to repeat this pattern with three other classes which compare different types of strings and could raise three other exceptions . It is difficult to offer specifics there without seeing the situation, but you can use the "Pull Up Method" in the version of my parseStringAsDouble to the common ancestor of NumericComparator , which itself implements java Comparator .

Answer 3

There are many subjective answers to this question. Here are my own $ .02.

First, the problem you are describing is a canonical symptom of a language that lacks first-class features that allow you to briefly describe these patterns.

Secondly, in my opinion, it should be a mistake to compare two lines as doubled if one of them cannot be considered a double representation. (The same is true for zeros, etc.). Therefore, you must allow the distribution of exceptions! This will be a controversial opinion, I expect.

Answer 4

You can create a utility method that processes the parsing and returns a specific value in case of nulls or parsing exceptions.

Answer 5

Take a step back. Where do these Strings come from? What is Comparator used for? Do you have a Collection of Strings that you want to sort or so?

Answer 6

tl; dr: Take the manual from the JDK. The double comparator is not defined for either number or zeros. Invite people to give you useful data (Doubles, Dates, Dinosaurs, whatever) and write your own comparators for this.

As far as I can tell, this is an example of user input validation. For example, if you enter input from a dialog box, the right place is to make sure that you have a syntax string that is Double, Date, or something else in the input handler. Make sure it’s good, before the user can leave, click “Good” or equivalent.

This is why I think so:

First question: if the strings are not treated as numbers, I think you are trying to solve the problem in the wrong place. Say, for example, I'm trying to compare "1.0" with "Two" . The second, obviously, cannot be analyzed, like a double, but is it smaller than the first? Or is it more. I would like to argue that users will need to turn their strings into pairs before they ask for more (which you can easily answer using Double.compareTo, for example).

Second question: if the lines are "1.0" and null , what is more? The JDK source does not handle NullPointerExceptions in the comparator: if you give it zero, autoboxing will fail.

Worst part - I need to repeat this pattern with three other classes that compare different types and can catch three more exceptions when parsing.

That is why I would like to argue that parsing must occur outside of your Comparator with exception handling before it reaches your code.

Answer 7

Try the following:

 import com.google.common.base.Function; import com.google.common.collect.Ordering; Ordering.nullsFirst().onResultOf( new Function<String, Double>() { public Double apply(String s) { try { return Double.parseDouble(s); } catch (NumberFormatException e) { return null; } }) 

The only problem if you think that this is that zero lines and other non-parallel lines will all be mixed. This probably doesn't matter much considering the advantages - it gives you a comparator that is guaranteed to be correct, while using a hand-held comparator, even relatively simple ones, it is amazing how easy it is to make a subtle error that breaks transitivity or umm, antisymmetry.

http://google-collections.googlecode.com

Answer 8

There seem to be two problems here, and they can be broken down into separate components. Consider the following:

 public class ParsingComparator implements Comparator<String> { private Parser parser; public int compare(String s1, String s2) { Object c1 = parser.parse(s1); Object c2 = parser.parse(s2); new CompareToBuilder().append(c1, c2).toComparison(); } } 

The Parser interface will have implementations for numbers, dates, etc. You can potentially use the java.text.Format class for your Parser interface. If you don't want to use commons-lang, you can replace using CompareToBuilder with some logic to handle NULL values ​​and use Comparable instead of Object for c1 and c2.

Answer 9

If you can change the signature, I suggest you write a method so that it can accept any supported object.

  public int compare(Object o1, Object o2) throws ClassNotFoundException { String[] supportedClasses = {"String", "Double", "Integer"}; String j = "java.lang."; for(String s : supportedClasses){ if(Class.forName(j+s).isInstance(o1) && Class.forName(j+s).isInstance(o1)){ // compare apples to apples return ((Comparable)o1).compareTo((Comparable)o2); } } throw new ClassNotFoundException("Not a supported Class"); } 

You can even define it recursively when you discard your strings in paired numbers, and then return the result of calling yourself with these objects.

Answer 10

IMHO, you must first create a method that returns Double from String, embedding cases of failure with a null error and parsing (but you must determine what to do in such cases: throw an exception? Return default value?).

Then your comparator just has to compare the resulting duplicate instances.

In other words, refactoring ...

But I'm still wondering why you need to compare strings, expecting them to be paired. I mean, what prevents you from manipulating doubles in code that would actually use this comparator?

Answer 11

according to your needs and Ewan's post, I think there is a way to extract a structure that you can reuse:

 class NumericComparator implements Comparator<String> { private SafeAdaptor<Double> doubleAdaptor = new SafeAdaptor<Double>(){ public Double parse(String s) { return Double.parseDouble(s); } }; public int compare(String s1, String s2) { final Double i1 =doubleAdaptor.getValue(s1, "s1"); final Double i2 = doubleAdaptor.getValue(s2, "s2"); return i1.compareTo(i2); } } abstract class SafeAdaptor<T>{ public abstract T parse(String s); public T getValue(String str, String name) { T i; if (str == null) { throw new NullPointerException(name + " is null"); // String } try { i = parse(str); } catch (NumberFormatException e) { throw new ClassCastException(name + " incorrect format"); // Comparator } return i; } } 

I am retrieving a method as an abstract class that can be reused in other cases (although the class name is sucking).

greetings.

Answer 12

So I improved compare () ...

sure you did it.

firstly, the Comparator interface does not indicate what happens to zeros. if your null check, if statement works for your use case, this is great, but the general solution throws npe.

how cleaner ... why final? why all the traps / throws? why use compareTo for a primitive shell?

 class NumericComparator implements Comparator<String> { public int compare(String s1, String s2) throws NullPointerException, NumberFormatException { double test = Double.parseDouble(s1) - Double.parseDouble(s2); int retVal = 0; if (test < 0) retVal = -1; else if (test > 0) retVal = 1; return retVal; } } 

It seems like it might be easier for you to clarify the test of renaming t1 and retVal to q.

to repeat the pattern ... a. you could use generics with reflection to call the appropriate parseX methods. it didn't seem worth it.