Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

EDIT - 0 - Still need to fix markup language to make more unique so escaping is not needed:

This will be a JSON / JQuery Alternative.

/********************
group:ajax
********************/


/*
- ajax_object() - creates a browser dependent ajax_object for the ajax method call 
 - good place to add mysql logging to track object usage and reshape code via machine learning
 - ouptut - returns false or ajax_object
*/

function ajax_object()
  {
  var object;
  try
    {
    object=new XMLHttpRequest();
    }
  catch(error_1)
    {
    alert('ajax_object : not instantiated:contact-support@archemarks.com : Error : ' + error_1)
    }
  return object;
  }

/* 
- ajax() - post style ajax 
 - 'path' holds the servers side function to respond to the request
 - 'param' hold the serialized information to send the server
 - 'ajax_func' holds the javascript function to respond to the server
 - 'div' div holds the html location to send the response text
 -  Output: returns 1 for pass.
*/

function ajax(path,param,ajax_func,html_div)
  {
  var object=new ajax_object();
  object.open("POST",path,true);
  object.setRequestHeader("Content-type","application/x-www-form-urlencoded");
  object.setRequestHeader("Content-length",param.length);
  object.setRequestHeader("Connection","close");
  object.onreadystatechange=function()
    {
    if(this.readyState===4)
      {
      if(this.status===200)
        {
        if(this.responseText!=null)
          {
          ajax_func(this.responseText,html_div);
          }
        else alert("ajax: responseText is null")
        }
      else alert('AJAX FAIL: Path - ' + path + ' .status = ' + this.status + ' . responseText = ' + this.responseText)
      }
    }
    object.send(param);
    return 1;
  }

/*
- "enumeration" for aml_status_type
*/

var aml_status_type = 
  {
  "pass":0, 
  "fail":1, 
  "undefined":2
  }

/*
- check_aml()
*/

function check_aml(text)
  {
  var aml_response=patterns.aml.exec(text);   
  if(aml_response)
    {
    if(aml_response[2]=='p') 
      {
      return aml_status_type.pass;
      }
    else if (aml_response[2]=='f')
      {
      return aml_status_type.fail; 
      }
    }
  else
    {
    return aml_status_type.undefined; 
    }
  }

/*
- ajax_signin()
*/

function ajax_signin(server_response_text,html_div)
  {
  var aml_status=check_aml(server_response_text.slice(0,6));
  if(aml_status===aml_status_type.pass)
    {
    reload();
    }
  else if(aml_status===aml_status_type.fail)
    {
    document.getElementById(html_div).innerHTML=''; 
    document.getElementById(html_div).innerHTML=server_response_text;  
    }
  else if(aml_status===aml_status_type.undefined)
    {
    server_response_text=server_response_text.substr(6);
    alert('php error: ' + server_response_text);  
    }
  }

/*
- ajax_singup()
*/

function ajax_signup(server_response_text,html_div)
  {
  aml_status=check_aml(server_response_text.slice(0,6));
  if(aml_status===aml_status_type.pass)
    {
    document.forms['upload_form'].submit();
    }
  else if(aml_status===aml_status_type.fail)
    {
    document.getElementById(html_div).innerHTML=server_response_text;
    }
  else if(aml_status===aml_status_type.undefined)
    {
    server_response_text=server_response_text.substr(6);
    alert('php error: ' + server_response_text); 
    }
  }

/*
- ajax_bookmark()
*/ 

function ajax_bookmark(server_response_text,html_div)
  {
  aml_status=check_aml(server_response_text.slice(0,6));
  if(aml_status===aml_status_type.pass)
    {/*add code here when ready */}
  else if (aml_status===aml_status_type.fail)
    {/*add code here when ready */}
  else if (aml_status===aml_status_type.undefined)
    {
    server_response_text=server_response_text.substr(6);
    alert('php error: ' + server_response_text); 
    }
  }

/*
- ajax_tweet() - takes in sturctured data and converts to html - remove the conversion into another method
*/

function ajax_tweet(server_response_text,html_div)
  {
  var first_split,second_split,tweet_count,return_string='';  
  var aml_status=check_aml(server_response_text.slice(0,6));     
  if(aml_status===aml_status_type.pass)
    {
    server_response_text=server_response_text.substr(6);
    first_split=server_response_text.split(/\|\|/);
    for(tweet_count=0;tweet_count<first_split.length;tweet_count++)
      {
      second_split=first_split[tweet_count].split(/\|/);
      return_string=return_string+'<div class="Bb2b"><img class="a" src="pictures/' + second_split[0] + '.jpg" alt=""/><a class="a" href="javascript:void(0)\">' + second_split[1]  + ' posted ' + view_date(second_split[2],second_split[3]) + '</a><br/><p class="c">' + second_split[4] + '</p></div>';
      }
    fill_id(html_div,return_string);
    }
  else if (aml_status===aml_status_type.fail)    
    {/*add code here when ready */}
  else if (aml_status===aml_status_type.undefined)
    {
    server_response_text=server_response_text.substr(6);
    alert('php error: ' + server_response_text);   
    }
  }

