4
\$\begingroup\$

I have written a small JavaScript function which performs an AJAX request every x seconds.

I was hoping some of you could give it a look over and give me any pointers on redundant code or bad practices.

The idea of the function is to:

  1. Allow for polling to be initiated
  2. Once initiated, poll once every x seconds and perform an AJAX request.
  3. Stop polling once disablePolling() is executed.
  4. Be able to be reused anywhere.

requester.js

function Request() {

this.poll = false;

this.activatePoll = function () {
    this.poll = true;
    this.runPoll();
};

this.disablePoll = function () {
    this.poll = false;
};

this.runPoll = function () {
    var self = this;
    var poll = setTimeout(function () {
        $.ajax({
            url: './request.php',
            success: function (response) {
                console.log(response);
            },
            dataType: "json",
            complete: function () {
                if (self.poll == false) {
                    clearTimeout(poll);
                } else {
                    self.runPoll();
                }
            }
        })
    }, 1000);
};
}

Usage:

index.html

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Requester</title>
</head>
<body>
<h1>Requester</h1>
<div id="requester">
    <input type="button" onclick="$request.test();" value="test">
    <input type="button" onclick="$request.activatePoll();" value="poll">
    <input type="button" onclick="$request.disablePoll();" value="turn off poll">
</div>
</body>

<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.2.0/jquery.min.js"></script>
<script src="js/request.js"></script>
<script>
    $(document).ready(function () {
        $request = new Request();
    });
</script>
</html>

request.php

<?php
echo json_encode($time());
?>

It should also be mentioned that theAajax request will need to also POST some parameters.

\$\endgroup\$
0
5
\$\begingroup\$
  • Your code is simply a GET request, so the shorthand getJSON can be used for this.

  • Instead of success and complete, use the more standard Promise method then.

  • Instead of a recursive setTimeout, use setInterval instead. This avoids your code from rescheduling a callback on each iteration. You can always clear interval timers with clearInterval.

  • When using setInterval, clearing the timer will suffice for stopping the polling. You don't need the boolean flag to tell you to stop.

  • When using setInterval, you don't need an extra function for a recursive call anymore.

  • You also seem to use OOP. It is normally advised to put methods on the prototype and not the instance so that methods are shared per instance and not created per instance.

  • It is usually not advised to use inline scripts. Use addEventListener to bind your click handlers.

  • Booleans are usually prefixed with an is, has or are depending on the grammar. This is to not mistake a property as some other non-boolean value.

  • Put the url and interval in a variable that acts as a constant. That way, you can easily configure as well as spot the values and not dig through code.

  • If ES6 is available for you, self = this can be taken care of by using arrow functions.

In the end, your code could be as simple as

function Request() {
  this.pollTimer = null;
  this.interval = 1000;
  this.url = './request.php';
}

Request.prototype.disablePoll = function () {
  clearInterval(this.pollTimer); 
  this.pollTimer = null;
};

Request.prototype.activatePoll = function () {
  this.pollTimer = setInterval(() => {
    $.getJSON(this.url).then(response => console.log(response))
  }, this.interval);
};
\$\endgroup\$
8
  • \$\begingroup\$ The only issue with setInterval and ajax is the order of console.log is not guaranteed to be sequential. \$\endgroup\$
    – Brad M
    Mar 3 '16 at 18:41
  • 2
    \$\begingroup\$ thank you Joseph. 1 question though. By using setInterval() and then() instead of setTimeout(). Will this still run the next request only after the previous request has completed? \$\endgroup\$
    – ayala
    Mar 3 '16 at 19:09
  • \$\begingroup\$ it should also be mentioned that the ajax request will need to also POST some parameters. \$\endgroup\$
    – ayala
    Mar 3 '16 at 19:11
  • \$\begingroup\$ @BradM But then, no one leaves console.log in production code. :D \$\endgroup\$
    – Joseph
    Mar 3 '16 at 20:07
  • \$\begingroup\$ @ayala Your question made no mention doing a sequential poll. Your question was "poll once every x seconds and perform an AJAX request" and "function which performs an AJAX request every x seconds." \$\endgroup\$
    – Joseph
    Mar 3 '16 at 20:09
1
\$\begingroup\$

I prefer to do object definition in an IIFE, unless you're willing to use Babel or some other transpiler. I've included comments detailing other thoughts on certain pieces. I also agree with Joseph's recommendations too so I did my best to try not to repeat much (if any) of them. Here is what your Request definition would look like:

