Efficiently creating a menu consisting of multi-level structure

Question

I was asked to modify existing code to allow multi-level page structure. Each page can have one parent, and one page can have multiple children.

I will present following code. My question is is there a more efficient way of doing this? I am hard-coding the number of levels that are reachable.

Is there easier way to do this automatically without hard coding? Recursion perhaps?

function createMenu($parents) {
 foreach ($parents as $parent) {
 $parentName = $this->getPagefromID($parent);
 $kids = $this->areThereKids($parent);
 if ($kids == "yes") {
 echo "<ul class=\"children\">\n";
 $query = "SELECT * FROM pages";
 $result = mysql_query($query) or die('Query failed: ' . mysql_error());
 while ($row = mysql_fetch_assoc($result)) {
 $thisparent = $row['Parent'];
 $name = $row['Name'];
 $id2 = $row['ID'];
 if ($thisparent == $parent) {
 echo "<li><a href=\"edit.php?id=$id2\">" . $name . "</a></li>\n";
 if($this->areThereKids($id2) == "yes") {
 $children = $this->getMyKids($id2);
 foreach($children as $kid) {
 $info = $this->getPersonInformation($kid);
 $kidid = $info[0]["ID"];
 echo "<li><a href=\"edit.php?id=$kidid\">" . $info[0]['Name'] . "</a></li>\n";
 // more sub
 if($this->areThereKids($kidid) == "yes") {
 $children1 = $this->getMyKids($kidid);
 foreach($children1 as $kid1) {
 $info1 = $this->getPersonInformation($kid1);
 $kidid1 = $info1[0]['ID'];
 echo "<li><a href=\"edit.php?id=$kidid1\">-" . $info1[0]['Name'] . "</a></li>\n";
 if($this->areThereKids($kidid1) == "yes") {
 $children2 = $this->getMyKids($kidid1);
 foreach($children2 as $kid2) {
 $info2 = $this->getPersonInformation($kid2);
 $kidid2 = $info2[0]['ID'];
 echo "<li><a href=\"edit.php?id=$kidid2\">--" . $info2[0]['Name'] . "</a></li>\n";
 // No need for 6 levels.
 }
 }
 }
 }
 }
 }
 }
 }
 echo "</ul>\n";
 }
 echo "</li>\n";
 }
}

• hjpotter92: Switch over to using mysqli or pdo instead of mysql_* functions.

Answer 1

• @hjpotter92 makes a good point: mysql_* is deprecated and should be replaced with mysqli_* or PDO. It's probably easier to replace it with the former, but it may help to read this article comparing mysqli and PDO.

• $this->areThereKids() is a terrible method name. It's even worse that it returns "yes" or some other value. Refactor this to $this->hasKids() (or better yet, $this->hasChildren()), returning either boolean true or false.

  • Similarly, $this->getMyKids() could be better named $this->getChildren(), but this is not as egregious as the point above.

• or die(mysql_error()) is tolerable in development code, but should be avoided in production code. Instead, prefer to print a user-friendly error or apologetic message, and log the details separately.

• You are correct in that your code lends itself naturally to recursion. I'm not interested in writing the recursive function for you, but notice in particular that in every level of nesting,

  • you use the same template for output.
  • you have the variables $info and $id.
  • if there are children, you store them in a variable $children and you iterate through them, using the same template and variables.

  I welcome you to try implementing the recursive function on your own, keeping the above three points in mind. When you've done so, you can submit your revised code here for another review!