patternphpMinor
Simple content management system
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):
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
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
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:
As of the second part, things are not too great:
PS: AND YOU need to work on your formatting A LOT... like very much!
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
strcasecmprealm.
- If
$_GET['key']is missing, you will get a notice with the switch whileempty()is safe.
$idseems to be an integer1so 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
echolike thisecho 'string'.'string'when echo is a language construct and allows thisecho 'string', 'string'which skips the concatenation?
- Why use
$num_results == 0when you can just use!$num_results?
- Why not escape arguments in MySQL Query (let's just say you have an
$idinteger 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.