patternjavascriptMinor
Zip Code to City, State for registration form
Viewed 0 times
cityregistrationzipforstatecodeform
Problem
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:
...
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?
I am using ASP.NET if that means anything to the JavaScript.
Here is the code, ASPX pertinent to the JavaScript only:
...
$(document).ready(function () {
$('#').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 ').value = addr.city;
document.getElementById('').value = addr.state;
addr.success = true;
for (name in addr) {
console.log('### google maps api ### ' + name + ': ' + addr[name]);
}
response(addr);
});
} else {
response({ success: false });
}
}
})
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?
Solution
Global variables
In your function
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:
The only local variable that is being created here is
Experiment
Run this in your console:
Then, try this:
You will get an error, because now that the anonymous function is out of scope,
Now, try this:
You will see
Hello, World!
Why? Well, since you created
This
I recommend you split up each conditional by line, like this:
I'm confused by this line:
...document.getElementById('')...
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
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:
I recommend creating an object of constants that are the different types. That way, you won't have to keep writing, for example,
The object only has to be really simple: it's merely to store constants:
Note: I chose to name the identifiers using
Now, you can easily access a type like this:
This improves the readability of your code by a lot, because it gets rid of all the strings floating around your conditional statements.
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('')...
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.LPThis improves the readability of your code by a lot, because it gets rid of all the strings floating around your conditional statements.
Code Snippets
var street_number = route = street = city = state = zipcode = country = formatted_address = '';(function() { var foo = bar = "Hello, World!"; } )();console.log(foo);console.log(bar);if (types == "sublocality,political" || types == "locality,political" || types == "neighborhood,political" || types == "administrative_area_level_3,political") {Context
StackExchange Code Review Q#97249, answer score: 4
Revisions (0)
No revisions yet.