HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Implementing a cross-browser AJAX utility as an exercise

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
ajaxcrossutilityexercisebrowserimplementing

Problem

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.

Solution

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
  }
}

Code Snippets

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
  }
}

Context

StackExchange Code Review Q#28325, answer score: 3

Revisions (0)

No revisions yet.