Re: Optimizing Help

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx
#  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., &quot;3&quot; &quot;4.7&quot;): <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., &quot;3&quot; &quot;4.7&quot;): <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!


--
PHP General Mailing List (http://www.php.net/)
To unsubscribe, visit: http://www.php.net/unsub.php


[Index of Archives]     [PHP Home]     [Apache Users]     [PHP on Windows]     [Kernel Newbies]     [PHP Install]     [PHP Classes]     [Pear]     [Postgresql]     [Postgresql PHP]     [PHP on Windows]     [PHP Database Programming]     [PHP SOAP]

  Powered by Linux