5
\$\begingroup\$

I implemented the Haversine formula to calculate the distance between two (latitude, longitude) coordinates.

I was wondering if it looks natural to Clojure programmers. What could be improved?

(ns br.com.reactive-poc.calc)

(def raio 6372.795477598)

(defn calculate-distance
"Calcula distância entre duas coordenadas geográficas"
([source to]
  {:pre [every? (every-pred map? #(contains? % :lgt) #(contains? % :ltd)) [source to]]}
  (let [delta-latitude (Math/toRadians (- (:source/ltd source) (:to/ltd to)))
        delta-longitude (Math/toRadians (- (:source/lgt source) (:to/lgt to)))
        ]
    (* (* raio (* 2 (Math/atan2 (Math/sqrt (+ (Math/pow (Math/sin (/ delta-latitude 2)) 2) 
                                              (* (Math/cos (Math/toRadians (:source/ltd source))) (Math/cos (Math/toRadians (:to/ltd to))) (Math/sin (/ delta-longitude 2)) (Math/sin (/ delta-longitude 2))))) 
                                (Math/sqrt (- 1 (+ (Math/pow (Math/sin (/ delta-latitude 2)) 2) 
                                                   (* (Math/cos (Math/toRadians (:source/ltd source))) (Math/cos (Math/toRadians (:to/ltd to)) ) (Math/sin (/ delta-longitude 2)) (Math/sin (/ delta-longitude 2))))))))) 1000)
    )
  )
)
\$\endgroup\$
1
  • \$\begingroup\$ Can you show us an example? The pre-condition seems to be at odds with the way the data is destructured. \$\endgroup\$ Commented May 13, 2017 at 7:40

3 Answers 3

5
\$\begingroup\$

Naming and interface

Please avoid mixing English (calculate-distance) with Portuguese (raio). The code should be in English; write your comments in whatever language you prefer.

ltd and lgt seem like awkward abbreviations; lat and lon or lat and lng seem more conventional. The latter is what Google uses for the Maps and Android APIs, for example.

Putting "calculate" in the name of a function seems redundant, especially in a functional programming language.

One of the most surprising behaviours is that your function gives the result in meters, even though your raio constant is in kilometers. To discover that quirk, you would have to scroll all the way to column 219 at the end of the function, at the end of a very long line of code! A better comment on the function would help; better naming would be beneficial as well.

Implementation

As I mentioned, those are very long lines of code — almost unreadable. It would be more readable if you broke down the calculation into subexpressions.

In fact, (+ (Math/pow (Math/sin (/ delta-latitude 2)) 2) (* (Math/cos (Math/toRadians (:source/ltd source))) (Math/cos (Math/toRadians (:to/ltd to))) (Math/sin (/ delta-longitude 2)) (Math/sin (/ delta-longitude 2)))) is a common subexpression that you evaluate twice. So, not only is your code unreadable, it's also inefficient.

I would convert the latitudes and longitudes to radians right away, since degrees are useless for trigonometry. Half the difference in angles are easily recognizable quantities. Defining the \$\sin^2(\theta)\$ function as a lambda is also useful.

(def earth-radius-km 6372.795477598)

(defn distance
    "Calcula distância entre duas coordenadas geográficas em metros"
    ([latlng1 latlng2]
        {:pre [every? (every-pred map? #(contains? % :lat) #(contains? % :lng)) [latlng1 latlng2]]}
        (let [lat1 (Math/toRadians (:latlng1/lat latlng1))
              lng1 (Math/toRadians (:latlng1/lng lnglng1))
              lat2 (Math/toRadians (:latlng2/lat latlng2))
              lng2 (Math/toRadians (:latlng2/lng lnglng2))
              half-dlat (/ (- lat2 lat1) 2)
              half-dlng (/ (- lng2 lng1) 2)
              sin2 (fn [x] (Math/pow (Math/sin x) 2))
              a (+ (sin2 half-dlat) (* (Math/cos lat1) (Math/cos lat2) (sin2 half-dlng)))]
             (* 2000 earth-radius-km (Math/atan2 (Math/sqrt a) (Math/sqrt (- 1 a)))))))
\$\endgroup\$
0
2
\$\begingroup\$

I happen to have written a distance calculator in Clojure so I am going to show you my code. I'm sorry that it doesn't have answers to all your questions, but I will try to give you some comments, as well. I haven't used spec, so I can't say anything about it.

(ns mic-project.distance-calculator)

(defn ^:private degree->radian [angle]
  (/ angle 57.2958))

(defn ^:private squared-sine [angle]
  (let [sine (Math/sin angle)]
    (* sine sine)))

(defn ^:private squared-cosine [angle]
  (let [cosine (Math/cos angle)]
    (* cosine cosine)))

(defn distance-km [{:keys [lat-1 lng-1 lat-2 lng-2]}]
  (let [lat-1-rad (degree->radian (Double. lat-1))
        lng-1-rad (degree->radian (Double. lng-1))
        lat-2-rad (degree->radian (Double. lat-2))
        lng-2-rad (degree->radian (Double. lng-2))
        first-squared-sine (squared-sine (/ (- lat-2-rad lat-1-rad) 2))
        second-squared-sine (squared-sine (/ (- lng-2-rad lng-1-rad) 2))
        cosines-product (* (Math/cos lat-1-rad)
                           (Math/cos lat-2-rad))
        square-rooted-sum (Math/sqrt (+ first-squared-sine
                                        (* cosines-product second-squared-sine)))
        earth-radius-km 6371
        haversine-result (* 2 earth-radius-km (Math/asin square-rooted-sum))]
    haversine-result))

(defn distance-miles [coordinates]
  (* 0.621371 (distance-km coordinates)))

It is unusual to use def inside a function definition. You can let raio.

I think it's also uncommon to use let for defining functions like delta-latitude and delta-longitude. I think it's much better to defn those functions outside of calculate-distance. To make it more obvious that these are helper functions of calculate-distance, you could make them private.

I suggest you stick to the Clojure Style Guide when you name your vars, so instead of lgtSource, use lgt-source.

The Clojure Style Guide suggests we use -> instead of to in function names. For example, instead of convert-to-rad, I have used the name degree->radian.

To make sure you are calling the function with the right type of argument, you can use preconditions. Here's one way to do it. I haven't tested the code below.

(defn- valid-longitude? [arg]
  (and
   (number? arg)
   (<= -180 arg 180))

(defn- valid-latitude? [arg]
  (and
   (number? arg)
   (<= -90 arg 90))

and then do something like

(defn calculate-distance [{:keys [lat-1 lng-1 lat-2 lng-2]}]
  {:pre [(valid-latitude? lat-1)
         (valid-longitude? lng-1)
         (valid-latitude? lat-2)
         (valid-longitude? lat-2)]}
...
...
)

To understand [{:keys [lat-1 lng-1 lat-2 lng-2]}], google Clojure function argument destructuring.

The function would then be called like this:

(calculate-distance {:lat-1 22.664823
                     :lng-1 11.161030
                     :lat-2 32.208499
                     :lng-2 10.741567})

I hope this helps.

\$\endgroup\$
1
\$\begingroup\$

The pre-condition

  • is inoperative
  • corrected, is inconsistent with the data shape the function requires.

Let's cut down the function so that it destructures its arguments:

(defn calculate-distance
  "Calcula distância entre duas coordenadas geográficas"
  [source to]
  {:pre [every? (every-pred map? #(contains? % :lgt) #(contains? % :ltd)) [source to]]}
  (let [delta-latitude (Math/toRadians (- (:source/ltd source) (:to/ltd to)))
        delta-longitude (Math/toRadians (- (:source/lgt source) (:to/lgt to)))]
    [delta-latitude delta-longitude]))

(I've dispensed with the harmless but redundant parentheses wrapping the single arity).

The pre-condition is

{:pre [every? (every-pred map? #(contains? % :lgt) #(contains? % :ltd)) [source to]]}

This evaluates, in turn,

  • every?
  • (every-pred map? #(contains? % :lgt) #(contains? % :ltd))
  • [source to]

... all of which are logically true - two functions and a vector.

You need to wrap the whole expression in ( ... ) to make it evaluate:

{:pre [(every? (every-pred map? #(contains? % :lgt) #(contains? % :ltd)) [source to])]}

Before we do that, let's look at the data shape the function requires. For example,

(calculate-distance {:source/ltd 0, :source/lgt 0}
                    {:to/ltd 0, :to/lgt 0})
=> [0.0 0.0]

The expressions in the let use qualified keywords: :source/ltd ... .

This is bad. We want both points specified with the same shape - the more so since they are symmetrical in effect: the distance from here to there is the same as the distance from there to here.

Let's go with the shape that the (corrected) pre-condition requires:

(defn calculate-distance
  "Calcula distância entre duas coordenadas geográficas"
  [source to]
  {:pre [(every? (every-pred map? #(contains? % :lgt) #(contains? % :ltd)) [source to])]}
  (let [delta-latitude (Math/toRadians (- (:ltd source) (:ltd to)))
        delta-longitude (Math/toRadians (- (:lgt source) (:lgt to)))]
    [delta-latitude delta-longitude]))

Now

(calculate-distance {:source/ltd 0, :source/lgt 0}
                    {:to/ltd 0, :to/lgt 0})
AssertionError ...

but

(calculate-distance {:ltd 0, :lgt 0}
                    {:ltd 0, :lgt 0})
=> [0.0 0.0]

So far, so good.

Let's do a couple of quick examples:

(calculate-distance {:ltd 0, :lgt 0}
                    {:ltd 0, :lgt 0})
=> 0.0

Good.

(calculate-distance {:ltd 0, :lgt 0}
                    {:ltd 0, :lgt 90})
=> 1.0010363727626067E7

The distance from the North pole to the equator along the Greenwich meridian is about 10 million metres or 10 thousand kilometres - correct.


As for the calculation, I've nothing to add to the other answers. Though I would prefer in a JVM environment to do as you do and call the Mathfunctions directly.

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.