1
\$\begingroup\$

I'm trying to create my own service, $hR. This service uses XMLHTTPrequest to get data from a URL with promise.

;(function(global){  

    var http = function(){
        var self = {}
        self.get = function (url){

            var ajax = new XMLHttpRequest();
            ajax.open('GET',url,true)
            ajax.send()
            var p = new Promise(function(resolve, reject){
                ajax.onreadystatechange = function (){
                    if(ajax.readyState == 4){
                        var obj = JSON.parse(ajax.responseText)
                        resolve(obj)
                    }
                }
            })
            return p;
           }//Get Methood
        return self;
        }
    global.$hR = http();
}(window));

It works great for me, but I created this service only to study and learn. I want to improve my code.

Can someone give me a more expert way to this same service?

\$\endgroup\$
2
\$\begingroup\$

First of all, I would stop what you're doing. What you're essentially doing is what the new fetch is for. It does an asynchronous HTTP call, and returns a promise. For older browsers, there's a polyfill.

Should you choose to proceed, here's a few things:

I am not sure why http is a function that returns an object that attaches to $hR. A lot of the code is unnecessary. If what you're doing is just exposing $hR as a global and having a bunch of functions on it and maybe a private scope to put stuff in, then the following will do:

;(function(ns){               // 2

  ns.get = function(){...}    // 3

}(this.$hR = this.$hR || {}); // 1

What this code does is:

  1. Uses an existing $hR global or create a new object and assign it as the $hR global.
  2. The global is passed into the closure as ns. ns will represent your global object.
  3. Attach functions to ns. In this case, the get function.

Now for your AJAX code. A cleaner way to define this code is to put everything inside the promise constructor. This way, the promise function just looks like a wrapper.

ns.get = function(url){
  return new Promise(function(resolve, reject){
    // The entire AJAX operation inside here
  });
}

Your xhr handler only checks for state 4. State 4 only indicates the operation succeeded but does not indicate that you encountered HTTP errors like 400 or 500. Additionally, if the xhr did fail, there's no way of knowing from the Promise since you did not do a reject. Check for HTTP success and failure, do a resolve and reject respectively.

if(ajax.readyState === 4 && ajax.status === 200){
  resolve(ajax.responseText);
} else {
  reject(ajax.responseText);
}
\$\endgroup\$
  • \$\begingroup\$ i try to use your answer but i give me erorr uncaught Promise , \$\endgroup\$ – Lior Cris Sep 5 '16 at 11:10
  • \$\begingroup\$ i think the problem is if and else , it get reject evrey time \$\endgroup\$ – Lior Cris Sep 5 '16 at 11:26
1
\$\begingroup\$
    ;(function(ns){  
    ns.get = function(url){
        return new Promise(function(resolve, reject){
        var ajax = new XMLHttpRequest();
                ajax.open('GET',url,true)
                ajax.send()
                ajax.onreadystatechange = function(){
                    if(ajax.readyState === 4 && ajax.status === 200){
                      resolve(JSON.parse(ajax.responseText));
                    }
                    else if (ajax.readyState !== 4 && ajax.status !== 200)
                      reject(ajax.responseText);

                    }
      });
    }
}(this.$hR = this.$hR || {}))

this is work for me! thank you so much!

\$\endgroup\$

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.