Re: Optimizing Help
| From: | Jack Jackson | Date: | Tue, 14 Jun 2005 22:52:41 +0000 |
| Subject: | Re: Optimizing Help | ||
| References: | 1 2 | Groups: | php.general |
| Request: | Send a blank email to php-general+get-216982@lists.php.net to get a copy of this message | ||
Jochem,
I cannot believe you spent this much time helping me with this. I really appreciate it. I will go through and make ALL those changes. I am grateful. Especially the changes which cleaned up jibberish and tightened from several lines to one - really! Thank you thank you thank you!
--JJ
Jochem Maas wrote:
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, marcus@mkaz.com # 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!