var Request = (function() {
  function Request(url, interval) {
    this.poll = false;
    this.interval = null;
    this.url = url;
    this.interval = interval || 1000;
  }

  var proto = Request.prototype;

  // other potentially better names could be 'startPolling', 'start' or just 
  // 'poll'
  proto.activatePoll = function() {
    this.poll = true;
    this.runPoll();
  };

  // other potentially better names could be `stopPolling`, `end` or `stop`, 
  // `endPolling`
  proto.disablePoll = function() {
    this.poll = false;
  };

  // other potentially better names could be `run`, `executed`
  // also these feels "private" so I'd prefix it with an _ like _runPoll just
  // to signify "you probably don't need to use this"
  proto.runPoll = function() {
    // I prefer _this over self because `this` is a known value in JS and self
    // is just a name. Admittedly _this is a name too but it's still 
    // distinguishable
    // also, group var declarations. No reason to see `var` scattered in a 
    // function. vars are function level declarations anyways so regardless of
    // where you define them they're accessible in function scope -- putting 
    // them first and gruoped is a clear way to express this
    var _this = this,
        poll;

    if (!this.poll) {
      // If this gets called when we're not polling, we don't need to create 
      // a timeout.
      return;
    }

    poll = setTimeout(function() {
      $.ajax({
        url: _this.url,
        success: function(response) {
          console.log(response);
        },
        dataType: "json",
        complete: function() {
          // we don't need to test here, because we're returning from runPoll
          // if we're not polling.
          _this.runPoll();
        }
      });
    }, _this.interval);
  };

  return Request;
})();

edit

I did alter your API contract though, you really should think about reusability when you're building "classes." In this case, you take a required first parameter which is the URL you wish to request. There is an optional second argument that sets the interval but will specify to 1000 if none is given. This gives you flexibility and easy reusability: new Request("/request.php") or new Request("/poll_less_often.php", 5000).

\$\endgroup\$
2
  • \$\begingroup\$ Thank you izuriel for your answer and information. Very helpful. I have adjusted my code in the original post. I noticed you have used "var proto = Request.prototype;" rather than "Request.prototype.disablePoll = function () ...". What difference / improvement can this make? Or if easier, what is this called and ill do some googling. \$\endgroup\$
    – ayala
    Mar 4 '16 at 17:50
  • \$\begingroup\$ Well. It reduces keystrokes so it's much easier to type repeatedly, it's also what a mini fire would do (with smaller variables and less white space). But that's really preference. Do whichever makes you feel better. \$\endgroup\$
    – izuriel
    Mar 5 '16 at 2:42
1
\$\begingroup\$

There are a lot of edits here so hopefully I successfully sifted through to what you're looking for.

Personally I liked your original approach better than your edit with prototypes. Both are perfectly fine but I like to put everything in the object rather than prototyping so as to know for sure I've got everything in one place. I use prototypes when I'm extending built-in date functionality or other libraries.

To make your code more portable I recommend treating it like a starting point; one that can be extended and modified as necessary. In my version below I give the option to override certain default values when constructing the object. You're not always going to be talking to 'request.php' every 2 seconds. This will be especially important if you minify your code. You don't want to have to sift through that code to adjust defaults. It's also good for if you want to have more than one poll running at once. Being able to initialize more than one object with different properties will allow that.

Here's what I would do...no jquery required.

function Request(url,seconds){
    var self = this;
    this.pollId = null;
    this.busy = false;
    this.data = null;
    //have a default that can be overridden with construction parameter
    this.url = "./request.php";
    if(typeof(url) !== 'undefined' && url !== null){
        this.url = url;
    }
    //have a default that can be overridden with construction parameter
    this.seconds = 2000;
    if(typeof(seconds) !== 'undefined' && seconds !== null){
        this.seconds = seconds;
    }

    this.startPoll = function(){
        this.pollId = setInterval(self.postAJAX,this.seconds);
    };

    this.stopPoll = function(){
        clearInterval(this.pollId);
        this.pollId = null;
    };
    //to be called on polling completion - can be overridden if needed
    this.success = function(response){
        console.log(response);
    };

    this.postAJAX = function(){
        //check if we're in the middle of a request
        if(this.busy){
            return;
        }
        //we're not so set the busy flag
        this.busy = true;
        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");
        }
        var data = "json="+encodeURIComponent(this.data);
        //Send the proper header information along with the request
        xmlhttp.open("POST", this.url, true);
        xmlhttp.setRequestHeader("Content-type", "application/x-www-form-urlencoded");
        xmlhttp.setRequestHeader("Content-length", data.length);
        xmlhttp.setRequestHeader("Connection", "close");
        //state change handler
        xmlhttp.onreadystatechange = function(){
            if(xmlhttp.readyState == 4 && xmlhttp.status == 200) {
                //clear the busy flag
                self.busy = false;
                //log the response or whatever other action you want
                self.success(xmlhttp.responseText);
            }
        }
        xmlhttp.send(data);
    };
}
\$\endgroup\$
2
  • \$\begingroup\$ You could use " url = url | './request.php' " for the parameter-check / assigning default-values. The same concerning "seconds". Or is there a special reason because the usage of if is more appropriate here? \$\endgroup\$ Mar 7 '16 at 12:39
  • \$\begingroup\$ Since your going to manually implement the XHR handling jQuery would provide for you you may as well add sniffing and support for the newer and much easier to use fetch API, or provide a polyfill for it. Otherwise there is really no point to teach an outdated method for handling XHR requests that every library and it's predecessor has wrapped for ages now. \$\endgroup\$
    – izuriel
    Mar 7 '16 at 17:51

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.