• 1

• 2.7k
• 2
• 15
• 20

Variables

SirPython was very insightful with his catch about the global variables. I didn't even notice that when I read through. What he didn't notice is that the variables, global or not, are never really even used. Only the city variable is used in the condition below, and none of the others are ever referenced.

Even the city variable is superfluous, though. It is assigned '' each loop, then compared if it is == '', which it always is.

SirPython also recommends creating a variable for results.results[0]. I propose making a variable for results.results[0].address_components as you never use anything else from results.results[0].

Your addr variable is declared in too large a scope. It would be better in the success function, since it is not used outside of there.

Conditionals

var types = results.results[0].address_components[ii].types.join(",");
if (types == "sublocality,political" || types == "locality,political" || types == "neighborhood,political" || types == "administrative_area_level_3,political") {
 addr.city = (city == '' || types == "locality,political") ? results.results[0].address_components[ii].long_name : city;
}
if (types == "administrative_area_level_1,political") {
 addr.state = results.results[0].address_components[ii].short_name;
}

These conditionals have a lot of redundancy, and are very noisy. We can shorten them by not joining them together, but rather by dealing with the parts individually. We can see that all conditions depend on the second type being "political" so let's pull that out and continue if it's not there. We can then use an array trick to test if the first type is in the array, rather than having multiple checks.

Putting that together so far, we have:

var address_components = results.results[0].address_components;
for (var ii = 0; ii < address_components.length; ii++) {
 var types = address_components[ii].types;
 if (types[1] !== "political") {
 continue;
 }
 if (["sublocality", "locality", "neighborhood", "administrative_area_level_3"].indexOf(types[0]) !== -1) {
 addr.city = (types[0] == "locality") ? address_components[ii].long_name : '';
 }
 if (types[0] == "administrative_area_level_1") {
 addr.state = address_components[ii].short_name;
 }
}
document.getElementById('<%=txtCity.ClientID%>').value = addr.city;
document.getElementById('<%=txtState.ClientID%>').value = addr.state;
addr.success = true;
for (name in addr) {
 console.log('### google maps api ### ' + name + ': ' + addr[name]);
}
response(addr);

Now that we've cleaned it up a bit, this conditional confuses me:

if (["sublocality", "locality", "neighborhood", "administrative_area_level_3"].indexOf(types[0]) !== -1) {
 addr.city = (types[0] == "locality") ? address_components[ii].long_name : '';
}

We first check if the type is one of "sublocality", "locality", "neighborhood", or "administrative_area_level_3", then inside of that conditional, we assign the value only if the type is "locality". Otherwise, we assign an empty string (the former value of city). I think that this is not what was intended.

The outer conditional if (zip.length >= 5) can be reversed to reduce a level of indentation:

if(zip.length < 5) {
 response({ "success": false });
 return;
}
$.getJSON ...

Also, ZIP codes in the US (which appears to be your target) are only valid with exactly five or exactly nine (ZIP+4) digits. You can do some more robust validation to ensure an entered ZIP code is valid.

Error handling

You call response with an unsuccessful value when the ZIP code is not the right length, but you don't do so under other failure conditions. You could do the same if the HTTP request returns a failure, or if the API returns an error.

Other

document.getElementById('<%=txtCity.ClientID%>').value = addr.city;
document.getElementById('<%=txtState.ClientID%>').value = addr.state;

I agree with SirPython that you should be consistent and use jQuery here as well.

A matter of opinion, but I don't like to see <%= ... %> blocks sprinkled in among Javascript. It makes it a little harder to read, and to see where values are coming from. If they are a must, I at least prefer to see them assigned to constants at the top of the script:

var ZIP_ID = "#<%= txtZip.ClientID %>";
var CITY_ID = "#<%= txtCity.ClientID %>";
var STATE_ID = "#<%= txtState.ClientID %>";
// The rest of the code...