Page 1 of 1

My upload class

Posted: Sun Mar 25, 2007 10:45 pm
by PHPycho
I have the following code

Code: Select all

<?php
class uploader
{	
	var $uploadDir;	
	var $newFileName;	
	var $fileInfo = array();
	var $maxFileSize;       
	var $allowTypes = array();	  
	var $fileToDelete;
	//var $errMsg = "";
	var $errMsg = array();
	var $upMsg = "";	
	var $delMsg = "";
	var $randNo = "";	
		
	function getExtension()
	{
		$fileExt = substr(strrchr($this->fileInfo['name'],"."),1);
		//$fileExt = explode(".",$this->fileInfo['name']);
		return $this->fileExt = $fileExt;
	}
	
	function checkTypes()
	{
		if(!in_array($this->fileInfo['type'],$this->allowTypes))
		{
			return FALSE;
		}
		else
			return TRUE;
	}              
	
	
	function checkUpload()
	{
		// Check the Size
		/* If fileSize > maxSize */
		if($this->fileInfo['size'] > $this->maxFileSize)
		{
			//array_push($this->errMsg,"File Size is larger");
			$this->errMsg[] = "File Size is larger <br />";
			return FALSE;
		}
	   
		/* If fileSize == 0 */
		else if($this->fileInfo['size'] == 0)
		{
			//array_push($this->errMsg,"No file uploaded");
			$this->errMsg[] = "No file uploaded <br />";
			return FALSE;
		}
	   
		/* Check the Types */
		else if(!$this->checkTypes())
		{
			//array_push($this->errMsg,"Invalid file type !!");
			$this->errMsg[] = "Invalid file type !! <br />";
			return FALSE;
		}
	   
		/* If everything goes fine then Upload */
		else
		{                     
			return TRUE;					   
		}  			
			   
	}
	
	function doUpload()
	{
		$uploadPath = $this->uploadDir."/".$this->fileInfo['name'];
		@move_uploaded_file($this->fileInfo['tmp_name'],$uploadPath);
		//finally rename
		$ext = $this->getExtension();
		$newUploadPath = $this->uploadDir."/".$this->newFileName.".".$ext;
		if(@rename($uploadPath,$newUploadPath))
		{
			$this->upMsg = "Sucessfully Uploaded & Renamed !!";
		}
		else
		{
			$this->upMsg = "Unable to Rename !!";
		}
		
	}
	
	function deleteFile()
	{
		if(file_exists($this->fileToDelete))
		{
			if(unlink($this->fileToDelete))
			{
				$this->delMsg = "File Successfully Deleted !!";	
			}
			else			
			{
				$this->delMsg = "Unable to Delete the file !!";
			}
		}
		else
		{
			$this->delMsg = "Such file Doesnt exists !!";
		}
		return $this->delMsg;
	}
	
	function getErrors()
	{
		return $this->errMsg;
	}
	
	function genRandomNo()
	{
		//using uniqid() or microtime() for unique no generation
	  $randNo = substr(md5(uniqid(microtime())), 0, ;
	  return $randNo;
	}
       
}
?>

Code: Select all

<?php
//including and creating object goes here...
$uploaderObj->uploadDir		 = "../uploads/images/";
		$uploaderObj->fileInfo 		 = $_FILES['client_img'];		
		$uploaderObj->maxFileSize	 = 1048576; //in bytes
		$uploaderObj->allowTypes	 = array("image/jpeg","image/jpg");		
		
		//Check Upload and Insert the Image Contents in DB
		if($uploaderObj->checkUpload())
		{
			$clientImagesObj->insert($clientID,$caption);
			$uploaderObj->newFileName  = $clientImagesObj->id;
			//Finally Upload the file
			$uploaderObj->doUpload();
     		echo $uploaderObj->upMsg;
		}
		else
		{
			print_r($uploaderObj->getErrors());
			//echo $uploaderObj->getErrors();			
		}		
?>
Can anybody help me on making this class more effective ?
Thanks in advance to all of you !!

Posted: Sun Mar 25, 2007 11:06 pm
by Christopher
I think I would move $_FILES inside the class to abstract the process a little more. And make the defaults something reasonable (e.g. maxFileSize set to the INI max size).

Posted: Mon Mar 26, 2007 10:44 am
by John Cartwright
There are a couple things that need to be addressed
  • You are handling errors differently throughout your class, stick to storing your errors in an array and having $this->getErrors() retrieve the errors
  • Tied in with the last comment, don't return and error message if the method failed (doUpload() and deleteFile()), return a boolean instead
  • Code: Select all

    @move_uploaded_file($this->fileInfo['tmp_name'],$uploadPath);
    You are never checking if the move_uploaded_file() failed
  • When checking errors, I like to check every possibility to give them the full idea of what wen't wrong. As of right now, you are only returning a single error by doing elseif()'s, perhaps having them as their own conditionals.. your choice
  • If everything wen't well, I don't see a need for returning anything other than a boolean true.. let the developer decide what to display. Things like

    Code: Select all

    if(@rename($uploadPath,$newUploadPath))
                    {
                            $this->upMsg = "Sucessfully Uploaded & Renamed !!";
                    }
                    else
                    {
                            $this->upMsg = "Unable to Rename !!";
                    }
    could be reduced to

    Code: Select all

    return @rename($uploadPath,$newUploadPath));
  • Your API seems a bit strange to me.. to me there is no need to externally do checkUpload().. this should be called internally by doUpload()

    Code: Select all

    $upload = new uploader('client_img', $maxFileSize, $allowTypes, $pathToUpload);
    
    if (!$upload->doUpload())
    {
       echo '<pre>';
       print_r($upload->getErrrors());
    }
I think that should get you started.

Posted: Mon Mar 26, 2007 2:22 pm
by Christopher
I didn't have time to post any code, but agree with Jcart's comments. To allow multiple file upload you might want to move the form field name to the upload method so you could process several files:

Code: Select all

$upload = new uploader($maxFileSize, $allowTypes, $pathToUpload);

if (!$upload->doUpload('client_img'))
{
   echo '<pre>';
   print_r($upload->getErrrors());
}

Posted: Mon Mar 26, 2007 2:56 pm
by John Cartwright
Ah good idea arborint 8)

Posted: Tue Mar 27, 2007 12:11 am
by PHPycho
Thanks devnetworkians
great tips and suggestions !!
As i am new to OOP your suggestions helping me a great deal.
Based on the suggestions and comments made above i am going to make a new uploader class.
Once completed i will post again in this section for your feedback.
Thaks a lot

Posted: Tue Mar 27, 2007 12:39 am
by jmut
arborint wrote:I didn't have time to post any code, but agree with Jcart's comments. To allow multiple file upload you might want to move the form field name to the upload method so you could process several files:

Code: Select all

$upload = new uploader($maxFileSize, $allowTypes, $pathToUpload);

if (!$upload->doUpload('client_img'))
{
   echo '<pre>';
   print_r($upload->getErrrors());
}
good indeed.
I would rather make factory that returns collection of uploader objects. And keep uploader object strictly processing one upload only.

Posted: Wed Jul 11, 2007 1:59 am
by zyklone
i just wonder why did you use rename() if you can rename the file before you upload it. in your case.. you upload it first then you rename the file. i think it will make slower.

Posted: Wed Jul 11, 2007 6:26 am
by feyd
rename() is used to move files too, however rename() shouldn't even be used. move_uploaded_file() should be used instead.