1
\$\begingroup\$

I've created two simple methods: one to instantiate a new XMLHttpRequest object and open the request and another to handle the request.

Can you please point out improvements that can be made or any shortcomings in my code?

ajax: function(opts) {

  var xhr, method, body, constructor;

  xhr = new XMLHttpRequest();
  method = opts.body ? 'POST' : opts.method ? opts.method.toUpperCase() : 'GET';

  if(opts.body) {
    constructor = opts.body.constructor;
    if(constructor === Object || constructor === String) {
      body = opts.body;
    }
  }

  this.ajaxComplete.call(xhr, opts);

  xhr.open(method, opts.url, (opts.async === undefined ? true : false));

  // Set the default request headers
  xhr.setRequestHeader('X-Requested-With', 'XMLHttpRequest');
  xhr.setRequestHeader('Content-Type', 'application/x-www-form-urlencoded');

  if(opts.headers) {
    for(var prop in opts.headers) {
      xhr.setRequestHeader(prop, opts.headers[prop]);
    }
  }

  xhr.send(body || null);
},

ajaxComplete: function(opts) {
  var that = this,
      data, complete;

  that.onreadystatechange = function() {
    if(that.readyState == 4) {
      if(that.status == 200) {
        data = (that.responseXML || that.responseText);
        opts.success.call(that, data);
      }
      else {
        data = (that.responseXML || that.responseText);
        opts.error.call(opts, data, that.statusText, that.status);
      }
    }
  }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ Does your code work? constructor===Object seems fishy \$\endgroup\$ – Dan Pantry Apr 16 '15 at 16:42
2
\$\begingroup\$

As Dan Pantry pointed out, === Object and === String do not work; you'll have to compare with a string name of the object:

if(opts.body) {
  constructor = opts.body.constructor;
  if(typeof constructor === "object" || typeof constructor === "string") {
    body = opts.body;
  }
}

Not every browser supports the XMLHttpRequest object. You'll have to do a little checking to get the right object.

The two main objects used are XMLHttpRequest and ActiveXObject("Microsoft.XMLHTTP"). The second one is used by IE.

You can use a simple ternary to see which object to use:

xhr = (window.XMLHttpRequest ? new XMLHttpRequest() : new ActiveXObject("Microsoft.XMLHTTP"));

If you need to copy this into a variable, most people use self, not that.


I don't think you need to check whether to return that.responseXML or that.responseText; I think responseText will contain the response no matter what, so you should just use that.


The complete variable is un-used in the ajaxComplete function; you don't need it.


You should put parenthesis around some places when defining the method variable. It is quite difficult to understand being that there is more than one ternary.


For one-time-use variables like data, don't store them in a variable; just substitue the value of the variable where you are using the variable and destroy the variable.

It's a waste of memory to keep something in memory that you only use once.

\$\endgroup\$
  • 1
    \$\begingroup\$ Are you sure you aren’t mixing up constructor with typeof? typeof most certainly does return strings, but constructor will be a function, like String. \$\endgroup\$ – icktoofay Jun 29 '15 at 3:35
  • 1
    \$\begingroup\$ Also, XMLHttpRequest ? /* … */ : /* … */ will fail with a ReferenceError if XMLHttpRequest doesn’t exist—you’ll need to use window.XMLHttpRequest in the condition instead. \$\endgroup\$ – icktoofay Jun 29 '15 at 3:37

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.