1
\$\begingroup\$

I am wanting to use 1 page in my Joomla site to display data. How the criteria will work is EITHER the hire date and term date will be passed into the page and we can capture using $GET_(), OR if the dates are null on page load then we need to capture them from the date pickers on the page. This is the syntax that I have, but I am repeating the syntax/query in two places, and I am sure there is a way to consolidate this.

How could this syntax be optimized?

<html>
<?php
$hiredate = $_GET['hdate'];
$termdate = $_GET['tdate'];
$option = array();
$option['driver'] = 'mssql';
$option['host'] = 'Server';
$option['user'] = 'User';
$option['password'] = 'Pass';
$option['database'] = 'DB';
$option['prefix'] = '';
$db = JDatabase::getInstance($option);
$sql = $db->getQuery(true);
$sql = "SELECT DISTINCT employeeName FROM Currentemployees ORDER BY employeeName ASC";
$db->setQuery($sql);
$sql = $db->loadObjectList();
?>
<body>
    <form method="POST">
        Select a employee:
        <select name="employee" id="employee">
            <option selected="selected">All</option>
            <?php
            foreach ($sql as $name) {
                ?>
                <option value="<?= $name->employeeName ?>"><?= $name->employeeName ?></option>
                <?php
            };
            ?>
        </select>
        Hire Date:<input type="date" name="Hiredate" value="<?= $_POST['Hiredate'] ?>">
        Term Date:<input type="date" name="Termdate" value="<?= $_POST['Termdate'] ?>">
        <input type="submit" name="submit" value="Get Info">
    </form>
</body>
<?php
//date is null so we need to capture it from the date picker
if (!isset($_GET['hiredate']) || is_null($_GET['hiredate'])) {
    $Hiredate = $_POST['Hiredate'];
    $Termdate = $_POST['Termdate'];
    if (isset($_POST['submit'])) {
        if (isset($Hiredate) && isset($Termdate)) {
            $option = array();
            $option['driver'] = 'mssql';
            $option['host'] = 'Server';
            $option['user'] = 'User';
            $option['password'] = 'Pass';
            $option['database'] = 'DB';
            $option['prefix'] = '';
            $db = JDatabase::getInstance($option);
            $query = $db->getQuery(true);
            $query->select($db->quotename(array('employee,empID,hiredate,termdate')));
            $query->from($db->quoteName('employeeinfo'));
            $datefield_name = $db->quoteName('hiredate');
            $query->where("$datefield_name >= " . $db->quote($Hiredate), 'AND');
            $query->where("$datefield_name <= " . $db->quote($Termdate));
        } else {
            echo "Please check the selection criteria and process again.";
        }
        $db->setQuery($query);
        $query = $db->loadObjectList();
    }
if (isset($_GET['hiredate']) {
            $option = array();
            $option['driver'] = 'mssql';
            $option['host'] = 'Server';
            $option['user'] = 'User';
            $option['password'] = 'Pass';
            $option['database'] = 'DB';
            $option['prefix'] = '';
            $db = JDatabase::getInstance($option);
            $query = $db->getQuery(true);
            $query->select($db->quotename(array('employee,empID,hiredate,termdate')));
            $query->from($db->quoteName('employeeinfo'));
            $datefield_name = $db->quoteName('hiredate');
            $query->where("$datefield_name >= " . $db->quote($Hiredate), 'AND');
            $query->where("$datefield_name <= " . $db->quote($Termdate));
        } else {
            echo "Please check the selection criteria and process again.";
        }
        $db->setQuery($query);
        $query = $db->loadObjectList();
}
    ?>
    <div id="dvdata">
        <table id="example" >
            <thead>
                <tr>
                    <th>employee </th>
                    <th>employee id </th>
                    <th>Hire Date </th>
                    <th>Term Date Date</th>
                </tr>
            </thead>
            <?php
            foreach ($query as $res) {
                print "<tr>";
                print "<td>" . $res->employee . "</td>";
                print "<td>" . $res->employeeid . "</td>";
                print "<td>" . date('m-d-Y', strtotime($res->hiredate)) . "</td>";
                print "<td>" . date('m-d-Y', strtotime($res->termdate)) . "</td>";
                print "</tr>";
            }
        }
        ?>
        </tbody></table></div></html>
\$\endgroup\$
1
  • \$\begingroup\$ Oddly, the best place to get a review might actually be Joomla Stack Exchange because they will be able to inform you on the do's and don't's within the scope of the Joomla CMS. For starters, you shouldn't be doing any $_GET or $_POST within the CMS. Most glaring is the procedural scripting among Joomla's OOP code base. \$\endgroup\$ – mickmackusa Apr 30 '20 at 15:03
2
\$\begingroup\$

First of all, you need to initialize your DB in a separate method/function, preferably in an external file. Then separate template from code.

Make the template included in the code only once. Mixing templates with processing is bad practice. Have a look on the web for MVC programming pattern and examples of its use.

If you do not make a difference between a GET and a POST request here, you may use $_REQUEST for query parameters. It combines the values in $_GET and $_POST together. Actually for querying database, GET is preferable.

So far, you need to structure and rework your code so that there are no redundant blocks repeated. Then you are welcome again to submit your code when it is more readable.

\$\endgroup\$
0
\$\begingroup\$

TimSparrow has good points- the database connection lines should be consolidated and perhaps moved to a separate function. The options, especially the credentials, can and perhaps should be moved to a configuration file. I haven't used Joomla before but have used similar libraries like Laravel, where database credentials were kept out of the repository and stored in configuration files.

Suggestions

Abstracting Database connection code

Each time the following lines appear, those could be moved to a function or static class method, which could perhaps cache the connection:

class DatabaseConnection {
    static $connection;
    public static function getConnection() {
        if (!isset(static::$connection)) {
            //these options - especially the credentials, could come from an included environment file
            $option = array();
            $option['driver'] = 'mssql';
            $option['host'] = 'Server';
            $option['user'] = 'User';
            $option['password'] = 'Pass';
            $option['database'] = 'DB';
            $option['prefix'] = '';
            static::$connection = JDatabase::getInstance($option);
        }   
        return static::$connection;
    }
}

Then utilize that wherever a connection is needed:

$db = DatabaseConnection::getConnection();
$sql = $db->getQuery(true);

This is known as the Singleton Pattern

<body> closed before last table

In the first block of HTML, the <body> tag is closed:

    </form>
</body>

Which then puts subsequent HTML outside the body tag - e.g.

<div id="dvdata">
    <table id="example" >

... and so on...

So move that closing body tag (i.e. </body>) right before the closing HTML tag:

</tbody></table></div></body></html>

Questions

  1. Are there really separate query string parameters hdate and hiredate, that are different than the form field value Hiredate (i.e. two different variables in $_GET and another in $_POST?
  2. Also, is the second block (i.e. if (isset($_GET['hiredate']) {) supposed to use the values from POST data instead of the query string?
\$\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.