2
\$\begingroup\$

Not sure if this question will be considered "off topic". If it is, I'll remove it, but: I hadn't see this yet so I wrote it and would like to know if this is a good approach to it. Would anyone care to offer improvements to it, or point me to an example of where someone else has already written it better?

function clwAjaxCall(path,method,data,asynch)
  {
    var xmlhttp;
    if (window.XMLHttpRequest)
      {// code for IE7+, Firefox, Chrome, Opera, Safari
      xmlhttp=new XMLHttpRequest();
      }
    else
      {// code for IE6, IE5
      xmlhttp=new ActiveXObject("Microsoft.XMLHTTP");
      }    
    if(asynch)
      {
        xmlhttp.onreadystatechange=function()
          {
          if (xmlhttp.readyState==4 && xmlhttp.status==200)
            {
            //alert(xmlhttp.responseText);
            //var newaction=xmlhttp.responseText;
            //alert('Action becomes '+newaction);
            return xmlhttp.responseText;
            }
          }        
      }
    if(method=='GET'){path=path+"/?"+data;}  
    xmlhttp.open(method,path,asynch);
    if(method=='GET'){xmlhttp.send();}else{xmlhttp.send(data);}
    if (!asynch){return xmlhttp.responseText;}

  }

I then called it like

Just Testing
<script type="text/javascript" src="/mypath/js/clwAjaxCall.js"></script>
<script type="text/javascript">
  document.write("<br>More Testing");
  document.write(clwAjaxCall("http://www.mysite.com",'GET',"var=val",false));
</script>

UPDATE

I don't know if it's any better to anyone else than it was before, but I like it. :-)

I have re-written it like this:

// I wrote this with help from StackOverflow and have twaeked it some since then.
// call it like this:
//
//        var holder={};
//        holder.text='';
//        watch(holder,function(){ //depends on watch.js. You might choose a different watch/observe method.
//          //do stuff with holder.text in here
//        });
//        var path='http://www.example.com/?key=value';
//        AjaxCall(path,"get",null,true,holder);

function AjaxCall(path,method,data,asynch,holder){
    // holder is expected to be an object. It should be watched with watch.js, Object.observe or a similar method as they become available.
    var xmlhttp;
    if (window.XMLHttpRequest){// code for IE7+, Firefox, Chrome, Opera, Safari
      xmlhttp=new XMLHttpRequest();
    }
    else{// code for IE6, IE5
      xmlhttp=new ActiveXObject("Microsoft.XMLHTTP");
    }    
    if(asynch){
      xmlhttp.onreadystatechange=function(){
        if (xmlhttp.readyState==4 && xmlhttp.status==200){
          holder.text=xmlhttp.responseText;
        }
      }        
    }
    if(method=='GET'){path=path+"/?"+data;}  
    xmlhttp.open(method,path,asynch);
    if(method=='GET'){xmlhttp.send();}else{xmlhttp.send(data);}
    if (!asynch){return xmlhttp.responseText;}
}
\$\endgroup\$
  • \$\begingroup\$ I think You're right. I've seen questions get moved before, but I don't know how / probably don't have the proper permissions to do it yet. I did just create an account over there. Thanks. \$\endgroup\$ – TecBrat Mar 29 '12 at 13:48
  • \$\begingroup\$ You can flag the question for moderator attention so it gets migrated. Click the flag link under the question. \$\endgroup\$ – Mike L. Mar 29 '12 at 13:49
  • \$\begingroup\$ I've already found an issue with it, and it might just be me not quite knowing how the implement it, and that's when async is set to true, I'm getting "undefined" as my output. \$\endgroup\$ – TecBrat Mar 29 '12 at 14:09
  • \$\begingroup\$ Use jQuery ( see api.jquery.com/jQuery.ajax ). They have a robust set of Ajax methods and other good stuff. \$\endgroup\$ – Randy Mar 29 '12 at 14:11
  • \$\begingroup\$ I see a lot of "use JQuery" responses and half way expect them to be sarcastic, but that does look like a robust solution. (just for practice, I think I'll tweak mine to accept a function name for asynch requests anyway.) \$\endgroup\$ – TecBrat Mar 29 '12 at 14:42
1
\$\begingroup\$

Your code seems fairly robust, but it is inefficient. Here are a few ways you could speed it up.

  • Reuse request objects. There is a good bit of overhead creating these, so you definitely want to be reusing these if you are making more than a few requests per page. But be careful - some older browsers have strange quirks about reused request objects.
  • Don't make a new function for every onreadystatechange. The function is executed with xmlhttp as the context, so, within that function, this === xmlhttp.
  • Whenever possible, cache. You can cache whether the browser supports XMLHTTPRequest or ActiveX so you don't check every time the function is called.
  • synchronous requests are usually a bad idea. Are you sure you should be using them? If not, don't write code that is more general than it needs to be. It will just make the function less efficient.

Also, you're missing a semicolon.

As an example, here's a loader function I wrote a while ago. The feature set is a little different than yours - it doesn't support synchronous requests, but it does support error handlers, data encoding, and timeouts - and behind the scenes it implements all the optimizations above. Like your code, it results in a single function; the simplest use case would be load('log.php');.

/***
* load.js
*   Simplifies use of ajax.
* parameters
*   url     : String : address of requested content
*   options : Object : (optional) additional parameters. All keys are optional.
*     onload  : Function : passed request.responseText on success
*     onfail  : Function : passed status on status!=200 or 'timeout' on timeout
*     post    : Object   : POST key-value pairs. GET is used when post==false
*     timeout : Number   : seconds to wait before aborting request
***/

var load=(function() {
    var pool=[],
        getReq=window.XMLHttpRequest?
            function() {return new XMLHttpRequest();}:
            function() {return new ActiveXObject('Microsoft.XMLHTTP');};
    function statechange() {
        if(this.readyState==4) {
            clearTimeout(this.t);
            this.onreadystatechange=null;
            pool[pool.length]=this;
            var fn=this.o[this.status==200?'onload':'onfail'];
            delete this.o;
            fn&&fn(this.status==200?this.responseText:this.status);
        }
    }
    return function(url,options) {
        var req,t,data='';
        req=pool.length?pool.pop():getReq();
        req.o=options=options||{};
        if(options.post) {
            for(t in options.post)
                data+='&'+encodeURIComponent(t)+'='+encodeURIComponent(options.post[t]);
            data=data.substring(1);
            req.open('POST',url,true);
            req.setRequestHeader('Content-type','application/x-www-form-urlencoded');
        } else {
            req.open('GET',url,true);
        }
        req.onreadystatechange=statechange;
        req.send(data||undefined);
        if(options.timeout) {
            req.t=setTimeout(function() {
                req.onreadystatechange=null;
                req.abort();
                pool[pool.length]=req;
                if(req.o.onfail)
                    req.o.onfail('timeout');
            },options.timeout*1000);
        }
    };
})();
\$\endgroup\$
  • \$\begingroup\$ Nice library-independent load functionality, boost :) Have you thought about componentizing it by returning an object with the load (and possible future) method(s) as properties, as opposed to putting your load method on the global object? \$\endgroup\$ – Ryan Apr 1 '12 at 23:16
  • \$\begingroup\$ Thanks. When I wrote this it was purely for my own use, so no, I hadn't. But now that you mention it, I really shouldn't be forcing load into the global context. I updated the code (I still don't want to return an object though, because I like the simplicity of load(url);.) \$\endgroup\$ – delete me Apr 2 '12 at 12:37
  • \$\begingroup\$ When I have time, I'll try that out. Thanks. \$\endgroup\$ – TecBrat Apr 2 '12 at 14:53

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.