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.

\$\endgroup\$
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
  }
}
\$\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
         }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I know this isn't terrifically related, but all 20x responses are success, not just 200 \$\endgroup\$ – Patrick Sep 16 '14 at 19:33

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.