3
\$\begingroup\$

I am currently learning JavaScript and created this cross-browser AJAX utility as a learning exercise. It was not meant to be used in production code, and I just tried to cover only the cases that I'm aware of, so there might be a lot of edge cases that are not covered.

I'd be happy if you could check it and let me know how it looks and give me suggestions on how to improve it:

var ajax = {
 request: function(url, requestType, callback, queryString) {
 var ids = ['MSXML2.XMLHTTP.3.0',
 'MSXML2.XMLHTTP',
 'Microsoft.XMLHTTP'],
 xhr;
 if (typeof window.XMLHttpRequest === 'function') {
 xhr = new XMLHttpRequest();
 } else { // For IE versions below 7
 for (var i = 0; i < ids.length; i++) {
 try {
 xhr = new ActiveXObject(ids[i]);
 break;
 } catch(e) {}
 }
 }
 // Do I really need to put the callback in a
 // closure, as the xhr object gets recreated
 // each time ajax.request() is called?
 xhr.onreadystatechange = (function(myXhr) {
 return function() {
 callback(myXhr);
 };
 })(xhr);
 xhr.open(requestType, url, true);
 if (requestType.toUpperCase() === 'GET') {
 xhr.send('');
 } else if (requestType.toUpperCase() === 'POST') {
 xhr.send(queryString);
 }
 }
}

P.S.: This exercise is from the book "Object-Oriented JavaScript" by Stoyan Stefanov.

asked Jul 10, 2013 at 12:41
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

I can't specifically comment on your usage of the extra tries for < IE7, as I always make sure my audience is using something better. It seems like there should be a more elegant way to check each of those than a try with an empty catch, but without knowing how IE treats them, I can't say what it would be. The rest is okay, but I would suggest the following. My changes are explained in my own comments.

var ajax = {
 request: function(url, requestType, callback, queryString) {
 var ids = ['MSXML2.XMLHTTP.3.0',
 'MSXML2.XMLHTTP',
 'Microsoft.XMLHTTP'],
 xhr;
 // Simplification of this check while essentially doing the same thing
 if (window.XMLHttpRequest) {
 xhr = new XMLHttpRequest();
 } else {
 for (var i = 0; i < ids.length; i++) {
 try {
 xhr = new ActiveXObject(ids[i]);
 break;
 } catch(e) {}
 }
 }
 // Calling a function to return a function is redundant.
 // Do what you're trying to with as little extra as possible.
 xhr.onreadystatechange = function() {
 callback(xhr);
 };
 xhr.open(requestType, url, true);
 if (requestType.toUpperCase() === 'GET') {
 // When initiating a get request, the send function needs no arguments at all.
 xhr.send();
 } else if (requestType.toUpperCase() === 'POST') {
 xhr.send(queryString);
 }
 // If you want to be extra careful, include an else here to handle a bad requestType
 }
}
answered Jul 11, 2013 at 16:23
\$\endgroup\$
0
\$\begingroup\$

Another idea to extend the callback function

Sometimes it never reaches state 4 (finished), for example in temporary connection loss... in that case you will need to send request again... however the previous callback may suddenly be called also and you get it twice.

x.onreadystatechange = function() {
 if(x.readyState== 4){
 if(x.status==200){
 callbackFunction(x.responseText);
 } else {
 // request error
 }
 }
}
answered Nov 29, 2013 at 9:41
\$\endgroup\$
1
  • \$\begingroup\$ I know this isn't terrifically related, but all 20x responses are success, not just 200 \$\endgroup\$ Commented Sep 16, 2014 at 19:33

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.