views:

296

answers:

7

Hi,

We have some code wich sorts a list of addresses based on the distance between their coordinates. this is done through collections.sort with a custom comparator.

However from time to time an adress without coordinates is in the list causing a NullPointerException. My initial idea to fix this was to have the comparator return 0 as dististance for addresses where at least one of the coordinates is null. I fear this might lead to corruption of the order the 'valid' elements in the list.

so is returning a '0' values for null data in a comparator ok, or is there a cleaner way to resolve this.

A: 

No, there is no cleaner way. Perhaps:

  • if the coordinates of both compared objects are null, return 0
  • if the coordinates of one of the objects are null, return -1 / 1 (depending on whether it's the first or the second argument)

But more importantly - try to get rid of / fill in the missing coordinates, or, better: don't put addresses with missing coordinates in the list.

Actually, not putting them in the list is the most logical behaviour. If you put them in the list, the result won't be actually ordered by distance.

You may create another list, containing the addresses with missing coordinates, and make it clear to whoever needs that information (end-user, API user), that the first list only contains addresses with the needed data, while the second list contains addresses that lack required information.

Bozho
A method can't return -1/1 or true
matt b
@matt b of course. I meant 0 (equal)
Bozho
+6  A: 

Handle it like null means infinitely far away. Thus comp(1234, null) == -1, comp(null, null) == 0, comp(null, 1234) == 1. With this, you get a consistent ordering.

Sjoerd
... or comp(null,null)==-1, dependant on whether you want two addressless entries to show up as being in the same place or not.
izb
@izb, that violates the Comparator requirement of reflexivity.
Kevin Bourrillion
+2  A: 

You probably dont want to return 0 as that implies the addresses are equidistant and you really dont know. This is quite a classic problem where you are trying to deal with bad input data. I dont think its the responsibility of the comparator to try and determin how far the address is in realtive terms when you dont know the distance. I would remove those addresses from the list before sorting.

The hack would be to move them to the bottom of the list (but thats ugly !)

LenW
+1  A: 

Instead of looking at this like it's a technical problem with the comparator, it's better to take a look at the requirements again: what you're really trying to do here, what are you going to do with this sorted list?

  • If you are trying to sort them to show the most relevant solutions first to a user, it might be a good idea to put the unknown locations last, so treat it like infinity (returning 0/-1/1 depending on which of them is null).
  • If you are going to use this result to draw some graph or do some other calculations that depend on them really being sorted by their distance, then the nulls probably shouldn't have been in there anyways (so either remove them first, or throw an exception if at that point there actually weren't supposed to be any addresses with a null location).

As you already realize, always returning 0 when one of them is null is not a good idea here; it can corrupt the result indeed. But what you should do instead depends on what you need, not on what others usually do/need. How your program behaves with addresses that don't have a location (so what the user is going to see) should not depend on some technical detail like what the "best practice" for comparators is. (To me, asking what the "best practice" is here, sounds like asking what the "best requirements" are).

Wouter Coekaerts
+2  A: 

My take on this is that anything you try to "make good" the null coordinates is just papering over the cracks. What you really need to do is find and fix the bugs that are injecting the spurious null coordinates.

In my experience, infestations of NPE bugs are frequently caused by the following bad coding habits:

  • inadequate validation of input parameters,
  • using null to avoid creating empty arrays or collections, or
  • returning null when an exception should have been thrown.

If this describes your application, you should be spending time to root out the code problems rather than thinking of ways to hide the NPEs.

Stephen C
Unless a null coordinate is a valid value.
Steve Kuo
+1  A: 

I personally hate dealing with special null cases everywhere in my comparators so i was looking for a cleaner soluation and finally found google collections. Their Ordering is just awesome. They support compound comparators, offer to sort nulls to the top and to the end and allow to run certain functions before comparing. Writing comparators has never been so easy. You should give it a try.

Willi
+2  A: 

Just to expand on Willi Schönborn's answer, I came here to say that google-collections is exactly what you're after here.

In the general case, you can just write your own Comparator to ignore nulls (assume non-null, so it can concentrate on the important logic), and then use Ordering to handle the nulls:

Collections.sort(addresses, Ordering.from(new AddressComparator()).nullsLast());

In your case, though, it's data WITHIN the Address (the Coordinates) that is being used to sort, right? google-collections is even more useful in this case. So you might have something more like:

// Seems verbose at first glance, but you'll probably find yourself reusing 
// this a lot and it will pay off quickly.
private static final Function<Address, Coordinates> ADDRESS_TO_COORDINATES = 
  new Function<Address, Coordinates>() {
      public Coordinates apply(Address in) {
          return in.getCoordinates();
      }
  };

private static final Comparator<Coordinates> COORDINATE_SORTER = .... // existing

then when you want to sort:

Collections.sort(addresses,
    Ordering.from(COORDINATE_SORTER)
            .nullsLast()
            .onResultOf(ADDRESS_TO_COORDINATES));

and that's where the power of google-collections really starts to pay off.

Cowan
Note you could have made COORDINATE_SORTED extend Ordering in the first place and skipped the Ordering.from().
Kevin Bourrillion