6
\$\begingroup\$

Took me a couple of days (in between working on other projects) to get the code working, but now that I do have it working I would like to know if there is a way that I could make it work better, or clean up my act JavaScript/jQuery surrounding my JSON utilization skills.

I am using ASP.NET if that means anything to the JavaScript.

Here is the code, ASPX pertinent to the JavaScript only:

<tr>
    <td>
        <asp:Label runat="server" ID="lblCity" Text="City:"></asp:Label></td>
    <td>
        <asp:TextBox runat="server" ID="txtCity" CssClass="textbox" Enabled="False" BackColor="#CCCCCC" ></asp:TextBox></td>
</tr>
<tr>
    <td>
        <asp:Label runat="server" ID="lblState" Text="State:"></asp:Label></td>
    <td>
        <asp:TextBox runat="server" ID="txtState" CssClass="textbox" Enabled="False" BackColor="#CCCCCC"></asp:TextBox></td>
</tr>
<tr>
    <td>
        <asp:Label runat="server" ID="lblZip" Text="Zip Code:"></asp:Label></td>
    <td>
        <asp:TextBox runat="server" ID="txtZip" CssClass="textbox" TabIndex="6"></asp:TextBox></td>
</tr>

...

<script>
    $(document).ready(function () {
        $('#<%=txtZip.ClientID%>').blur(function (e) {
            getAddressInfoByZip($(this).val());
        })
        function response(obj) {
            console.log(obj);
        }
        function getAddressInfoByZip(zip) {
            var addr = {};

            if (zip.length >= 5) {
                $.getJSON("https://maps.google.com/maps/api/geocode/json", { address: zip }, function success(results, status) {
                    for (var ii = 0; ii < results.results[0].address_components.length; ii++) {
                        var street_number = route = street = city = state = zipcode = country = formatted_address = '';
                        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;
                        }
                    };
                    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);
                });
            } else {
                response({ success: false });
            }   
        }
    })
</script>

I got the idea from SO and various places on the net, but I have hacked the code into this usable piece of script, but is it good code?

\$\endgroup\$
4
\$\begingroup\$

Global variables

In your function getAddressInfoByZip, you are declaring quite a number of global variables.

This is bad practice: all these variables should be local variables because they only serve a purpose in this function.

Now, you may not notice that you are creating a bunch of global variables. The evilness is happening in this line:

var street_number = route = street = city = state = zipcode = country = formatted_address = '';

The only local variable that is being created here is street_number; all those other variables are global.

Experiment

Run this in your console:

(function() { var foo = bar = "Hello, World!"; } )();

Then, try this:

console.log(foo);

You will get an error, because now that the anonymous function is out of scope, foo no longer exists.

Now, try this:

console.log(bar);

You will see

Hello, World!

Why? Well, since you created bar in that variable chain, it is created globally because there is no var keyword assigned to it.


This if statement is a little long:

if (types == "sublocality,political" || types == "locality,political" || types == "neighborhood,political" || types == "administrative_area_level_3,political") {

I recommend you split up each conditional by line, like this:

if (types == "sublocality,political" || 
    types == "locality,political" || 
    types == "neighborhood,political" || 
    types == "administrative_area_level_3,political") {

I'm confused by this line:

...document.getElementById('<%=txtCity.ClientID%>')...

Previously, you were using JQuery to interact with elements. Why did you suddenly stop using it here? If you are going to "import" JQuery, you might as well use it.


Inside the body of a function passed to $.getJSON, you are accessing a parameter passed in to the function called results. However, every time you are accessing it, you are accessing the 0th index of the results property of this results parameter.

Why don't you just store that in a variable? It would make your code a lot cleaner in a lot of places, and it increases readability because it doesn't make someone wonder why you are specifically accessing this value each time you want to use it.

Create a variable set to this value:

results.results[0];

I recommend creating an object of constants that are the different types. That way, you won't have to keep writing, for example,

"sublocality,political"

The object only has to be really simple: it's merely to store constants:

var Type = {
    SP: "sublocality,political",
    LP: "locality,political",
    AAL1P: "administrative_area_level_1,political",
    ...
}

Note: I chose to name the identifiers using PascalCase for the object name and SHOUTCASE for the property names because most enumerated types are named like this.

Now, you can easily access a type like this:

Types.LP

This improves the readability of your code by a lot, because it gets rid of all the strings floating around your conditional statements.

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

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...
\$\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.