spatial4j icon indicating copy to clipboard operation
spatial4j copied to clipboard

DistanceUtils is hell

Open chrismale opened this issue 13 years ago • 2 comments
trafficstars

It's a mix of badly formatted code, bad variable names, too many comments to be readable, calcBoxByDistFromPt_latHorizAxisDEG, Shapes are used but never Points (always lat, lon as separate doubles), some major methods aren't even used and it's impossible to tell what's been tested by TestDistances (which seems to double as both a test for DistanceUtils and for DistanceCalculators).

Ideally most of the distance methods would be moved to their Calculators. Any distance calculations directly used by multiple consumers should be re-evaluated to see if the consumers should be using Calculators instead.

chrismale avatar Sep 25 '12 09:09 chrismale

Some names are unfortunately long like the one you mentioned. I don't love underscores in methods but this method is tightly related to calcBoxByDistFromPt. The DEG is a convention I imposed for those methods that are degrees oriented, versus the ones that are radians oriented which generally have RAD. I'm glad I did the DEG, RAD convention as it was more confusing / error-prone before. Quite a few methods at the top of this class are inherited from Solr's spatial which has some alternative distance calculations. Maybe those should stay; maybe not.

I think I do like the idea of moving these methods to different DistanceCalculators, staying as static utility methods. Perhaps such a move would coincide with a potential re-look at the DistanceCalculator API.

dsmiley avatar Sep 25 '12 14:09 dsmiley

Some names are unfortunately long like the one you mentioned. I don't love underscores in methods but this method is tightly related to calcBoxByDistFromPt.

That's fine. I don't mind us having long names as the length isn't whats bugging me. I think it's just that in order to reduce the length, the names have become unreadable at a glance. I'd prefer users know exactly what they do than the names be short.

Quite a few methods at the top of this class are inherited from Solr's spatial which has some alternative distance calculations. Maybe those should stay; maybe not.

If there are some alternative distance measures then we should create DistanceCalculators for them. If thats not practical then we should probably send them back to Solr.

I think I do like the idea of moving these methods to different DistanceCalculators, staying as static utility methods.

I don't see a reason for them to stay as static utility methods. They aren't utility methods, they're the core behaviour of the Calculators. DistanceCalculators should be the entry point for the calculation of distances. They allow us to bundle similar behaviour together, provide detailed documentation, and free us to make some underlying changes.

Perhaps such a move would coincide with a potential re-look at the DistanceCalculator API.

You haven't elaborated on what that means and really it's unrelated to this issue. Once we've broken DistanceUtils up into the appropriate Calculators, then we can take a look at the Calculators themselves.

chrismale avatar Sep 25 '12 23:09 chrismale