|
|
Posted by Jack Jackson on 06/15/05 01:52
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!
>>
>
>
>
Navigation:
[Reply to this message]
|