3
\$\begingroup\$

I decided to make a simple project to learn the Google Maps API. I'd appreciate a review of the code below. I'd like to know what I did right and wrong.

The code pulls data from one source and adds markers on a map via the Google Maps API. A working sample is located here: www.seedtip.com

$(function() {
//var geocoder = new google.maps.Geocoder();

    var marketId = []; //returned from the API
    var allLatlng = []; //returned from the API
    var allMarkers = []; //returned from the API
    var marketName = []; //returned from the API
    var infowindow = null;
    var pos;
    var userCords;
    var tempMarkerHolder = [];

    //Start geolocation

    if (navigator.geolocation) {    

        function error(err) {
            console.warn('ERROR(' + err.code + '): ' + err.message);
        };

        function success(pos){
            userCords = pos.coords;
        }

        // Get the user's current position
        navigator.geolocation.getCurrentPosition(success, error);
        //console.log(pos.latitude + " " + pos.longitude);
        } else {
            alert('Geolocation is not supported in your browser');
        }

    //End Geo location

//map options
var mapOptions = {
    zoom: 5,
    center: new google.maps.LatLng(37.09024, -100.712891),
    panControl: false,
    panControlOptions: {
        position: google.maps.ControlPosition.BOTTOM_LEFT
    },
    zoomControl: true,
    zoomControlOptions: {
        style: google.maps.ZoomControlStyle.LARGE,
        position: google.maps.ControlPosition.RIGHT_CENTER
    },
    scaleControl: false,

};

//Adding infowindow option
infowindow = new google.maps.InfoWindow({
    content: "holding..."
});

//Fire up Google maps and place inside the map-canvas div
map = new google.maps.Map(document.getElementById('map-canvas'), mapOptions);

//grab form data
$('#chooseZip').submit(function() { // bind function to submit event of form

    //define and set variables
    var userZip = $("#textZip").val();
    //console.log("This-> " + userCords.latitude);

    var accessURL;

    if(userZip){
        accessURL = "http://search.ams.usda.gov/farmersmarkets/v1/data.svc/zipSearch?zip=" + userZip;
    } else {
        accessURL = "http://search.ams.usda.gov/farmersmarkets/v1/data.svc/locSearch?lat=" + userCords.latitude + "&lng=" + userCords.longitude;
    }


        //Use the zip code and return all market ids in area.
        $.ajax({
            type: "GET",
            contentType: "application/json; charset=utf-8",
            url: accessURL,
            dataType: 'jsonp',
            success: function (data) {

                 $.each(data.results, function (i, val) {
                    marketId.push(val.id);
                    marketName.push(val.marketname);
                 });

                //console.log(marketName);

                var counter = 0;
                //Now, use the id to get detailed info
                $.each(marketId, function (k, v){
                    $.ajax({
                        type: "GET",
                        contentType: "application/json; charset=utf-8",
                        // submit a get request to the restful service mktDetail.
                        url: "http://search.ams.usda.gov/farmersmarkets/v1/data.svc/mktDetail?id=" + v,
                        dataType: 'jsonp',
                        success: function (data) {

                        for (var key in data) {

                            var results = data[key];

                            //console.log(results);

                            //The API returns a link to Google maps containing lat and long. This pulls it apart.
                            var googleLink = results['GoogleLink'];
                            var latLong = decodeURIComponent(googleLink.substring(googleLink.indexOf("=")+1, googleLink.lastIndexOf("(")));

                            var split = latLong.split(',');
                            var latitude = split[0];
                            var longitude = split[1];

                            //set the markers.    
                            myLatlng = new google.maps.LatLng(latitude,longitude);

                            allMarkers = new google.maps.Marker({
                                position: myLatlng,
                                map: map,
                                title: marketName[counter],
                                html: 
                                        '<div class="markerPop">' +
                                        '<h1>' + marketName[counter].substring(4) + '</h1>' + //substring removes distance from title
                                        '<h3>' + results['Address'] + '</h3>' +
                                        '<p>' + results['Products'].split(';') + '</p>' +
                                        '<p>' + results['Schedule'] + '</p>' +
                                        '</div>'
                            });

                            //put all lat long in array
                            allLatlng.push(myLatlng);

                            //Put the marketrs in an array
                            tempMarkerHolder.push(allMarkers);

                            counter++;
                            //console.log(counter);
                        };

                            google.maps.event.addListener(allMarkers, 'click', function () {
                                infowindow.setContent(this.html);
                                infowindow.open(map, this);
                            });


                            //console.log(allLatlng);
                            //  Make an array of the LatLng's of the markers you want to show
                            //  Create a new viewpoint bound
                            var bounds = new google.maps.LatLngBounds ();
                            //  Go through each...
                            for (var i = 0, LtLgLen = allLatlng.length; i < LtLgLen; i++) {
                              //  And increase the bounds to take this point
                              bounds.extend (allLatlng[i]);
                            }
                            //  Fit these bounds to the map
                            map.fitBounds (bounds);


                        }
                    });
                }); //end .each
            }
        });

    return false; // important: prevent the form from submitting
});
});
\$\endgroup\$
3
\$\begingroup\$

