4

I'm trying to create a series of <ul> and <li> to create a directory/file structure to navigate some files created from a table in a dB. The table (tb_lib_manual) contains both files and folders.

If a record has a null entry for fileID then it is a folder not a file. each record has a parentID to show which folder is parent, for files and folders in the root this is 0.

The php code is thus:

class library_folders extends system_pageElement 
{
    private $html = '';
    private $i = 0;
    private $stmtArray = array();
    private $objectArray = array();

    function __construct() 
    {
        parent::__construct();
        $this->nextList();
    }


    function nextList($parentID = 0) 
    {
        $qSQL = 'SELECT * FROM tb_lib_manual WHERE parentID=:parentID';
        $stmtArray[$this->i] = $this->dbConnection->prepare($qSQL);
        $stmtArray[$this->i]->bindValue(':parentID', $parentID, PDO::PARAM_INT);
        $stmtArray[$this->i]->execute();
        if($stmtArray[$this->i]->rowCount() > 0)
        {
            $display ='';
            if($parentID != 0)
            {
                $display = ' style="display:none"';
            }
            $this->html .= '<ul' . $display . '>';
        }


        while ($this->objectArray[$this->i] = $stmtArray[$this->i]->fetchObject()) 
        {
            $this->html .= '<li>' . $this->objectArray[$this->i]->title;
            if($this->objectArray[$this->i]->fileID == null)
            {
                //we have a folder!
                $manualID = $this->objectArray[$this->i]->manualID;
                $this->i ++;
                $this->nextList($manualID);
                $this->i--;
            }
            $this->html .= '</li>';
        }


        if($stmtArray[$this->i]->rowCount() > 0)
        {
            $this->html .= '</ul>';
        }   

        echo $this->html;
    }   
function __destruct()
    {
        parent::__destruct();
    }

}

The problem is when the code returns back to the while loop after calling itself it restarts the loop rather than carrying on where it left off, causing a repeat in the child folder. Is there either a better way to do this or am I doing something wrong!?

Table looks like this:

enter image description here

output like this: '

  • A Manual
  • A Manual
  • Folder 1
  • Nasa exercise
  • A Manual
  • A Manual
  • Folder 1
  • Nasa exercise
'
TPJ
  • 51
  • 4
  • is manualID the same as ParentID? – I wrestled a bear once. Jun 08 '15 at 12:07
  • yes it is sorry should've explained, obviously different columns... – TPJ Jun 08 '15 at 12:09
  • You might place some debug output of the variables like `var_dump($manualID);`. – Henrik Jun 08 '15 at 12:17
  • Hi sorry have no idea how to place output of debug as I can't get my debug to work (aptana studio 3) however the table data looks like this (just edited question) – TPJ Jun 08 '15 at 12:27
  • I'm more interested in what's in `$this->objectArray`. I think we're missing the other important parts of the class – Machavity Jun 08 '15 at 12:32
  • Hi fair comment have added whole class – TPJ Jun 08 '15 at 12:36
  • As a side remark (much to the side and really a point of detail from a semantic-nazi) : better use `
      ` than `
      ` as your list is most probably ordered (alphabetically I guess) and it would make little sense (or at least be highly un-friendly) to have it randomly ordered...
    – Laurent S. Jun 08 '15 at 12:39
  • Thank you all for comments, I've realised the mistake, I think the code works, the problem was where I put the echo, I've moved to the last line of the constructor and bingo! Thanks you basti for your answer, unfortunately I don't understand it - yet... Although I can see that multiple queries on the dB are bad, I wasn't anticipating too much data in the table... – TPJ Jun 08 '15 at 13:00

2 Answers2

1

Fixed the code, very silly mistake, echo in the wrong place...

class library_folders extends system_pageElement 
{
    private $html = '';
    private $i = 0;
    private $stmtArray = array();
    private $objectArray = array();

    function __construct() 
    {
        parent::__construct();

        $this->nextList();

        echo $this->html;

    }


    function nextList($parentID = 0) 
    {
        $qSQL = 'SELECT * FROM tb_lib_manual WHERE parentID=:parentID';
        //echo $this->i;        
        $stmtArray[$this->i] = $this->dbConnection->prepare($qSQL);
        $stmtArray[$this->i]->bindValue(':parentID', $parentID, PDO::PARAM_INT);
        $stmtArray[$this->i]->execute();
        if($stmtArray[$this->i]->rowCount() > 0)
        {
            $this->html .= '<ul>';
        }

        while ($this->objectArray[$this->i] = $stmtArray[$this->i]->fetchObject()) 
        {
            $this->html .= '<li>' . $this->objectArray[$this->i]->title;
            if($this->objectArray[$this->i]->fileID == null)
            {
                //we have a folder!
                $manualID = $this->objectArray[$this->i]->manualID;
                $this->i ++;
                $this->nextList($manualID);
                $this->i--;
            }
            $this->html .= '</li>';
        }


        if($stmtArray[$this->i]->rowCount() > 0)
        {
            $this->html .= '</ul>';
        }   

    }   

