3
\$\begingroup\$

I'm pretty excited to have coded my first JavaScript file which now works, and I've spent my evening trying to improve it.

It takes a location in the #loc input box and geocodes it (gets coordinates from a location) when the search button is pressed, or when the autosuggestion is clicked, then submits the form.

I've worked hard on this but I am really interested to see if any JavaScript gurus can spot any mistakes, because I love beautiful code.

jsFiddle

var coded = false;
geocode();
$.cookie("country", "uk");

// GEOCODE FUNCTION
function geocode() {
        var input = $('#loc')[0];
        var options = {types: ['geocode']};
        var country_code = $.cookie('country');

        if (country_code) {
                options.componentRestrictions = {
                        'country': country_code
                };
        }

        var autocomplete = new google.maps.places.Autocomplete(input, options);

        google.maps.event.addListener(autocomplete, 'place_changed', function() {
                processLocation();
        });

        $('#searchform').on('submit', function(e) {
                if (coded === false) {
                        processLocation();
                }
                return true;
        });

        $("#loc").bind("change paste keyup", function() {
                coded = false;
        });
}

function processLocation() {
        var geocoder = new google.maps.Geocoder();
        var address = $('#loc').val();
        geocoder.geocode({
                'address': address
        },
        function(results, status) {
                if (status === google.maps.GeocoderStatus.OK) {
                        coded = true;
                        $('#lat').val(results[0].geometry.location.lat());
                        $('#lng').val(results[0].geometry.location.lng());
                } else {
                        coded = false;
                        alert("Sorry - We couldn't find this location. Please try an alternative");
                }
        });
        coded = true;
}
\$\endgroup\$
3
\$\begingroup\$

Looking at your code, the first thing that stands out is the coded variable. You're checking or setting its value in many different places, which is the sort of thing you want to avoid, as it can quickly lead to errors. You're manually keeping track of the state.

A simpler, more robust solution would be to store the last searched-for location in the processLocation function, and if it's asked twice or more for that same location, give the same result as the first time. That way, the logic is all in one place.

The second thing I notice is that there's some duplication going on. For instance, getting $("#loc") several times, and in different functions. Another thing you'll want to avoid. It's the DRY principle: Don't repeat yourself.

Lastly, I'm afraid there's an outright bug. You use $.cookie to set the country to "uk", and that'll happen every time the page loads. Cookies exist to store data between pageviews; by overwriting any existing value with "uk" every time the page loads, what you're doing is no different from just writing var country = "uk"; and not using cookies at all. Moreover, unless there's a way for the user to choose another country, there's not even a reason to use a var much less a cookie. You could simply set componentRestrictions.country to "uk" directly.

Here's a rewrite

$(function () { // this will be called when the page has loaded - and keep things contained
  var input = $("#loc"), // find the elements once and for all
      lat   = $("#lat"),
      lng   = $("#lng"),
      lastQuery = null,
      autocomplete;

  function processLocation(query) {
    var query = $.trim(input.val()),
        geocoder;

    if( !query || query == lastQuery ) {
      return; // stop here if query is empty or the same as last time
    }

    lastQuery = query; // store for next time

    geocoder = new google.maps.Geocoder();
    geocoder.geocode({ address: query }, function(results, status) {
      if( status === google.maps.GeocoderStatus.OK ) {
        lat.val(results[0].geometry.location.lat());
        lng.val(results[0].geometry.location.lng());
      } else {
        alert("Sorry - We couldn't find this location. Please try an alternative");
      }
    });
  }

  // set up

  autocomplete = new google.maps.places.Autocomplete(input[0], {
    types: ["geocode"],
    componentRestrictions: {
      country: "uk"
    }
  });

  google.maps.event.addListener(autocomplete, 'place_changed', processLocation);

  $('#searchform').on('submit', function (event) {
    processLocation();
    event.preventDefault();
  });
});
\$\endgroup\$
  • \$\begingroup\$ Flambino, thank you so much, this is a really clean rewrite. I've been playing with this all morning but there is one little problem with it that I can't fix. After the search button is clicked it should geocode (if it hasn't already) and then submit the form. Your code does everything right except submit the form, can you tell me how I do that? \$\endgroup\$ – J.Zil May 16 '13 at 9:44
  • 1
    \$\begingroup\$ @JamesWillson Ah - missed that, sorry. I'd recommend that you should post the question on StackOverflow (since it's a "how do I..."-question) - or maybe there's something similar there already. I'd answer here, but the structure/flow of things will need some tweaking (you have to wait for the geocoding to succeed/fail before the form submission can procede. Otherwise, the form might well get submitted before the geocoder gets results back). Your code didn't make such checks, so neither does mine, and I'd rather keep my answer as close to 1:1 with the original question. I hope that makes sense \$\endgroup\$ – Flambino May 16 '13 at 16:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.