1
\$\begingroup\$

I'm working on a CMS, currently on being able to create static pages, manage menus etc. so.. I have This function that reads from the DB and finds all the static pages, and their position in the menu. I want to know what Do you think.

function update_menu(&$pdo, $filepath){
$qry = "SELECT * FROM pages WHERE top = 1";
$exec = $pdo->query($qry);
$string = "<ul>";
while($row=$exec->fetch(PDO::FETCH_ASSOC)){
    $string.= "<li><a href='?page=".$row['id']."'>".$row['title']."</a>";
        $qry = "SELECT * FROM pages WHERE parent = ?";
        $child_ex = $pdo->prepare($qry);
        $child_ex->bindValue(1, $row['id'], PDO::PARAM_INT);
        if($child_ex->execute()){
            $i=0;
            while($child=$child_ex->fetch(PDO::FETCH_ASSOC)){
                if($i==0){
                    $string.="<ul>";
                }
                $string.="<li><a href='?page=".$child['id']."'>".$child['title']."</a>";
                $i++;
            }
            if($i != 0)
                $string.="</ul>";
        }
    $string.="</li>";
}
$string.="</ul>";
file_put_contents($filepath, $string);

};

Output html:

<ul><li><a href='?page=1'>Page1</a><ul><li><a href='?page=4'>append1</a><li><a href='?page=5'>append2</a></ul></li><li><a href='?page=2'>page2</a><ul><li><a href='?page=6'>append1b</a></ul></li><li><a href='?page=3'>page3</a></li></ul>

SampleOutput

\$\endgroup\$
5
  • \$\begingroup\$ See: codereview.stackexchange.com/a/26810/20611 \$\endgroup\$ Jun 13 '13 at 22:14
  • 1
    \$\begingroup\$ What is the purpose of this CMS? After all, there is a lot of PHP based CMS'es already, so why reinvent another? \$\endgroup\$ Jun 13 '13 at 22:25
  • 1
    \$\begingroup\$ there's not propose, I like to do my own stuff plus its a good way of learning and gaining experience (Im a Cs student) \$\endgroup\$ Jun 13 '13 at 22:40
  • \$\begingroup\$ @CarlosArturoAlaniz A quote a wise man once told me "Why are building your own framework?! Let me guess what you do is too special for one that already exists? No it not, why would you put yourself through what has already been done for you. What ever you build is not special" \$\endgroup\$ Jan 21 '14 at 15:40
  • \$\begingroup\$ @SaggingRufus you guys miss some reflexion. Further pushing your point, why do you even speak since what you've just said has already been said so much times, and so much better then you did? \$\endgroup\$ Feb 18 '14 at 20:10
2
\$\begingroup\$

I think it overall is good, but there's some minor changes that could still quicken this script, namely using fetchAll and foreach instead of fetch and while

using fetchAll will get the values quicker then fetch on every single row. also foreach will prevent you from having to check if you have results, it just won't run if there are not.

also, you prepare the statement multiple times in the huge loop, instead of preparing it outside of the loop and execute() it multiple times

function update_menu(&$pdo, $filepath){
    $qry = "SELECT * FROM pages WHERE top = 1";
    $exec = $pdo->query($qry)->fetchAll(PDO::FETCH_ASSOC);
    $qryChild = $pdo->prepare("SELECT * FROM pages WHERE parent = ?");
    $string = '<ul>' . "\n";
    foreach($exec as $row)){
        $string.= "<li><a href='?page=".$row['id']."'>".$row['title'].'</a>' . "\n";
        $qryChild->execute($row['id']);
        $childs = $qryChild->fetchAll(PDO::FETCH_ASSOC);
        $i=0;
        foreach($childs as $child){
            if($i==0){
                $string.='<ul>' . "\n";
            }
            $string.="<li><a href='?page=".$child['id']."'>".$child['title'].'</a>' . "\n";
            $i++;
        }
    if($i != 0) $string.='</ul>' . "\n";
    $string.='</li>' . "\n";
    }
$string.='</ul>' . "\n";
file_put_contents($filepath, $string);

also, adding some "\n" will give some better readability to you html code

\$\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.