A969cbe277419ae0af0acee58f4cfa15

This submission is from a question I received on Javascript Kata (http://www.javascriptkata.com/2007/12/06/ask-dan-more-on-javascript-threading/). Please read the post first and then try to refactor the code...

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
function startup() {
  setInterval(”pinger(’live’,0)”,10000);
  setInterval(”pinger(’standby’,1)”,10000);
  setInterval(”pinger(’dev’,2)”,10000);
  setInterval(”pinger(’test1′,3)”,10000);
  setInterval(”pinger(’test2′,4)”,10000);
  setInterval(”pinger(’test3′,5)”,10000);
  setInterval(”pinger(’test4′,6)”,10000);
}

function pinger(server,divit) {
  console=document.getElementById(’ping_div’ + divit);
  console.innerHTML=”;
  sendRequest(”../php/ping_sys2.php?target=” + server);
}

Refactorings

No refactoring yet !

Avatar

Casper

December 6, 2007, December 06, 2007 16:45, permalink

No rating. Login to rate!

Are you sure you don't have a race condition sneaking in there somewhere in sendRequest()? I'd like to see the implementation of that method to really make sure. No global variables or something obvious like that?

Bbcf8931a1494ef9ac6b9ca774ef41df

BK

December 6, 2007, December 06, 2007 22:42, permalink

No rating. Login to rate!

I agree with Casper that this looks a lot like the sendRequest method would always use the same XMLHTTP object and so, by getting the alert message, the request has enough time to come back before a new one takes ownership of the onreadystate handler.

425569e03b2ca45336670d8905f2ae6e

Stuart

December 7, 2007, December 07, 2007 11:09, permalink

No rating. Login to rate!

The sendRequest object. Since reading about the global var I have tried removing the "var xmlReq;" line from code and there is no difference in behaviour. The final call still gets executed while the rest stay on "sending". I can certainly see the sense in the suggestion that the onreadystate handler isn't having enough time to complete its execution. 

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
var xmlReq;

function sendRequest(url, params, HttpMethod){

if(!HttpMethod) {
HttpMethod=”POST”;
}

xmlReq=startXMLHTTPRequest();

if(xmlReq) {

xmlReq.onreadystatechange=onReadyState;
xmlReq.open(HttpMethod,url,true);
xmlReq.send(params);
}
}

function startXMLHTTPRequest(){

var req=null;

try {
req=new ActiveXObject(”Microsoft.XMLHTTP”);
}
catch(e)
{
req=new XMLHttpRequest()
}

return req;

}

function onReadyState()
{
var readyXML=xmlReq.readyState;
var data=null;

if(readyXML == 4){
if(xmlReq.status == 200) {
console.innerHTML=”;
data=xmlReq.responseText;
}
else {
data=xmlReq.status;
}
}
else{
console.innerHTML=”;

if(readyXML == 3){
data=”Serving …”;
}
if(readyXML == 2){
data=”Loading …”;
}
if(readyXML == 1){
data=”Sending …”;
}
}

dataInnerHTML(data);
}

function dataInnerHTML(data){

if(data!=null){
var newline=document.createElement(”div”);
console.appendChild(newline);
newline.innerHTML+=”"+data+”";
}

}
Bbcf8931a1494ef9ac6b9ca774ef41df

BK

December 7, 2007, December 07, 2007 15:59, permalink

No rating. Login to rate!

First thing, even though you don't define the global var, since you probably didn't add var in front of xmlReq=startXMLHTTPRequest();
it is still considered a global variable. This means that when you test readyState, you don't necessarily test the one that is really doing the callback but now that I think about it, I don't think the problem is only there. Console is also defined as global! This means that everything is written to the las div called. Maybe you should use closures.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
function startup() {
    setInterval("pinger('live',0)",10000);
    setInterval("pinger('standby',1)",10000);
    setInterval("pinger('dev',2)",10000);
    /*setInterval("pinger('test1',3)",10000);
    setInterval("pinger('test2',4)",10000);
    setInterval("pinger('test3',5)",10000);
    setInterval("pinger('test4',6)",10000);
    */
}

function pinger(server,divit) {
    var data = new Object();
    data.console=document.getElementById('ping_div' + divit);
    data.console.innerHTML="";
    sendRequest("http://www.google.com", undefined, undefined, data);
}


function sendRequest(url, params, HttpMethod, data){
    if(!HttpMethod) {
        HttpMethod="POST";
    }

    var xmlReq=startXMLHTTPRequest();

    if(xmlReq) {
        xmlReq.onreadystatechange=function(){onReadyState(xmlReq, data)}
        xmlReq.open(HttpMethod,url,true);
        xmlReq.send(params);
    }
}

function startXMLHTTPRequest(){
    var req=null;
    
    try {
        req=new ActiveXObject("Microsoft.XMLHTTP");
    }catch(e){
        req=new XMLHttpRequest()
    }
    
    return req;
}

function onReadyState(xmlReq, data){
    
    switch (xmlReq.readyState){
        case 4:
            if(xmlReq.status == 200) {
                console.innerHTML="";
                data.txt=xmlReq.responseText;
            }else {
                data.txt=xmlReq.status;
            }
            break;
        case 3:
            data.txt="Serving …";
            break;
        case 2:
            data.txt="Loading …";
        case 1:
            data.txt="Sending …";
    }
    
    dataInnerHTML(data);
}

function dataInnerHTML(data){
    if(data!=null && data.txt!=null){
        var newline=document.createElement("div");
        data.console.appendChild(newline);
        newline.innerHTML+=""+data.txt+"";
    }
}
B849433e0e1a3f4ca0cf7cc55b8acd53

Casper

December 7, 2007, December 07, 2007 16:54, permalink

No rating. Login to rate!

Like BK said you have global variables all over the place and this breaks the code. You have to think about what happens when your methods are called concurrently. For example "console" will be overwritten by the latest call, since it's a global variable.

But, the number 1 rule of coding is don't reinvent the wheel. XMLHTTPRequests have been done a million times before, and there are very good libraries that wrap this stuff into much easier packages. For example the solution by using the excellent jQuery library:

Html snippet

1
2
3
4
5
6
<!-- Import jQuery -->
<script src="http://jquery.com/src/jquery-latest.js" type="text/javascript"></script>  

<div id="ping_div_live"></div>
<div id="ping_div_standby"></div>

Javascript part

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
// Untested code, but hopefully you get the idea
function startup() {
  ping('live',    10000);
  ping('standby', 10000);
  // ...
}

function ping(hostname, interval) {
  setInterval(function() { do_ajax_ping(hostname); }, interval);
}

function do_ajax_ping(hostname) {
  // Using the jQuery AJAX method here, makes this easy
  $.ajax({
      type: "GET",
      url: "../php/ping_sys2.php?target=" + hostname,
      processData: false,

      // Using a closure to be called on success here
      success: function(data) {  
        // jQuery DOM manipulation. Again..easier like this.
        $("#ping_div_" + hostname).append("<div>" + data + "</div>");
      }
   });
}
425569e03b2ca45336670d8905f2ae6e

Stuart

December 10, 2007, December 10, 2007 14:18, permalink

No rating. Login to rate!

Thanks for the responses ... as I said I wrote my own handler as an exercise in learning. Probably once having done it to understand the process, I should have have switched back to a standard library. I would never have recognised (without your help) the added oo element, which is now my focus for understanding completely. I hate being in a situation where I am dependant on other code without knowing how it works (I'm strange/pedantic like that).

Thanks for your help and I will wander off and delve the object intricacies that have been thrown up.

Your refactoring





Format Copy from initial code

or Cancel