    function __destruct()
    {
        parent::__destruct();
    }

}
TPJ
  • 51
  • 4
0

Yes, there are better ways of doing this.

If you fetch the whole tree every time, you might as well load all the nodes into a big array (of objects), put each node's id as index and then loop over the array once to create the parent references by for example

// create a fake root node to start your traversal later on
// only do this if you don't have a real root node 
// which I assume you don't
$root = (object) ["children" => []];

// loop over all nodes
foreach ($nodes as $node)
{
    // if the node has a parent node that is not root
    if ($node->parentId > 0)
    {
         // put it into it's parent's list of children
         $nodes[ $node->parentId ]->children[] = $node;
    }
    else
    {
         // otherwise put it into root's list of children
         $root->children[] = $node;
    }
}

Complexity: You do one query and you have to iterate all your nodes once.

For this to work your nodes need to be objects. Otherwise each assignment to $node->children will create a copy of the assigned node where you wanted a reference.

If you do not want to fetch all nodes, you can go through your tree level by level by creating a list of node ids from the previous level.

function fetchLevel ($nodes, $previousLevelIds)
{
    // get all children from the previous level's nodes
    // I assume $previousLevelIds to be an array of integers. beware of sql injection
    // I refrained from using prepared statements for simplicity
    $stmt = $pdo->query("SELECT id, parentId FROM nodes WHERE parentId IN (".implode(",",$previousLevelIds).")");

    // fetch nodes as instances of stdclass
    $currentLevelNodes = $stmt->fetchAll(PDO::FETCH_OBJ);

    $nextLevelIds = [];
    foreach ($currentLevelNodes as $node)
    {
         // ids for next level
         $nextLevelIds[] = $node->id;

         // parent <-> child reference
         $nodes[ $node->parentId ]->children[] = $node;
    }

    // fetch the next level only if we found any nodes in this level
    // this will stop the recursion
    if ($nextLevelIds)
        fetchLevel($nodes, $nextLevelIds);
}

// start by using a fake root again
$root = (object) ["id" => 0, "children" => []];
$nodes = [0 => $root];
fetchLevel($nodes, [0]);

// or start with a specific node in the tree
$node = $pdo->query("SELECT id, parentId FROM nodes WHERE id = 1337")->fetch(PDO::FETCH_OBJ);
$nodes = [$node->id => $node];
fetchLevel($nodes, [$node->id]);

// or a number of nodes which don't even have to 
// be on the same level, but you might fetch nodes multiple times
// if you it this way

Complexity: Number of queries <= height of your tree. You only iterate each fetched node once.

For displaying the tree as html list you iterate once more:

class Foo {

    public $html;

    public function getList ($nodes)
    {
         // outer most ul
         $this->html = '<ul>';

         $this->recurseList($nodes);

         $this->html .= '</ul>';
    }

    protected function recurseList ($nodes)
    {
        foreach ($nodes as $node)
        {
            $this->html .= "<li><span>".$node->name."</span>";

            if ($node->children)
            {
                if ($node->parentId > 0)
                    $this->html .= '<ul style="display:none">';
                else
                    $this->html .= '<ul>';

                $this->recurseList($node->children);

                $this->html .= "</ul>";
            }

            $this->html .= "</li>";
        }
    }
}

Some unrelated remarks:

  • instead of a hard-coded style="display:none" you could just use a css rule like ul li ul {display:none} to hide all lists below root
  • I split fetching the data from the database and converting the data for displaying into two separate scripts, which is the standard when you develop using MVC. If you don't want to do this run the scripts in succession.
  • PHP itself is a template engine, but consider using another template engine to display your html like Smarty. I personally prefer smarty for it's simpler syntax.
  • stackoverflow answer on how to bind a PHP array to a MySQL IN-operator using prepared statements
  • if you need to fetch subtrees often consider using a special table structure to reduce the number of queries
Community
  • 1
  • 1
Basti
  • 3,998
  • 1
  • 18
  • 21
  • Thanks Basti, fixed my code, but I think I'm starting to see why your solution is better, but I don't understand it fully yet as I'm a novice... – TPJ Jun 08 '15 at 13:07
  • If you are interested to learn and have questions, just ask away. Since you found the problem in your code yourself, put your own answer explaining it and accept it. ;-) – Basti Jun 08 '15 at 13:08