2
\$\begingroup\$

For a small blog with a limited count of categories, I wrote this code to fetch categories and subcategories from the database.

I used a recursive function to call all categories and sub categories , my goal since I do not have a lot of results is to get them all and display them.

So here is my code. I wish to get your feedback about it.

<?php
//mysql cat table table 
/*

  CREATE TABLE `cat` (
  `id` int(255) NOT NULL,
  `id_parent` int(255) DEFAULT NULL,
  `name` varchar(255) NOT NULL,
  `descr` varchar(1000) NOT NULL
  )

 */
//preparing dsn var for PDO 
$dsn = 'mysql:host=localhost;dbname=dbname';

//instantiating pdo object 
$pdo = new PDO($dsn, "dbuser", 'dbpass');

//make a function take sql and pdo object 
function getCats($sql, $pdo) {
    //preparing sql statment 
    $stmt = $pdo->prepare($sql);
    //executing our query
    $stmt->execute();
    //saving results in array
    $array = $stmt->fetchAll();
    //checking if array siez is zero we return 1;
    if ($array == []) {
        return 1;
    }
    //var_dump($array);
    //outputting html list for all our cats 
    echo "<ul>";
    for ($i = 0; $i < count($array); $i++) {
        echo "<li>";
        echo $array[$i]['name'];
        echo "</li>";
        //calling recursive function to get sub cats 
        getCats("SELECT * from cat where id_parent = {$array[$i][0]}", $pdo);
    }
    echo '</ul>';
}

//calling the function 
getCats("SELECT * from cat where id_parent is NULL", $pdo);
\$\endgroup\$
5
\$\begingroup\$

In no particular order:

Your id_parent field is nullable, and you use this fact to flag the category as having no parent. This forces you to treat the field differently id_parent is NULL verse id_parent = .... If you used a magic number, such as 0 to represent no parent, you could treat top level and child categories similarly.

You use a prepared statement, but create the SQL dynamically with "... where id_parent = {$array[$i][0]}". This is poor design. Use prepared statements with bound parameters to prevent SQL injection & gain efficiency.

When you query your table, you retrieve all columns of data, but you are only using the name column and the 0th column. Use "select id, name from..." to just retrieve the data you are going to use. Not only do you gain efficiency, you document the code with the fields you will be using, and gain resilience against table structure changes that change the column indices.

The name column and the 0th column?? Be consistent; don't mix names and column numbers.

You are querying your SQL table multiple times, once for each possible category parent. With n categories, you query the table n+1 times! With such a small table, it would be better to query the table once, store the data in in-memory arrays, and then print the data from the in-memory arrays.

Why return 1;? Nothing is returned when the function exits normally. You could just return;. Or better, reverse the test, put the print loop in the then clause, and don’t have any embedded returns!

Your SQL table should list id as primary key and possibly id_parent as foreign key references cat(id).


Sample code:

Building up the category ($cat) array with a single SQL query:

$stmt = $pdo->prepare("select id, name, id_parent from cat");
$stmt->execute()
$result = $stmt->fetchAll();

$root_id = NULL;
$cat = [$root_id => [ 'name' => '', 'children' => []]];

# Create $cat[id] entries for each row in table, with empty children list
foreach($result as $row) {
    $cat[$row['id']] = [ 'name' => $row['name'], 'children' => [] ];
}

# Add each item to its parent's children list
foreach($result as $row) {
    $cat[$row['id_parent']]['children'][] = $row['id'];
}

Displaying the categories from in-memory array:

function showCat($cat, $id) {
    echo $cat[$id]['name'];
    $children = $cat[$id]['children'];
    if ($children) {
        echo "<ul>";
        foreach($children as $child_id) {
            echo "<li>";
            showCat($cat, $child_id);
            echo "</li>";
        }
        echo "</ul>";
    }
}

showCat($cat, $root_id);
\$\endgroup\$
  • 2
    \$\begingroup\$ btw, the query could be run in a single line $result = $pdo->query("select id, name, id_parent from cat")->fetchAll(); \$\endgroup\$ – Your Common Sense Jun 20 '18 at 7:09

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.