HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Simple content management system

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
contentmanagementsystemsimple

Problem

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:

```
";
$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 '';
}
}
echo "";

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

echo "";
$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 "
".$result['title']."";
$query2 = mysql_query('SELECT name,artId,po

Solution

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!

Code Snippets

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');

Context

StackExchange Code Review Q#28262, answer score: 3

Revisions (0)

No revisions yet.