jack jackson wrote:
Hello,
Recently with the help of several of you I got a script working. It's
complex, I'm still new, some parts of it came from other authors and I
adapted it, and generally despite the fact that it seems to work
perfectly at this point, I am certain that there is bloat, repetition
and simply useless crap.
I'd like to know if anyone can have a look and help me optimize what I
have. For example, there are several mysql queries - would it be
possible to combine them somehow? I use the image upload section twice
because I couldn't figure out how to make it work using different vars
within the same section. That kind of newbie stuff.
The code is here: http://hashphp.org/pastebin.php?pid=3701
# <?php
#
# require_once('nscfg.php');
#
# //Initialize vars
# $query = '';
# $dropdown = '';
# $result = 0;
# $errors = array();
# $msgArray = array();
# $msgText = '';
be neat, it helps:
$query = '';
$dropdown = '';
$result = 0;
#
# $query = 'SELECT art.*,publisher.*
# FROM art,subject,publisher,series
# GROUP BY publisher_name';
make good (ab)use of alignment (your style may differ ;-)
$query = 'SELECT art.*,
publisher.*
FROM art,
subject,
publisher,
series
GROUP BY publisher_name';
// ps - why is 'series' & 'subject' in there?
#
# $subject_query = 'SELECT art.*,subject.*
# FROM art,subject
# GROUP BY subject_name';
#
# $series_query = 'SELECT art.*,series.*
# FROM art,series
# GROUP BY series_name';
#
# $media_query = 'SELECT media_id,media_name
# FROM media
# GROUP BY media_name';
#
# $result = mysql_query($query);
# $media_result = mysql_query($media_query);
# $subject_result = mysql_query($subject_query);
# $series_result = mysql_query($series_query);
#
instead of creating 4 result identifiers, run and loop thru 1 at a time,
its allows you to reuse a single $result var (as well as being better in
other ways)
# $dropdown_publishers = '<select name="publisher">';
# while ($rows = mysql_fetch_assoc($result)){
# $dropdown_publishers .= '<option value="' . $rows['publisher_id']
. '">' . htmlentities($rows['publisher_name']);
# }
# $dropdown_publishers .= '</select>';
here is a candidate for a function:
buildOptionList($name, $result, $valKey, $nameKey)
{
while ($row = mysql_fetch_assoc($result)) {
$dropdown[] = '<option value="'
. htmlentities($row[$valKey], ENT_QUOTES)
. '">'
. htmlentities($row[$nameKey], ENT_QUOTES)
. '</option>'; // options have closing tags
}
return '<select name="' . $name . '">' .
join('',$dropdown) . '</select>';
}
#
#
# $dropdown_subject = '<select name="subject">';
# while ($rows = mysql_fetch_assoc($subject_result)){
^- white space?
# $dropdown_subject .= '<option value="' . $rows['subject_id'] .
'">' . htmlentities($rows['subject_name']);
# }
# $dropdown_subject .= '</select>';
#
# $dropdown_series = '<select name="series"><option value="">Select A
Series</option>';
# while ($rows = mysql_fetch_assoc($series_result)){
# $dropdown_series .= '<option value="' . $rows['series_id'] . '">'
. htmlentities($rows['series_name']);
# }
# $dropdown_series .= '</select>';
#
# $checkbox_media = array ();
# $media_types = array();
# while ($media_rows = mysql_fetch_assoc($media_result)){
# $checkbox_media[] = "<input type='checkbox' name='media_types[]'
value='{$media_rows['media_id']}' />{$media_rows['media_name']} ";
# }
#
# ///IMAGE SECTION - MAIN IMAGE (IMAGE UPLOAD SCRIPT COPYRIGHT INFO IN
CFG FILE)
# ///THIS SECTION Copyright (c) 2000 Marcus Kazmierczak, [EMAIL PROTECTED]
# if ($REQUEST_METHOD == "POST")
# {
# $uploaddir = "images/jpg";
#
# /*== checks the extension for .jpg or .tiff ==*/
# $pext = getFileExtension($imgfile_name);
TURN OFF register_globals, and access the $_FILES array instead.
#
# $pext = strtolower($pext);
#
#
# //If main image don't match extentions
# if (empty($POST['imgfile'])) {
# $errors[] = 'Please select an image. This is sort of the whole
point, yes?';
your error checking for whether the image was actually uploaded is
non-existent,
check the manual (+usernotes) for guidelines on how to check whether
anything
made it thru.
# }
#
# if (($pext != "jpg") && ($pext != "jpeg") && ($pext != "tif") &&
($pext != "tiff"))
# {
or maybe:
$validExtensions = array("jpg","jpeg","tif","tiff");
if (!in_array($pext, $validExtensions)) { /* bad */ }
/*
having said that the file extension doesn't say anything - better to use
the info
returned by imagegetsize() ... read this:
http://nl2.php.net/manual/en/function.getimagesize.php
*/
# print "<h1>DOH!</h1><p>That's not a valid image extension.
<br />";
# print "<p>You are only permitted to upload JPG, JPEG, tif or
TIFF images with the extension .jpg, .jpeg, .tif or .tiff ONLY<br /><br
/>";
# print "See, now, that last thing you tried to upload? It
ended in $pext. A review:<br />
# $pext != .jpg<br />
# $pext != .jpeg<br />
# $pext != .tif<br />
# $pext != .tiff<br />
# </p>\n";
# }
maybe a bit condescending, besides if the user can't grok
image files then maybe its silly to assume the he/she understands '!=',
I guess it depends on your audience.
#
# /*== setup final file location and name ==*/
# /*== rename file to md5 of image name ==*/
#
# $secondary_imgname = date("l-F-j-Y i:s");
# $final_imgname = md5($secondary_imgname) . ".$pext";
#
# $newfile = $uploaddir . "/$final_imgname";
#
why not just...? (spare a var or 2):
$newfile = $uploaddir.'/'.md5(date("l-F-j-Y i:s")).'.'.$pext;
# /*== do extra security check to prevent malicious abuse on MAIN
image==*/
# if (is_uploaded_file($imgfile))
# {
#
# /*== move file to proper directory ==*/
# if (!copy($imgfile,"$newfile"))
# {
move_uploaded_file()
# /*== if an error occurs the file could not
# be written, read or possibly does not exist ==*/
# print "Error Uploading image file. It\'s something you need
to call Nick about";
# exit();
# }
# }
#
#
# }
#
#
# ////IMAGE SECTION FOR THUMBNAIL
# ////YES I COULD HAVE OPTIMIZED THIS. I DON'T KNOW ENOUGH YET TO DO SO
;).
# if ($REQUEST_METHOD == "POST")
# {
#
# /*== upload directory where the file will be stored
# relative to where script is run ==*/
#
# $uploaddir = "images/jpg";
#
#
# /*== get file extension (fn at bottom of script) ==*/
# /*== checks to see if image file, if not do not allow upload ==*/
# $p2ext = getFileExtension($thb_image_name);
# $p2ext = strtolower($p2ext);
# if (($p2ext != "jpg") && ($p2ext != "jpeg"))
# {
# print "<h1>ERROR</h1>Image Extension Unknown.<br>";
# print "<p>Please upload only a JPEG image with the extension
.jpg or .jpeg ONLY<br><br>";
# print "The file you uploaded had the following extension:
$p2ext</p>\n";
#
# /*== delete uploaded file ==*/
# unlink($thb_image);
move_uploaded_file()
# exit();
# }
#
# /*== setup final file location and name ==*/
# /*== change spaces to underscores in filename ==*/
#
# $secondary_thb_filename = gmdate("Y-j-F-l i:s");
# $final_thb_filename = md5($secondary_thb_filename) . ".$pext";
# $new_thb_file = $uploaddir . "/$final_thb_filename";
making the file name could be made into a function?
#
# /*== do extra security check to prevent malicious abuse==*/
# if (is_uploaded_file($thb_image))
# {
#
# /*== move file to proper directory ==*/
# if (!copy($thb_image,"$new_thb_file"))
# {
# /*== if an error occurs the file could not
# be written, read or possibly does not exist ==*/
# print "Error Uploading File.";
# exit();
# }
# }
#
# /*== delete the temporary uploaded file ==*/
# unlink($thb_image);
#
#
# print("<img
src=\"images/jpg/9010648ac221fc9bf262b56d0c61a8a6.jpg\">");
#
# }
#
#
#
#
# /////VALIDATION SECTION
#
#
# if (isset($_POST['save'])=== true){
# //If user clicked Save, validate data
# $msgArray = array(
# 'title' => 'a title for your cartoon',
# 'art_thumbnail' => 'a Title',
# 'art_image' => 'a Price',
# 'art_width' => 'width (in inches/1 decimal
place, ie 3 or 3.2) as published',
# 'art_height' => 'height (in inches/1 decimal place, ie 3 or
3.2) as published',
# 'art_width' => 'the published width (in inches/1 decimal
place, ie 3 or 3.2)',
# 'art_height' => 'the published height (in inches/1 decimal
place, ie 3 or 3.2)',
# 'orig_width' => 'the original width (in inches/1 decimal
place, ie 3 or 3.2)',
# 'orig_height' => 'the original height (in inches/1 decimal
place, ie 3 or 3.2)',
# 'art_title' => 'the title of the cartoon',
# 'art_caption' => 'an under seven word caption',
# 'art_keywords' => 'keywords, separated,by,commas',
# 'art_blog' => 'a block of text about this cartoon',
# 'imgfile' => 'the main cartoon image',
# 'thb_imgfile' => 'the thumbnail image'
# );
# /*
#
# Though this is more of a kluge, we are going to determine if
# numeric elements contain numbers.
# */
# if (empty($_POST['title'])=== true){
# $errors[] = 'Please enter a title (duh)';
# }
# if (is_numeric($_POST['publisher'])=== false){
# $errors[] = 'Please enter a publisher';
# }
# if (is_numeric($_POST['art_width'])=== false){
# $errors[] = 'Please enter the width as published, in
inches, with up to one decimal place';
# }
# if (is_numeric($_POST['art_height'])=== false){
# $errors[] = 'Please enter the width as published, in
inches, with up to one decimal place';
# }
# if (is_numeric($_POST['orig_width'])=== false){
# $errors[] = 'Please enter the width of the original
artwork, in inches, with up to one decimal place';
# }
#
# if (is_numeric($_POST['orig_height'])=== false){
# $errors[] = 'Please enter the width of the original
artwork, in inches, with up to one decimal place';
# }
#
# if (is_numeric($_POST['media'])=== false){
# $errors[] = 'Please check at least one media box';
# }
#
#
# if (empty($_POST['imgfile'])) {
#
# $errors = validateDataPost($msgArray);
your overwriting $errors here, is that the intention?
# }
#
# if (empty($_POST['thb_imgfile'])) {
#
# $errors = validateDataPost($msgArray);
# }
#
# $thismonth = date("m");
# $today = date("d");
#
# $pubyear = intval(($_POST['pub_year']));
# $pubmonth = ($_POST['pub_month']);
# $pubday = ($_POST['pub_day']);
# $pubdate = "$pubyear" . '-' . "$pubmonth" . '-' . "$pubday";
# $origyear = intval(($_POST['creation_year']));
# $origmonth = ($_POST['creation_month']);
# $origday = ($_POST['creation_day']);
^
^--- whats this about
# $orig_date = "$origyear" . '-' . "$origmonth" . '-' . "$origday";
^
^--- why stick these inside doublequotes?
why not: ?
$orig_date = intval($_POST['creation_year'])
. '-' . $_POST['creation_month']
. '-' . $_POST['creation_day'];
#
#
# if (count($errors)>0){
#
# $msgText = '<tr><td><ul>';
# foreach($errors as $value){
# $msgText .= '<li>' . $value . '</li>';
# }
# $msgText .= '</ul></td></tr>';
# } else { //If we are here then we are ready to insert a new record.
#
# $trimblog = (trim($_POST['blog']));
# $blog = (nl2br($trimblog));
# $image = mysql_real_escape_string($final_imgname);
# $image_thb = mysql_real_escape_string($final_thb_filename);
# $pubwidth = mysql_real_escape_string(trim($_POST['art_width']));
# $pubheight = mysql_real_escape_string(trim($_POST['art_height']));
# $origwidth = mysql_real_escape_string(trim($_POST['orig_width']));
# $origheight = mysql_real_escape_string(trim($_POST['orig_height']));
# $publisher = mysql_real_escape_string(trim($_POST['publisher']));
# $series = mysql_real_escape_string(trim($_POST['series']));
# $subject = mysql_real_escape_string(trim($_POST['subject']));
# $title = mysql_real_escape_string(trim($_POST['title']));
# $caption = mysql_real_escape_string(trim($_POST['art_caption']));
# $blog = mysql_real_escape_string($blog);
# $keywords = mysql_real_escape_string(trim($_POST['keywords']));
# $media_types = ($_POST['media_types']);
#
# // var_dump($media_types); die();
# $query = "INSERT INTO art (art_id,art_thumbnail, art_image,
art_width, art_height, orig_width, orig_height, art_pub_date,
publisher_id, art_creation_date, series_id, subject_id, art_title,
art_caption, art_keywords, art_blog)
# VALUES ('','" . $image_thb . "','" . $image . "','" . $pubwidth
. "','" . $pubheight . "','" . $origwidth . "','" . $origheight . "','"
. $pubdate . "','" . $publisher . "','" . $orig_date . "','" . $series
. "','" . $subject . "','" . $title . "','" . $caption . "','" .
$keywords . "','" . $blog . "')";
do you enjoy reading this?
how about:
$query = "INSERT INTO art (
art_id,
art_thumbnail,
art_image,
art_width,
art_height,
orig_width,
orig_height,
art_pub_date,
publisher_id,
art_creation_date,
series_id,
subject_id,
art_title,
art_caption,
art_keywords,
art_blog
) VALUES (
'',
'$image_thb',
'$image',
'$pubwidth',
'$pubheight',
'$origwidth',
'$origheight',
'$pubdate',
'$publisher',
'$orig_date',
'$series',
'$subject',
'$title',
'$caption',
'$keywords',
'$blog'
)";
/*
but still not really 'it' - how about an array of 'field/post' names to
loop thru, do the escaping,
build a query dynamically?
# mysql_query($query);
# $art_id = mysql_insert_id();
# // foreach ($media_types as $type)
#
#
# if (!empty($media_types)) {
#
# foreach($media_types as $type)
# {
# $query2 = "INSERT INTO media_art (media_art_id,media_id, art_id)
# VALUES ('','{$type}','$art_id')";
#
# mysql_query($query2) or die(mysql_error());
# }
# }//Closes if (!empty($media_types))
# }//Closes if (count($errors)>0)
# }//Closes if (isset($_POST['save'])=== true)
maybe the pastebin borked your indentation, otherwise try lining things
up a bit.
#
# $logOut = $_SERVER['PHP_SELF'] . '?logout=true';
# ?>
#
made be you could stick the 'html' into an include file - its the first
step towards 'templating' :-)
e.g.
include './filename.html.php'; exit;
// where ./filename.html.php contains ...:
# <table align="center">
# <tr><th colspan="4">Main Menu</th></tr>
# <tr>
# <td><a href="addrecord.php">Add A Record</a></td>
# <td><a href="editrecord.php">Edit A Record</a></td>
# <td><a href="deleterecord.php">Delete A Record</a></td>
# <td><a href="<?php //echo $logOut; ?>">Logout</a></td>
# </tr>
# </table>
#
#
# <form action="<?php echo $_SERVER['PHP_SELF']; ?>" method="post"
name="addrecord" enctype="multipart/form-data">
# <table>
# <?php echo $msgText; ?>
# <tr><td>Add A Cartoon To The Database</td></tr>
#
# <tr><td>The Cartoon's Title: <input name="title" id="title"
type="text" size="50" maxlength="50"></td></tr>
#
# <tr><td>Caption: <input name="art_caption" id="art_caption"
type="text" size="50" maxlength="50"></td></tr>
#
# <tr><td>Keywords (Comma,separated,keywords): <input name="keywords"
id="keywords" type="text" size="50" maxlength="50"></td></tr>
#
# <tr><td>Comments and descriptive text (appear beneath photo, like a
blog): <br /><textarea name="blog" id="caption" type="textarea"
lines="4" cols="45" limit="2000"></textarea></td></tr>
#
# <tr><td><strong>Published dimensions</strong><br />
# Width, as published, in <strong>inches and up to one decimal
place</strong> (i.e., "3" "4.7"): <input
name="art_width" id="art_width" type="text" size="3" maxlength="3" /><br />
# Height, as published, in <strong>inches and up to one decimal
place</strong>: <input name="art_height" id="art_height" type="text"
size="3" maxlength="3" /></td></tr>
#
# <tr><td><strong>Original dimensions</strong><br />
# Width, originally, in <strong>inches and up to one decimal
place</strong> (i.e., "3" "4.7"): <input
name="orig_width" id="orig_width" type="text" size="3" maxlength="3"
/><br />
# Height, originally, in <strong>inches and up to one decimal
place</strong>: <input name="orig_height" id="orig_height" type="text"
size="3" maxlength="3" /></td></tr>
#
#
# <tr><td>Publisher: <?php echo $dropdown_publishers; ?></td></tr>
#
# <tr><td>Subject: <?php echo $dropdown_subject; ?></td></tr>
#
# <tr><td>Series: <?php echo $dropdown_series; ?></td></tr>
#
# <tr><td><strong>Media Types:</strong> (check all that apply)<br
/><?php echo join($checkbox_media) ;?></td></tr>
# </table>
#
#
# <b>Publication date:</b>
#
# <select name="pub_month">
# <option value="<?php echo $thismonth; ?>"><?php echo $current_month
?></option>
# <option value="01">January</option>
# <option value="02">February</option>
# <option value="03">March</option>
# <option value="04">April</option>
# <option value="05">May</option>
# <option value="06">June</option>
# <option value="07">July</option>
# <option value="08">August</option>
# <option value="09">September</option>
# <option value="10">October</option>
# <option value="11">November</option>
# <option value="12">December</option>
# </select>
#
# <select name="pub_day">
# <option value="<?php echo $today ; ?>"><?php echo $current_date
?></option>
# <option value="01">1</option>
# <option value="02">2</option>
# <option value="03">3</option>
# <option value="04">4</option>
# <option value="05">5</option>
# <option value="06">6</option>
# <option value="07">7</option>
# <option value="08">8</option>
# <option value="09">9</option>
# <option value="10">10</option>
# <option value="11">11</option>
# <option value="12">12</option>
# <option value="13">13</option>
# <option value="14">14</option>
# <option value="15">15</option>
# <option value="16">16</option>
# <option value="17">17</option>
# <option value="18">18</option>
# <option value="19">19</option>
# <option value="20">20</option>
# <option value="21">21</option>
# <option value="22">22</option>
# <option value="23">23</option>
# <option value="24">24</option>
# <option value="25">25</option>
# <option value="26">26</option>
# <option value="27">27</option>
# <option value="28">28</option>
# <option value="29">29</option>
# <option value="30">30</option>
# <option value="31">31</option>
# </select>
#
# <select name="pub_year">
# <option value="2004">2002</option>
# <option value="2004">2003</option>
# <option value="2004">2004</option>
# <option selected value="<?php echo $current_year ?>"><?php echo
$current_year ?></option>
# <option value="2006">2006</option>
# <option value="2007">2007</option>
# <option value="2008">2008</option>
# <option value="2009">2008</option>
# <option value="2010">2010</option>
# </select>
#
#
# <b>Creation date:</b>
#
# <select name="creation_month">
# <option value="<?php echo $thismonth ?>"><?php echo $current_month
?></option>
# <option value="01">January</option>
# <option value="02">February</option>
# <option value="03">March</option>
# <option value="04">April</option>
# <option value="05">May</option>
# <option value="06">June</option>
# <option value="07">July</option>
# <option value="08">August</option>
# <option value="09">September</option>
# <option value="10">October</option>
# <option value="11">November</option>
# <option value="12">December</option>
# </select>
#
# <select name="creation_day">
# <option value="<?php echo $today ?>"><?php echo $current_date
?></option>
# <option value="01">1</option>
# <option value="02">2</option>
# <option value="0 3">3</option>
# <option value="04">4</option>
# <option value="05">5</option>
# <option value="06">6</option>
# <option value="07">7</option>
# <option value="08">8</option>
# <option value="09">9</option>
# <option value="10">10</option>
# <option value="11">11</option>
# <option value="12">12</option>
# <option value="13">13</option>
# <option value="14">14</option>
# <option value="15">15</option>
# <option value="16">16</option>
# <option value="17">17</option>
# <option value="18">18</option>
# <option value="19">19</option>
# <option value="20">20</option>
# <option value="21">21</option>
# <option value="22">22</option>
# <option value="23">23</option>
# <option value="24">24</option>
# <option value="25">25</option>
# <option value="26">26</option>
# <option value="27">27</option>
# <option value="28">28</option>
# <option value="29">29</option>
# <option value="30">30</option>
# <option value="31">31</option>
# </select>
#
# <select name="creation_year">
# <option value="2004">2002</option>
# <option value="2004">2003</option>
# <option value="2004">2004</option>
# <option selected value="<?php echo $current_year ?>"><?php echo
$current_year ?></option>
# <option value="2006">2006</option>
# <option value="2007">2007</option>
# <option value="2008">2008</option>
# <option value="2009">2008</option>
# <option value="2010">2010</option>
# </select>
#
#
# <input type="hidden" name="MAX_FILE_SIZE" value="100000">
#
# <p>Upload main cartoon image for dispay (450px x X px) : <input
type="file" name="imgfile"> Click browse to upload a local file<br />
# <br />
#
# <p>Upload <em><strong>.jpg</strong></em> thumbnail image:
<input type="file" name="thb_image"> Click browse to upload a local .jpg
or .jpeg file<br />
# <br >
# <tr><td><input type="submit" name="save" value="Save"></td></tr>
# </form>
Thanks in advance!
