3
\$\begingroup\$

I have a Google Maps Geocoder and I am looking to refactor it. The user enters address details into a form where at the bottom there is the option to show on map.

  • First the map is hidden
  • The user clicks show_map checkbox
  • Map-area is shown
  • Map is initialized

The codeAddress() function is used if the user edits the address and wants to find it again. The showCoords() function passes the longitude and latitude through to the form for processing. I am looking for a best practice to refactor this code.

function initialize() {

    geocoder = new google.maps.Geocoder();

    codeAddress();

    map = new google.maps.Map(document.getElementById('map-canvas'));

    marker = new google.maps.Marker({
        map: map,
        position: map.getCenter(),
        draggable: true
    });

    google.maps.event.addListener(marker, 'dragend', function() {
        point = marker.getPosition();
        map.panTo(point);
        showCoords(point);
    });

    google.maps.event.addListener(map, 'dragend', function() {
        point = marker.getPosition();
        marker.setPosition(map.getCenter());
        showCoords(point);
    });

} /* END initialize */

function codeAddress(){
    var address = document.getElementById('address').value;
    geocoder.geocode({'address': address}, function(results, status){
        if(status == google.maps.GeocoderStatus.OK){
            map.setCenter(results[0].geometry.location);
            map.setZoom(15);
            marker.setPosition(map.getCenter());
            showCoords();
        }else{
            alert('Geocode was not successful for the following reason: ' + status)
        }
    });
}

function showCoords(point){
    point = marker.getPosition();
    document.getElementById("lat").value = point.lat().toFixed(5);
    document.getElementById("lng").value = point.lng().toFixed(5);
}
\$\endgroup\$
1
\$\begingroup\$

The main change I would make is to reduce the responsibility of codeAddress() to do just the geocoding. If it successfully looks up the address, then it can pass the location to a callback.

I don't have enough context to tell whether geocoder, map, and marker are global variables, or whether all of this code lies inside some other scope. Out of habit, I've converted them into local variables. You decide whether that's right for you. The point variables in your drag handlers should definitely be made local, though!

function initialize() {
    var geocoder = new google.maps.Geocoder();
    var map = new google.maps.Map(document.getElementById('map-canvas'));

    var marker = new google.maps.Marker({
        map: map,
        position: map.getCenter(),
        draggable: true
    });

    google.maps.event.addListener(marker, 'dragend', function() {
        var point = marker.getPosition();
        map.panTo(point);
        showCoords(point);
    });

    google.maps.event.addListener(map, 'dragend', function() {
        var point = marker.getPosition();
        marker.setPosition(map.getCenter());
        showCoords(point);
    });

    var address = document.getElementById('address').value;

    codeAddress(geocoder, address, function(location) {
        map.setCenter(results[0].geometry.location);
        map.setZoom(15);
        marker.setPosition(map.getCenter());
        showCoords();
    });
}

function codeAddress(geocoder, address, callback) {
    geocoder.geocode({'address': address}, function(results, status) {
        if (status == google.maps.GeocoderStatus.OK) {
            callback(results[0].geometry.location);
        } else {
            alert('Geocode was not successful for the following reason: ' + status)
        }
    });
}
\$\endgroup\$

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.