Danish map links (location.xx.inc location.dk.inc)

mikl - March 24, 2009 - 13:56
Project:Location
Version:6.x-3.x-dev
Component:Data update
Category:task
Priority:normal
Assigned:mikl
Status:needs review
Description

I've expanded location.dk.inc with map links to the Danish map site, findvej.dk.

Patch attached :)

AttachmentSize
dk.location.patch3.23 KB

#1

bdragon - March 24, 2009 - 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.)

#2

mikl - March 25, 2009 - 11:27

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.
AttachmentSize
location.dk_.inc_.patch 3.2 KB

#3

YesCT - April 17, 2009 - 14:11
Title:Danish map links» Danish map links (location.xx.inc location.dk.inc)

Can someone try this new patch?

#4

mikl - June 1, 2009 - 13:11

I've made a new patch with support for the Danish public transportation guide, Rejseplanen.dk :)

AttachmentSize
danish-location.patch 5.63 KB

#5

kaerast - June 25, 2009 - 11:44

This latest patch isn't applying for me for some reason. I therefore attach my own version which I rolled based upon yours.

AttachmentSize
location.dk_.inc_.diff 4.16 KB
 
 

Drupal is a registered trademark of Dries Buytaert.