2
\$\begingroup\$

The Problem

We are a textiles manufacturing plant with many PLCs on an automated line. We would like to create a display accessible over the Internet which will display key performance indicators in an easily digestible format.

I am a novice level web developer/junior level SQL DBA/junior level systems administrator acting as the primary net/sysadmin and DBA for our US site.

I am trying to figure out how to solve the problem preferably without having to learn a completely new language. I am experienced in PHP, so I have been trying to use that.

Basically, I have managed to get the system working, but I want to understand if this is a poor solution and/or if there is a better solution that I should work towards.

Please take a minute to browse through the following code and let me know your thoughts.

The Solution

The general concept is that individual queries will be stored in individual includes files and will be reloaded periodically using javascript.

index.php

<!DOCTYPE HTML>

<html>

<head>

    <title>Performance Display</title>

    <link href="https://fonts.googleapis.com/css?family=Noto+Sans" rel="stylesheet">

    <link rel="stylesheet" type="text/css" href="styles/normalize.css" />
    <link rel="stylesheet" type="text/css" href="styles/main.css" />

</head>

<body onload="updateClock(); setInterval('updateClock()', 1000);
              loadData(); setInterval('loadData()', 30000);">

<div id="container">

    <header>
        <div id="product">&nbsp;</div>
        <div id="data">&nbsp;</div>
        <div id="clock">&nbsp;</div>
    </header>

    <table id="product_table">
        <thead>
            <tr>
                <th>&nbsp;</th>
                <th>ACTUAL</th>
                <th>STANDARD</th>
            </tr>
        </thead>
        <tbody>
            <tr>
                <td id="output_actual">&nbsp;</td>
                <td id="output_standard">&nbsp;</td>
            </tr>
            <tr>
                <td id="speed_actual">&nbsp;</td>
                <td id="speed_standard">&nbsp;</td>
            </tr>
        </tbody>
    </table>

</div>

</body>

<script src="https://code.jquery.com/jquery-1.10.2.js"></script>
<script type="text/javascript" src="scripts/functions.js"></script>

</html>

functions.js

function updateClock() {

    // This is a standard clock function removed to avoid unnecessary clutter.

};

function loadData() {
    $("#date").load("includes/date.php");
    $("#product").load("includes/product.php");
    $("#output_actual").load("includes/output_actual.php");
    $("#speed_actual").load("includes/speed_actual.php");
    $("#output_standard").load("includes/output_standard.php");
    $("#speed_standard").load("includes/speed_standard.php");
};

product.php

<?php

include_once("connections.php");

    $sql = "SELECT TOP 1 * FROM Tbl_Perf_Data_Help ORDER BY Date_Time DESC";
    $stmt = sqlsrv_query($productionUSA->connect(), $sql);

    while($obj = sqlsrv_fetch_object($stmt)) {
        echo "PRODUCT ".$obj->Current_Product;
    }

connections.php

<?php

class connection {

    private $serverName;
    private $connInfo;
    public $conn;

    public function __construct($serverName, $db, $usr, $pwd) {

        $this->serverName = $serverName;
        $this->connInfo = array("Database"=>$db, "UID"=>$usr, "PWD"=>$pwd);

    }

    public function connect() {

        $this->conn = sqlsrv_connect($this->serverName, $this->connInfo);

        if (($this->conn === false) or (sqlsrv_begin_transaction($this->conn) === false)) {
            die(print_r(sqlsrv_errors(), true));
        }

        return $this->conn;
        sqlsrv_close($this->conn);

    }

}

$productionUSA = new connection("dbserv", "productionDB", "admin", "CorrectHorseBatteryStaple");

Final Thoughts

As you can see, each piece of dynamic data is being reloaded as a php page every 30 seconds. The reason I think this might be inelegant is that the complete code for the performance display has about 58 fields. That's 58 seperate php files with potentially 58 different connections to our database, if I understand that correctly.

Any guidance or suggestions would be greatly appreciated!