This is pretty nice so far. The code is easy to read, clear, straightforward.

The single biggest problem is probably this:

var mapOptions = {
    // ... a bunch of fields here
    scaleControl: false,  // <- remove that comma!
};

Many nice browsers let you get away with it, but that comma at the end is incorrect syntax. In some browsers (and non-browsers like IE) this can crash the script with cryptic error messages (happened to me, wasn't fun).


When iterating over the fields of a JavaScript object, you have to check if the field really belongs to that object, like this:

for (var key in data) {
    if (!data.hasOwnProperty(key)) {
        continue;
    }
    // ... 
}

For more details, see this post.


Declare variables in the smallest possible scope where they are used. For example the marketId array is not used outside the AJAX call, so it would have been better to declare there, and right before needed. Same goes for allLatlng, allMarkers, and possible others too I didn't verify.


This URL appears multiple times with minor variations:

accessURL = "http://search.ams.usda.gov/farmersmarkets/v1/data.svc/zipSearch?zip=" + userZip;

It would be better to define a baseURL to avoid duplicated string literals:

var baseURL = "http://search.ams.usda.gov/farmersmarkets/v1/data.svc";

It may seem a minor thing, but the indentation is not consistent, for example it's inexplicably reduced by one level at the start of var mapOptions = ..., then after a couple of lines it goes back to normal. It would improve readability to be consistent with that.


Unused local variable:

var pos;

Unnecessary semicolon at the end of function declaration:

function error(err) {
    console.warn('ERROR(' + err.code + '): ' + err.message);
};

There's one more, at the end of the for (var key in data) { ... }; loop.

\$\endgroup\$
2
\$\begingroup\$

I think you just need to declare your image var and again in the function call:


//set the markers.    
myLatlng = new google.maps.LatLng(latitude,longitude);
var image = "http://static.scripting.com/glyphs2/" + iconname + ".png";                   
allMarkers = new google.maps.Marker({
  position: myLatlng,
  map: map,
  icon: image,
  title: marketName[counter],
  html: 
   '<div class="markerPop">' +
    '<h1>' + marketName[counter].substring(4) + '</h1>' + //substring removes distance from title
    '<h3>' + results['Address'] + '</h3>' +
    '<p>' + results['Products'].split(';') + '</p>' +
    '<p>' + results['Schedule'] + '</p>' +
    '</div>'
});

You might also want to consider wrapping this in another function and applying Google Maps Geocoder, as you are pulling them on the fly.

\$\endgroup\$
  • \$\begingroup\$ Yeah, I was considering using the GeoCoder. I might do that in the future. Thanks \$\endgroup\$ – Paul Dessert Oct 21 '14 at 6:36

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.