Danish map links (location.xx.inc location.dk.inc) [#412112]

Comment	File	Size	Author
#5	location.dk_.inc_.diff	4.16 KB	kaerast
#4	danish-location.patch	5.63 KB	mikl
#2	location.dk_.inc_.patch	3.2 KB	mikl
	dk.location.patch	3.23 KB	mikl

Comment #1

bdragon commented 24 March 2009 at 17:45

Some nits:

* Don't use sprintf. Concatenation is fine. Using url() to do the heavy lifting is best.
* findvej.dk seems to be fine with using just a postcode, there should be a case for that, no?
* I'd prefer lat/lon over the rest of the choices, rather than the other way around. This makes it easier on the geocoder.
* You should use location_has_coordinates() to check whether the location has latitude / longitude set. This catches corner cases like stuff set to "0.0000" and so forth.
* You're checking if the fields are set, but you aren't checking if they are empty or not when determining the link.
* The raw ampersand when you're putting together the lat/lon url is suspect. In HTML this must be changed to the & entity.
* You accidentally joined the cvs id line with the top line of the file. (I automatically fix stuff like this when testing patches, so it's not actually a problem as such.)

Log in or register to post comments

Comment #2

mikl

Møn

commented 25 March 2009 at 11:27

Status	File	Size
new	location.dk_.inc_.patch	3.2 KB

Okay, re-roll:

Okay, sprintf might be overkill here, even if it makes the code a bit cleaner…
While postcodes are supported, Google's geocoding of postcodes in Denmark is a bit clueless and provides unexpected results. See for example my old hometown, Snedsted: http://findvej.dk/7752 – the city marker is placed well outside the city, half a mile away from the city center.
While using coordinates are cleaner and saves a geocoding-lookup up front, findvej will then not have the address available, so it won't be displayed on the map, and if you use any of findvej's features, it'll get the address via a reverse geocode lookup, so we won't be saving many calls and providing a poorer user experience.
I stand corrected, now using location_has_coordinates().
Corrected field checking as well.
Yeah, I've converted the code to use url()
I've changed it back to the old style.