3
\$\begingroup\$

I've written a simple script to refresh the page when the page has been updated.

How it works:

The Javascript polls a PHP script and sends it the current URL and the current version, the PHP script then queries MySQL which returns the true version of the page.

Here is the PHP

if (isset($_GET['URL']) && isset($_GET['CurrentVersion'])) {

$URL = mysqli_real_escape_string($DBConnection, $_GET['URL']);
$CurrentVersion = mysqli_real_escape_string($DBConnection, $_GET['CurrentVersion']);

$Query = "SELECT ID, URL, CurrentVersion FROM PageUpdates WHERE URL = '$URL'";
$Query = $DBConnection->query($Query);

if ($Query->num_rows == 1) {
    $Row = $Query->fetch_assoc();
    if ($Row['CurrentVersion'] > $CurrentVersion) {
        echo "TRUE";        
    } else {
        echo "FALSE";
    }
} else {
    echo "FALSE";
}
}

and here is the Javascript

CheckForUpdate();
function CheckForUpdate() {
var xhttp = new XMLHttpRequest();
xhttp.onreadystatechange = function() {
    if (this.readyState == 4 && this.status == 200) {
        if (this.responseText == "TRUE") {
            document.location.reload(true);
        } else {
            setTimeout(function() { CheckForUpdate() }, 50000);
        }
        console.log(this.responseText);
    }
};
xhttp.open("GET", "/IsPageUpdated.php?URL="+ window.location+"&CurrentVersion="+CurrentWebpageVersion, true);
xhttp.send();
}

CurrentWebpageVersion is defined in a script in the HTML so I can set it with PHP.

\$\endgroup\$
  • \$\begingroup\$ So what is your question? \$\endgroup\$ – r10y Apr 13 '17 at 23:51
1
\$\begingroup\$

JAVASCRIPT

Calling function before defining it

Invoking CheckForUpdate() before actually defining it, while perfectly valid in JavaScript, is generally a bad practice and it makes code harder to follow. It can also cause troubles in some rare cases.

Style

  • Function names should follow camelCase convention.
  • Use strict equality / identity operator (===) instead of regular equality operator (==). That way no type conversion will be performed and following this practice, you are less likely to hurt yourself in the future.
  • Spaces around +.

onreadystatechange property

Use addEventListener() with readystatechange argument instead. That way you could define multiple listeners of that type if necessary.

setTimeout() function

Firstly, escaping CheckForUpdate() into anonymous function to pass it as an argument to setTimeout() is unnecessary. This

setTimeout(function() { CheckForUpdate() }, 50000);

could be simply written as

setTimeout(CheckForUpdate, 50000);

Lastly, instead of setting timeout at every function call, you could call setInterval() once, outside the function.

Encode

Use encodeURI() function to encode whatever you pass as URL parameters.

Approach of constant querying the server

Constantly querying the server to know whether update is necessary or not, is rather far from the best possible thing to do. It would be way better to be able to get the message directly from the server, once it is necessary. This is where WebSockets come in handy. After linked MDN (emphasis mine):

WebSockets is an advanced technology that makes it possible to open an interactive communication session between the user's browser and a server. With this API, you can send messages to a server and receive event-driven responses without having to poll the server for a reply.

Respond with JSON (ᴀʀɢᴜᴀʙʟᴇ)

If you responded with {"update": true} instead of TRUE, you could use it later as if (JSON.parse(this.responseText).update) instead of if (this.responseText == "TRUE"). That way you could access values of response as properties of an object in the future. If this doesn't seem necessary, it may be an overkill, though.

Rewrite

After applying all the above suggestions (and more), the code could look like this:

function onUpdateStateChange() {
  if (this.readyState === 4 && this.status === 200) {
    if (JSON.parse(this.responseText).update) {
      return document.location.reload(true);
    }
    console.log(this.responseText);
  }
}

function checkForUpdate() {
  var request = new XMLHttpRequest(),
      address = '/IsPageUpdated.php?URL=' + window.location + '&CurrentVersion=' + CurrentWebpageVersion;

  request.open('GET', encodeURI(address), true);
  request.addEventListener('readystatechange', onUpdateStateChange);
  request.send();
}

setInterval(checkForUpdate, 50000);

PHP

Potential SQL injection

$URL = mysqli_real_escape_string($DBConnection, $_GET['URL']);
$Query = "SELECT ID, URL, CurrentVersion FROM PageUpdates WHERE URL = '$URL'";

Well, mysqli_real_escape_string() is not the preferred method of passing variables to SQL statements. You could hurt yourself, e.g. if you didn't explicitly defined character set to the right one. Use prepared statements and mysqli_prepare() instead.

Style

You have a few style problems, i.a. indentation and naming issues.

| improve this answer | |
\$\endgroup\$

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.