3
\$\begingroup\$

I know that OOP is ubiquitous nowadays. Nonetheless I still write procedural PHP. It now seems to me that I had fallen into a spaghetti code trap, because my code starts to seem meaningless even to me (it's author :)

Can you please tell me how bad is this?

<?php
/**
 * Created by PhpStorm.
 * User: Taras
 * Date: 23.01.2019
 * Time: 20:01
 */

include "db_conn.php";
$currentPage = "list";
include "header.php";

// Quantity of results per page
$limit = 10;

// Check if page has been clicked
if (!isset($_GET['page'])) {
    $page = 1;
} else{
    $page = $_GET['page'];
}

// Initial offset, if page number wasn't specified than start from the very beginning
$starting_limit = ($page-1)*$limit;

// Check if search form has been submitted
if (isset($_GET['search'])) {
    $searchTerm = $_GET['search'];

    $sql = "SELECT company.id, company.name, inn, ceo, city, phone
          FROM company
          LEFT JOIN address ON company.id = address.company_id
          LEFT JOIN contact ON company.id = contact.company_id
          WHERE
            MATCH (company.name, inn, ceo) AGAINST (:searchTerm)
            OR MATCH (city, street) AGAINST (:searchTerm)
            OR MATCH(contact.name, phone, email) AGAINST (:searchTerm)";
    $stmt = $pdo->prepare($sql);
    $stmt->execute(array(':searchTerm' => $searchTerm));

    $total_results_without_limit = $stmt->rowCount();
    $total_pages = ceil($total_results_without_limit/$limit);

    $sql = "SELECT company.id, company.name, inn, ceo, city, phone
              FROM company
              LEFT JOIN address ON company.id = address.company_id
              LEFT JOIN contact ON company.id = contact.company_id
              WHERE
                MATCH (company.name, inn, ceo) AGAINST (:searchTerm)
                OR MATCH (city, street) AGAINST (:searchTerm)
                OR MATCH(contact.name, phone, email) AGAINST (:searchTerm)
                ORDER BY id DESC LIMIT $starting_limit, $limit";
    $stmt = $pdo->prepare($sql);
    $stmt->execute(array(':searchTerm' => $searchTerm));
    // Count number of rows to make proper number of pages

} else { // Basically else clause is similar to the search block except no search is being made
    $sql = "SELECT * FROM company";
    $stmt = $pdo->prepare($sql);
    $stmt->execute();

    // Again count number of rows
    $total_results = $stmt->rowCount();
    $total_pages = ceil($total_results/$limit);
    // And then make a query
    $stmt = $pdo->prepare("
      SELECT company.id, company.name, company.inn,
            company.ceo, address.city, contact.phone
          FROM company
          LEFT JOIN address
          ON company.id = address.company_id
          LEFT JOIN contact
          ON company.id = contact.company_id
          ORDER BY id ASC LIMIT $starting_limit, $limit");
    $stmt->execute();
}
?>

<main class="container">
    <section class="section-search">
        <div class="row col-auto">
            <h2>Найти компанию</h2>
        </div>
        <form action="list.php" method="get">
            <div class="row">
                <div class="form-group col-lg-6 col-md-6 col-sm-12 row">
                    <label class="col-sm-4 col-form-label" for="searchTerm">
                        Поиск
                    </label>
                    <input type="text" class="form-control col-sm-8"
                           id="companyTerm" name="search">
                </div>
                <div class="float-right">
                    <input type="submit" class="btn btn-primary mb-2" value="Поиск">
                </div>
            </div>
        </form>
    </section>


    <section class="section-companies">
        <table class="table table-sm">
            <thead>
            <tr>
                <th scope="col">ИНН</th>
                <th scope="col">Название</th>
                <th scope="col">Генеральный директор</th>
                <th scope="col">Город</th>
                <th scope="col">Контактный телефон</th>
            </tr>
            </thead>
            <tbody>
            <?php
            // Filling the result table with results
            while ($row = $stmt->fetch(PDO::FETCH_ASSOC)) {
                echo "<tr><td scope='row'>" . $row['inn'] . "</td>";
                echo "<td><a href='company.php?id=" . $row["id"] . "'>" . $row['name'] . "</a>" . "</td>";
                echo "<td>" . $row['ceo'] . "</td>";
                echo "<td>" . $row['city'] . "</td>";
                echo "<td>" . $row['phone'] . "</td></tr>";
            }
            ?>

            </tbody>
        </table>
    </section>
    <?php

    // Paginating part itself
    for ($page=1; $page <= $total_pages ; $page++):?>

    <a href='<?php
        if (isset($searchTerm)) {
            echo "list.php?search=$searchTerm&page=$page";
        } else {
            echo "list.php?page=$page";
        } ?>' class="links"><?php  echo $page; ?>
    </a>

    <?php endfor; ?>
</main>
\$\endgroup\$
  • \$\begingroup\$ (Welcome to CodeReview!) Is your main concern with the readability of your code or that there should be a more succinct way to achieve basically the same effect? \$\endgroup\$ – greybeard Feb 6 at 8:14
1
\$\begingroup\$

Code duplication

To be honest - yes, this code is rather messy, mostly due to the code duplication. At least you could've shortened it by reusing the existing SQL, like

$sql = "SELECT company.id, company.name, inn, ceo, city, phone
      FROM company
      LEFT JOIN address ON company.id = address.company_id
      LEFT JOIN contact ON company.id = contact.company_id
      WHERE
        MATCH (company.name, inn, ceo) AGAINST (:searchTerm)
        OR MATCH (city, street) AGAINST (:searchTerm)
        OR MATCH(contact.name, phone, email) AGAINST (:searchTerm)";
$stmt = $pdo->prepare($sql);
// ...
$sql .= " ORDER BY id DESC LIMIT $starting_limit, $limit";
$stmt = $pdo->prepare($sql);

Performance problems

But the biggest problem with this code is not its structure but its ability to kill your server. Getting the number of rows returned by SELECT query is the most misused functinality in PHP.. Aside of the fact it is most of time just pointless, in a case like yours it could be dangerously harmful. Let's take your code, that looks quite simple and innocent:

$sql = "SELECT * FROM company";
$stmt = $pdo->prepare($sql);
$stmt->execute();
$total_results = $stmt->rowCount();

What does this code do? It is merely selecting all rows from the table and sending them to PHP, so it will be able to count them. Imagine there will be 100000 rows, with 1k data in each. It will result in sending 1G of data from database server to PHP. Not only it will consume a lot of resources in the process but most likely it will overflow the memory limit in PHP and make it die with the fatal error.

So you should never ever use rowCount() for pagination. Instead, you must ask a database to get you the number of rows.

Query builder

In order to make this code more maintainable (so every query part would be written only once), we must split the SQL into distinct parts and then use them to construct the particular query:

$select_count = "SELECT count(*) FROM company";
$select_data = "SELECT company.id, company.name, inn, ceo, city, phone
          FROM company
          LEFT JOIN address ON company.id = address.company_id
          LEFT JOIN contact ON company.id = contact.company_id";
$where = "WHERE MATCH (company.name, inn, ceo) AGAINST (:searchTerm)
            OR MATCH (city, street) AGAINST (:searchTerm)
            OR MATCH(contact.name, phone, email) AGAINST (:searchTerm)";
$order_limit = "ORDER BY id DESC LIMIT $starting_limit, $limit";

if (isset($_GET['search'])) {

    $parameters = [':searchTerm' => $_GET['search']];
    $sql_count = "$select_count $where";
    $sql_data = "$select_data $where $order_limit";

} else {

    $parameters = [];
    $sql_count = "$select_count";
    $sql_data = "$select_data $order_limit";
}

$stmt = $pdo->prepare($sql_count);
$stmt->execute($parameters);
$num_results = $stmt->fetchColumn();
$total_pages = ceil($num_results/$limit);

$stmt = $pdo->prepare($sql_data);
$stmt->execute($parameters);
$results = $stmt->fetchAll();

Better separation between HTML and database stuff

The rest is your code is Okay, I would only remove any mention of a database stuff from HTML. So I would rather get all the rows into array first and than just iterate over it in HTML.

        <tbody>
        <?php foreach ($results as $row): ?>
            <tr>
                <td scope='row'><?=$row['inn']?></td>
                <td><a href="company.php?id=<?=$row["id"]?>"><?=$row['name']?></a></td>
                <td><?=$row['ceo']?></td>
                <td><?=$row['city']?></td>
                <td><?=$row['phone']?></td>
            </tr>
        <?php endforeach ?>
        </tbody>
\$\endgroup\$
  • \$\begingroup\$ Great many thanks! You've discovered exactly what I was in doubt of, just couldn't make it clear. Can I ask you to point me at some books/online tutorials that cover topics like code reusage, proper architecture and the web app architecture as well? Because my other code is pretty simple to this :( \$\endgroup\$ – tnsaturday Feb 7 at 4:26

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.