/*
- ajax_null()
*/ 

function ajax_null()
  {
  }
share|improve this question

1 Answer 1

up vote 4 down vote accepted
  • If you're just passing the IDs of elements, rather than the elements themselves, your variable names should reflect that. (Yes, it's possible to pass DOM elements around -- they're just objects, as far as JS cares. And if you passed the elements instead of their IDs, the code to do stuff with them would be shorter and you wouldn't feel the need for all those annoying m# functions.)

  • ajax_object returns false rather than throwing an error if AJAX isn't supported. Moreover, it's more complicated than it needs to be. Every browser that matters now supports XMLHttpRequest; the two ActiveXObjects are allowances for IE6, which IMO can safely be ignored. The only ones still stuck with IE6 are companies with pigheaded IT departments (which would probably blacklist your site anyway as "not work related") and people in China (but can they even get to non-whitelisted sites? The thought police seem really strict there). But even if you wanted to support them, it can be simpler -- you can define XMLHttpRequest yourself.

    Observe:

    if (!window.XMLHttpRequest) {
        XMLHttpRequest = function() {
            // try getting the newest version first
            // if that fails, return a mostly-harmless fallback
            // if *that* fails, MSXML2 isn't supported and an error is thrown
            try {
                return new ActiveXObject("Msxml2.XMLHTTP.6.0");
            }
            catch (ex) {
                return new ActiveXObject("Msxml2.XMLHTTP");
            }
        };
    }
    
    // delete function ajax_object altogether; you don't need it anymore
    
    function ajax(path,param,ajax_func,html_div) {
        var object=new XMLHttpRequest();
        ...
    

    IE6 includes MSXML v3, as does every OS since Win2k SP4. And v2, i believe, is supported as far back as IE5 and Win98. If your user is running something older than that, stuff should break. Gleefully. And Microsoft.XMLHTTP is just an alias for Msxml2.XMLHTTP, so you don't really gain anything by trying to use that name as a fallback.

  • This "aml" stuff kinda creeps me out. If you intend for the response to be interpreted by JS, a more standard solution like JSON may be a better way to go. It's more portable (namely, you end up with built-in encoding/decoding in both JS and PHP), and you don't need to call a function like check_aml. In this case, it could be something like var response = JSON.parse(server_response_text); if (response.status == 'fail') {...}, and you could get rid of aml_status_type and check_aml. Though ideally, you'd parse the response in your onreadystatechange handler and pass that to the various functions.

  • In ajax_bookmark, you have code like

    if(aml_status===aml_status_type.pass)
      {;}
    else if (aml_status===aml_status_type.fail)
      {;}
    else if (aml_status===aml_status_type.undefined)
      {
    

    The first two if sections should not exist, if you're not going to have anything in them. Only test for the conditions you actually intend to do something about. (If you actually had code there and took it out to lessen the amount of code you paste here, ok. Ignore the suggestion to remove. But for future reference, when you omit stuff, a comment like /* do stuff here */ or an ellipsis (...) helps us know that.)

  • document.f1_1.submit();? You use document.getElementById('...') everywhere else; this is inconsistent. Either way, though, you're hard-coding the form name/id, which is kinda...eh.

  • I see references to m2() and m6(). These names are useless; without the code to the functions, there's no way to say what they do. The names should be descriptive enough that one can come in without prior knowledge, read each function individually and get the gist of it, without having to know intimate details about every other function in the script. As it is, i'd have to yoyo through the file (if there were even definitions for the functions, which there aren't in this sample).

share|improve this answer
    
in progress....(1) nice eye on the inconsistent usage of accessing the forms...I fixed this....(2)..added in comments to empty structures...I will fill these out later...2 complete..question on my item #3... –  user7459 Nov 10 '11 at 22:53
    
You pass elements the same way you pass any other object in JS. var element = document.getElementById('some_id');...and now you can say doStuffWith(element); and such as that. –  cHao Nov 10 '11 at 23:02
    
Posted minified methods. (3). Will remove these bit by bit as it might take a while. –  user7459 Nov 10 '11 at 23:04
2  
I mean, that (1) JSON is standard, so everybody who looks at the code will know what's going on immediately; (2) JS and PHP (and just about all the other languages in common use) have built-in parsers and serializers for it, so you don't need to write your own; and (3) it can handle structures of arbitrary complexity, so you can return something like {"status":"success","items":[{"id":"item1","message":"Hi!"},{"id":"item2","mess‌​age":"another message"},...]} without having to even worry about how you're going to parse that. It'll just be an object or array almost the entire time. –  cHao Nov 10 '11 at 23:26
2  
@stack.user.0 cHao is very wise and has given you very good suggestions. Don't implement a third of what cHao has suggested. Do all of it. JSON, passing DOM elements, removal of minified names, etc. If you only do a third your code is still going to be two thirds bad. Also, change your brace style. –  Paul Nov 11 '11 at 2:39

Your Answer

 
discard

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