Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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>
share|improve this question
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. – TecBrat Mar 29 '12 at 13:48
You can flag the question for moderator attention so it gets migrated. Click the flag link under the question. – Mike L. Mar 29 '12 at 13:49
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. – TecBrat Mar 29 '12 at 14:09

migrated from programmers.stackexchange.com Mar 29 '12 at 13:53

This question came from our site for professional programmers interested in conceptual questions about software development.

2 Answers

up vote 3 down vote accepted

Use jQuery ( see http://api.jquery.com/jQuery.ajax/ ). They have a robust set of Ajax methods and other good stuff.

share|improve this answer
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.) – TecBrat Mar 29 '12 at 14:42
jQuery is a good solution. If nothing else, per your "tweak" comment, compare your Ajax method vs the one they have coded. – Randy Mar 29 '12 at 21:55
So, the success: hook would be the feature I'm talking about. I think I get it. Thanks. – TecBrat Mar 30 '12 at 1:20

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);
        }
    };
})();
share|improve this answer
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? – Ryan Apr 1 '12 at 23:16
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);.) – st-boost Apr 2 '12 at 12:37
When I have time, I'll try that out. Thanks. – TecBrat Apr 2 '12 at 14:53

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.