Elegant Solution avoiding Global Variable

Answers (4)

Reputation: 12806

No need to do all that coding. You can already hide it from the global by just wrapping the value inside the eventhandler of the load function. Note this will not run, but at least I added a clearInterval after the first error. ;)

This sample also uses the built in fetch method, which works with promises and makes the code a lot more readable.

window.addEventListener('load', function() {
  let results;
  let interval = setInterval(() => {
    fetch('ajax1.php', {
      method: 'POST'
    }).then(response => {
      let count = parseInt(response.text);
      if (results === undefined) {
        results = count;
        return;
      }
      if (results !== count) {
        clearInterval(interval);
        document.location.reload();
      }
    }).catch(() => clearInterval(interval));
  }, 250);
});

A few more things to note about your original code though. :)

window.onload = ajax(anfang);

This statement is useless, as it executes the function at once, so the assignment is useless, it could be written instead as:

window.onload = () => ajax(anfang);

Your XMLHttpRequest method is also a bit overly verbose, note that the async parameter is optional and defaults to true so there is absolutely no reason to supply it as an argument.

Which would execute it after the window has loaded, or you could use the addEventListener as shown in the code above.

Also reloading it if the counts are different seems very resource heavy, wouldn't it be better to reload the data that now has changed?

Concerning the count function, what happens if in the polling interval, an item got added and deleted?

And a final point, if your polling page would throw an error, many errors would occur, considering to either warn the user that the polling didn't work, or to disable the polling so that the user doesn't keep on sending requests if it failed (maybe giving a manual button to refresh, or showing a message to the user that refreshing was disabled).

You could eventually also rewrite your code in the following way

function ajax() {
  var xhttp = new XMLHttpRequest();
  xhttp.onreadystatechange = function() {
    if (this.readyState === 4 && this.status === 200) {
      if (ajax.results === undefined) {
        ajax.results = this.responseText;
      } else if (ajax.results !== this.responseText) {
        document.location.reload();
      }
    }
  };
  xhttp.open("POST", "ajax1.php"); // SELECT COUNT(*)
  xhttp.send();
}

window.addEventListener('load', function() {
  setInterval( ajax, 250 );
});

This uses the ajax function to store the data, as functions are also objects. So the first time it will load the data, it will set the original values, the next times, it will compare the responseText with the value previously saved as a property on the ajax function.

Upvotes: 0

0x6563

Reputation: 1086

You can bind the property to your ajax function, but really the cleanest way would to use Scott Marcus's answer.

function ajax(f) {
  var xhttp = new XMLHttpRequest();
  xhttp.onreadystatechange = function() {
    if (this.readyState == 4 && this.status == 200) {
      f(this.responseText);
    }
  };
  xhttp.open("POST", "ajax1.php", true); // SELECT COUNT(*)
  xhttp.send();
}
ajax.anzahlErg = 0; // This is now a property of the ajax object and not the window object

function anfang(response) {
  ajax.anzahlErg = response;
}

function nachher(response) {
  if (ajax.anzahlErg != 0 && response != ajax.anzahlErg) {
    location.reload();
  }
}

window.onload = ajax(anfang);
  setInterval(function() {
    return ajax(nachher)
}, 250); //Polling Interval

Upvotes: 0

Scott Marcus

Reputation: 65825

Just wrap all your code in an Immediate Invoked Function Expression. This is called the "module pattern" and is very common in JS because functions cause their own scope.

Next, this line:

window.onload = ajax(anfang);

Will actually run ajax(anfang) as soon as it is encountered and not wait for the load event. The way it is written, it says: "Run this function right now and whatever function it returns is what should be registered as the function to call when window.load happens." Instead, you need to wrap that in a function that, itself becomes the handler.

Additionally, you should be using .addEventListener() to register event handlers instead of object properties, such as onload or .onreadystatechange.

(function(){
  var anzahlErg = 0; //<--- THIS IS NOW LOCAL TO THE IIFE

  function ajax(f) {
    var xhttp = new XMLHttpRequest();

    xhttp.addEventListener("readystatechange", function() {
      if (this.readyState == 4 && this.status == 200) {
        f(this.responseText);
      }
    });

    xhttp.open("POST", "ajax1.php", true); // SELECT COUNT(*)
    xhttp.send();
  }

  function anfang(response) {
    anzahlErg = response;
  }

  function nachher(response) {
    if (anzahlErg != 0 && response != anzahlErg) {
      location.reload();
    }
  }

  window.addEventListener("load", function() {
    ajax(anfang)
  });

  setInterval(function() {
    return ajax(nachher)
  }, 250); //Polling Interval
})();

Upvotes: 3

ktilcu

Reputation: 3128

Others have changed your code dramatically and they are good solutions but I thought I'd introduce closures and how to use them in your situation.

<script>
  function ajax(f) {
    var xhttp = new XMLHttpRequest();
    xhttp.onreadystatechange = function() {
      if (this.readyState == 4 && this.status == 200) {
        f(this.responseText);
      }
    };
    xhttp.open("POST", "ajax1.php", true); // SELECT COUNT(*)
    xhttp.send();
  }

  function cachedPoll() {
    var anzahlErg = 0;
    return {
      anfang: function anfang(response) {
        anzahlErg = response;
      },
      nachher: function nachher(response) {
        if (anzahlErg != 0 && response != anzahlErg) {
          location.reload();
        }
      }
    };
  }

  var pollers = cachedPoll();
  window.onload = function() {
    ajax(pollers.anfang);
  };
  setInterval(function() {
    return ajax(pollers.nachher)
  }, 250); //Polling Interval
</script>

I also updated your onload handler to be wrapped in a function instead of assigning the return value to window.onload.

Upvotes: 0

Elegant Solution avoiding Global Variable

Answers (4)

Related Questions