patternphpMinor
Image upload script for adding items to a database
Viewed 0 times
scriptimageuploadaddingdatabaseitemsfor
Problem
I wrote my first simple image upload script that allows users to add items to a database along with pictures of said item. My script takes the images uploaded via a form, processes and resizes them and finally saves them to a hard disk, with their file paths added to a database.
While my code is currently working, I have the following questions:
```
//Code to add the item to database.....
//If the insertion is successful
if($insertdata)
{
$upload=$_FILES['ImageUpload']['tmp_name'];
$uploadnum=count($upload);
for($i=0;$ilink to try again");
}
//Creates thumbnail
$thumbnewwidth=100;
$thumbnewheight=100;
$thumbimg=imagecreatetruecolor($thumbnewwidth, $thumbnewheight);
imagecopyresampled($thumbimg,$src,0,0,0,0,$thumbnewwidth,$thumbnewheight,$width,$height);
//Creates displayimage
$displaynewwidth=250;
$displaynewheight=250;
$displayimg=imagecreatetruecolor($displaynewwidth, $displaynewheight);
imagecopyresampled($displayimg,$src,0,0,0,0,$displaynewwidth,$displaynewheight,$width,$height);
//If the image currently being processed is the display pic, add "DP" to the fileuploadname
if($i==0)
{
//Sets thumbnail filepaths to the pictures and saves it
$thumbfilename=$thumbuploadpath.$renamedfile."DP";
$thumbfilepath=$
While my code is currently working, I have the following questions:
- Should I be processing the images after the items have been successfully inserted into the database?
- Should I be hard-coding/ specifying the image width and height explicitly or is there a better/ more efficient way of going about it.
- Is my code an acceptable way of handling uploaded images?
- Are there any better/ more efficient/ neater methods of refactoring my code?
```
//Code to add the item to database.....
//If the insertion is successful
if($insertdata)
{
$upload=$_FILES['ImageUpload']['tmp_name'];
$uploadnum=count($upload);
for($i=0;$ilink to try again");
}
//Creates thumbnail
$thumbnewwidth=100;
$thumbnewheight=100;
$thumbimg=imagecreatetruecolor($thumbnewwidth, $thumbnewheight);
imagecopyresampled($thumbimg,$src,0,0,0,0,$thumbnewwidth,$thumbnewheight,$width,$height);
//Creates displayimage
$displaynewwidth=250;
$displaynewheight=250;
$displayimg=imagecreatetruecolor($displaynewwidth, $displaynewheight);
imagecopyresampled($displayimg,$src,0,0,0,0,$displaynewwidth,$displaynewheight,$width,$height);
//If the image currently being processed is the display pic, add "DP" to the fileuploadname
if($i==0)
{
//Sets thumbnail filepaths to the pictures and saves it
$thumbfilename=$thumbuploadpath.$renamedfile."DP";
$thumbfilepath=$
Solution
Let's start with a review:
-
Use
-
Use absolute paths. Relative paths are brittle, and bound to cause hard-to-track errors when changing the directory structures. Absolute paths will allow you to very quickly know that you need to change something in case of structure shift.
-
Better filename handling: I usually save the file as a hash of the contents of the image (
-
Don't
Now, for your questions
-
Use
foreach: When iterating an array (or an Iterator), PHP provides you with the foreach loop, which allows you to easily iterate over the array.foreach ($_FILES as $file) { //$file now holds the current item, i.e. $_FILES[$i];-
Use absolute paths. Relative paths are brittle, and bound to cause hard-to-track errors when changing the directory structures. Absolute paths will allow you to very quickly know that you need to change something in case of structure shift.
-
Better filename handling: I usually save the file as a hash of the contents of the image (
sha1($image_file_contents) . ".$ext"), this has the nice added property of not allowing duplicate images (It won't catch all cases, but it works nicely). Also, dividing the images into subdirectories (by the first letter or two of the filename) will help you out tremendously when you start getting thousands of images.-
Don't
die(): Calling die() in your code will cause the script to terminate instantly. If you were in an included file or something else still needs to run, it won't. Store the error message, and output it at the end of the script.Now, for your questions
- Depends, are the pictures extremely heavy or otherwise take a burden on the server? If you're uploading 100 2MB pictures at a time, that may be the case. If you're only upload 2-3, it wouldn't.
- Don't generate the thumbnail in advance. Generate it on-demand and based on the requester's needs (i.e. an avatar in a forum is 64x64, but in the profile page it's 128x128). Save the results normally to a file and create a database entry for it, so that next time, you won't have to generate the thumbnail again.
- It is. It depends on the rest of the application. Personally, I'd go with a more OO solution, but that's because I understand how OO works and how to incorporate it into my needs.
- Split the code into smaller pieces of work. Either objects or functions, that do small tasks each (get input, do something, return output). This will help code readability tremendously.
Code Snippets
foreach ($_FILES as $file) { //$file now holds the current item, i.e. $_FILES[$i];Context
StackExchange Code Review Q#48876, answer score: 2
Revisions (0)
No revisions yet.