5
\$\begingroup\$

I'm working on a simple CMS. It's working quite ok, but I think my code can be improved a bit. I work with a section.php, which is included from the index.php with (example):

switch($_GET['key'])
{
case '': $page='section'; $id='1'; break;
case 'home': $page='section'; $id='1'; break;
}
include ('header.php');
include ($page.'.php');
include ('footer.php');

But I would like some feedback on my section.php code. I have very little knowledge of OOP, but if possible I would like to use it for section.php. I used it a little for $validator which checks for clean URLs. But I have trouble with re-using the same $sql_queries.

section.php:

<?php


        #-------------------------------------------#
        # SIDEBAR                                   #
        #-------------------------------------------#

echo "<div id='sidebar'>";
$result = mysql_query('SELECT ID,title,tagline FROM cmsarticles WHERE section = '.$id.' ORDER BY id ASC LIMIT 4');
    while ($row = mysql_fetch_array($result)){
    $path = 'photos/';
    $safeid = $validator->clean_url($row['title'].'-'.$row['ID']);    
    $get_photo = "SELECT name,artId,posId FROM images WHERE artId = ".$row['ID']." AND posId = 2 LIMIT 3";
    $photothumbs = mysql_query($get_photo) or die(mysql_error());
        while($photothumb = mysql_fetch_array($photothumbs)){
        echo '<a rel="group_image" href="photos/'.$photothumb['name'].'"><img class="photo" src='.$path.'thumb_'.$photothumb['name'].' title="'.$row['title'].'" width="250"></a>'; 
        }
    }
echo "</div>";

        #-------------------------------------------#
        # CONTENT                                   #
        #-------------------------------------------#

echo "<div id='content'>";
$query= mysql_query('SELECT cmsarticles.ID AS artID, cmsarticles.title, cmsarticles.tagline, cmsarticles.thearticle, cmsarticles.section, cmssections.ID, cmssections.name 
FROM cmsarticles, cmssections 
WHERE cmsarticles.section = cmssections.ID 
AND cmsarticles.section ='.$id.' ORDER BY cmsarticles.ID ASC');
$num_results = mysql_num_rows($query); 
if ($num_results == 0){ 
echo 'Nothing here at the moment.';
}else{
    while($result = mysql_fetch_array($query)) {
    $safeid = $validator->clean_url($result['title'].'-'.$result['artID']); 
    echo "<div id=id".$result['artID']." class='article'>
    <h1>".$result['title']."</h1>";
    $query2 = mysql_query('SELECT name,artId,posId FROM images WHERE artId = '.$result['artID'].' AND posId = 1 LIMIT 1');
        while($result2 = mysql_fetch_array($query2)){
        echo '<img src=photos/'.$result2['name'].'>';
        }
    echo"<p>".$result['thearticle']."</p>
    </div>";
    }
}
echo "</div>";


?>
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

I'd rewrite the first part like this:

if(empty($_GET['key']) or !strcmp($_GET['key'], 'home')){
    $page = 'section';
    $id = 1; // integer not string
}elseif(!strcmp($_GET['key'], 'blahblah')){
    $page = 'blahblah';
    $id = 2; // integer not string
} // ...
include ('header.php');
include ("{$page}.php");
include ('footer.php');
  • Switch for strings is not always the best idea. You might want case insensitive compare and that's the strcasecmp realm.
  • If $_GET['key'] is missing, you will get a notice with the switch while empty() is safe.
  • $id seems to be an integer 1 so why make it a string '1'?
  • Why concatenate like this $page.'.php' when you can have php do it for you "{$page}.php"?

As of the second part, things are not too great:

  • Why concatenate on echo like this echo 'string'.'string' when echo is a language construct and allows this echo 'string', 'string' which skips the concatenation?
  • Why use $num_results == 0 when you can just use !$num_results?
  • Why not escape arguments in MySQL Query (let's just say you have an $id integer and might get away with it here) when SQL injections are so nasty :)

PS: AND YOU need to work on your formatting A LOT... like very much!

\$\endgroup\$
5
  • \$\begingroup\$ Thank you very much. I will adjust my code and formatting. \$\endgroup\$
    – Ngai
    Jul 9, 2013 at 17:43
  • 1
    \$\begingroup\$ use $num_results === 0 for comparison and check for FALSE (false doesn't mean nothing was returned, it means something went wrong) \$\endgroup\$
    – Pinoniq
    Jul 9, 2013 at 19:57
  • 1
    \$\begingroup\$ @Pinoniq In most cases, when something goes wrong and your query is correct... it's a MySQL server issue and it's kind of out of your control. You usually don't need to go into such detail with error handling unless you are into very detailed logging and I don't think he's into that just yet. \$\endgroup\$
    – CodeAngry
    Jul 9, 2013 at 20:07
  • \$\begingroup\$ @CodeAngry true. But he should know about the difference between === and == \$\endgroup\$
    – Pinoniq
    Jul 10, 2013 at 7:48
  • 1
    \$\begingroup\$ I did not know about ===, i'll read into it. Thx \$\endgroup\$
    – Ngai
    Jul 10, 2013 at 17:49
2
\$\begingroup\$

Well, first of all, OOP is not something you simply use. OOP is a concept of classes and objects. But it doesnt magically make your code better.

Your main problem here is that your business logic is tightly coupled with you displaying of date (the view). You are fetching data, validating data and rendering the output all in the same file and on the same line. You should look in to DRY and Separation of Concerns

What I would start with is removing all business logic from your html. Thus creating a file that fetches all the data and stores it in a variable (e.g. $data) and a template file that only contains html and some statements (ofcourse foreach loops for lists are also user). This way you can develop templates without having to worry about messing up your business logic.

To sume up: OOP is never an answer. OOP is simply a concept, just like functional programming is a concept. If used correctly they can be very powerfull. And that is the fun part of PHP, it supports both.

Then some remarks ad your SQL: What you are doing is the following:

1st query: Fetch me the last 4 cmsarticles from section $section
2nd query: Fetch me all cmsarticles from section $section, and append the section.name to it from section $section

This could be rewritten to:

1st query: Fetch me the section.name from section $section
2nd query: Fetch me all cmsarticles from section $section

The queries will perform faster since you are simply looking up one table. And it will use less memory since the section.name in your current 2nde query will always be the same.

I hope this answer helps you! feel free to ask some more

\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much! I'll try to improve my code with your tips. \$\endgroup\$
    – Ngai
    Jul 9, 2013 at 17:44
  • \$\begingroup\$ Btw, DON'T use mysql_* functions. php.net/manual/en/function.mysql-connect.php see the big red warning? read it ;) \$\endgroup\$
    – Pinoniq
    Jul 9, 2013 at 19:58
  • \$\begingroup\$ It's on my to-do list to rewrite the system to mysqli. But i'm assuming it will take some time so i've postponed it a little. There's no way around the warning hah! \$\endgroup\$
    – Ngai
    Jul 10, 2013 at 17:51

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.