\$\endgroup\$
2
  • \$\begingroup\$ I am not PHP guy, so I can't help with that, but as a general notion, a better way is to push changing data to the browser with websockets. Something like socketo.me is probably worth looking into. \$\endgroup\$ Mar 8 '17 at 16:27
  • \$\begingroup\$ I will look into this! This sounds more like what I would want anyway. I'm particularly concerned with hitting our SQL DB with 58 transactions at once. \$\endgroup\$ Mar 8 '17 at 17:51
1
\$\begingroup\$

I agree with comment to post about potentially investigating web sockets. I guess the decision on what approach fits best would really be based on how frequently your data changes.

Even in thinking about the AJAX approach, I would say the same thinking also applies to how you decide to call the backend. Does each component in the UI have data that changes with different frequencies? If the components are highly related perhaps it makes sense to just call a single service endpoint every X seconds to get a single data feed you can use to update. Conversely, if these outputs all change with different frequencies, maybe they should each have their own polling intervals.

In the latter case, you may even go so far as to begin to think of applying a more proper component architecture in the UI, where each component has it's own view definition, data bindings, etc. There are a number of UI/javascript frameworks out there that are designed specifically for this use case - React, Polymer, Angular, etc.

That being said, what you have works, and so long as you do not have a bunch of browsers open and you have your backend scaled appropriately, the number of queries you are making against the database should not be problematic. Since you are loading each component asynchronously now, you might even consider adding a "jitter" to your $.load() calls so as to make the query load less spiky against the database.

For example:

$("#date").load("includes/date.php");
setTimeout(
    $("#product").load("includes/product.php"),
    1000
);
setTimeout(
    $("#output_actual").load("includes/output_actual.php"),
    2000
);
// etc.

You should move away from hard-coding javascript behaviors into your HTML. You instead of specifying an onload property on your body, include a javascript file like:

$(document).ready(function() {
    updateClock();
    setInterval('updateClock()', 1000);
    loadData();
    setInterval('loadData()', 30000);
});

Where are you just echoing objects in you ajax responses? Should you be wrapping in HTML (since you are directly $.loading into DOM, or use json_encoding() are something similar to give you an actual formatted response?


$("#date").load("includes/date.php");

Why are "includes" exposed publicly? Is this really an "include" at this point or actually a true API endpoint?

Can I call includes/connections.php and spawn a bunch of zombie connections to your DB?


Don't hardcode database credentials in your class files. In fact you should not even be setting up a actually setting up a DB connection in a class file.

In general, you don't want to mix code that defines logic in you application (functions, classes, etc.) with code that actually changes the state of your application (by calling functions, instantiating classes, rendering output, etc.) in the same file like you are doing in connections.php.


I don't understand purpose for your connection class. It doesn't really do anything. It looks like an improperly implements singleton. But right now, it does nothing to prevent spawning unnecessary connections to the database.

If you wish to keep it, give it a meaningful name. connection is very generic and gives no hint that this is intended to work with SQL server.


Make up your mind on whether you are trying to work with sqlsrv_* functions in an object-oriented manner or not. You start to provide an object wrapper around it in your connection class. But then just work with it in a procedural fashion.

I might actually suggest you dump these procedural methods and just work with PDO with PDO_SQLSRV extension. You would then be working in a much more modern paradigm.


die(print_r(sqlsrv_errors(), true));

Don't output errors directly to standard out (browser) from within a class such as this. This class is designed to connect to a database, not output to the end user. Leave the error messaging to logic further upi the call stack that is better positioned to provide a more user friendly message.

You current approach can also be a huge security vulnerability.


    return $this->conn;
    sqlsrv_close($this->conn);

The last line of code here is unreachable. return represents the last line of code that will be executed from within that code path in your function/method. Any lines of code after it in that code path are unreachable. You should never write unreachable lines of code.

It is probably good that this line is unreachable, is if the order of this lines was reversed you code would break. Why are you trying to close a connection right after opening it anyway?


   if (($this->conn === false) or (sqlsrv_begin_transaction($this->conn) === false)) {

Why are you trying to start a transaction here? Shouldn't this class only be concerned with getting the connection established? Do you not trust that the return value of sqlsrv_connect() is valid?

I don't see that you are working with transactions at all in your other code, so wouldn't the happy path execution of this code lead to a bunch of unnecessarily open transactions?

\$\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.