Ex8 #14
Ex8 #14
Conversation
updating from ohad
|
new |
| @@ -0,0 +1,18 @@ | |||
| function ajax_get(url, callback) { | |||
| xmlhttp = new XMLHttpRequest(); | |||
| function ajax_get(url, callback) { | ||
| xmlhttp = new XMLHttpRequest(); | ||
| xmlhttp.onreadystatechange = function() { | ||
| if (xmlhttp.readyState == 4 && xmlhttp.status == 200) { |
There was a problem hiding this comment.
What would happen is readyState!==4 or status!==200. Right now, nothing (your callback isn't called), which is probably not good.
You can simulate this by calling ajax_get in the console, or by a click of a button, after you have killed your local HTTP server.
| if (xmlhttp.readyState == 4 && xmlhttp.status == 200) { | ||
| console.log('responseText:' + xmlhttp.responseText); | ||
| try { | ||
| var data = JSON.parse(xmlhttp.responseText); |
There was a problem hiding this comment.
FYI: while this is fine in javascript, it's customary (and mandatory in other languages) to move the var data outside of the try, because if the JSON.parse throws an error, data will remain undefined.
So you can do this instead:
var data = null; // or undefined; try{ data = JSON.parse(...) } . . . callback(data);
| console.log(err.message + " in " + xmlhttp.responseText); | ||
| return; | ||
| } | ||
| callback(data); |
There was a problem hiding this comment.
Are you fine with the callback being called with undefined (if the responseText cannot be JSON-parsed? Maybe throw an error instead (or signal something else to the callback (two standard options: 1) have two callbacks - one for success - with data, one for failure - with some error message) 2) have a first param to the callback be error - if it's null, the next param is the parsed json)
| requirejs.config({ | ||
| baseUrl: 'js', | ||
| paths: { | ||
| ajax: 'ajax', |
There was a problem hiding this comment.
You should use requirejs' paths for other people's code, not yours.
This just alias updatePhoneBook to update. If you want it to be called update, rename the file; if you want it to be called updatePhoneBook, change the require call for it.
| element.setAttributeNode(attribute); | ||
| element.innerHTML = info; | ||
| return element; | ||
| } |
There was a problem hiding this comment.
You should put these functions in the exports or module.exports. Right now they are globally available and used, only if you require them. This is called a side-effect and it's bad.
if you put these in exports, the caller can then be:
require('updatePhoneBook', function(updatePhoneBook) { updatePhoneBook.insertJson()); }
| <html> | ||
| <head> | ||
| <link rel="stylesheet" href="style/style.css"> | ||
| <script data-main="js/config" src="js/lib/require/require.js"></script> |
There was a problem hiding this comment.
data-main should be a javascript file that actually does the work (the several sines below: the calls for ajax_get, insertJson). Please move these calls into config and rename it to be main.js.
| window.onload = function() { | ||
| require(['config'], function() { | ||
| require(['ajax','update'], function() { | ||
| ajax_get('json/data.json', function(data) { |
There was a problem hiding this comment.
Style error: insertJson (camelCase) vs. ajax_get (all lower case with underscore). Please use camelCase (standard for javascript).
|
OK |
2 participants
No description provided.