My upload class

Coding Critique is the place to post source code for peer review by other members of DevNetwork. Any kind of code can be posted. Code posted does not have to be limited to PHP. All members are invited to contribute constructive criticism with the goal of improving the code. Posted code should include some background information about it and what areas you specifically would like help with.

Popular code excerpts may be moved to "Code Snippets" by the moderators.

Moderator: General Moderators

Post Reply
User avatar
PHPycho
Forum Contributor
Posts: 336
Joined: Fri Jan 06, 2006 12:37 pm

My upload class

Post 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 !!
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post 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).
(#10850)
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post 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.
User avatar
Christopher
Site Administrator
Posts: 13596
Joined: Wed Aug 25, 2004 7:54 pm
Location: New York, NY, US

Post 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());
}
(#10850)
User avatar
John Cartwright
Site Admin
Posts: 11470
Joined: Tue Dec 23, 2003 2:10 am
Location: Toronto
Contact:

Post by John Cartwright »

Ah good idea arborint 8)
User avatar
PHPycho
Forum Contributor
Posts: 336
Joined: Fri Jan 06, 2006 12:37 pm

Post 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
jmut
Forum Regular
Posts: 945
Joined: Tue Jul 05, 2005 3:54 am
Location: Sofia, Bulgaria
Contact:

Post 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.
User avatar
zyklone
Forum Commoner
Posts: 29
Joined: Tue Nov 28, 2006 10:25 pm

Post 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.
User avatar
feyd
Neighborhood Spidermoddy
Posts: 31559
Joined: Mon Mar 29, 2004 3:24 pm
Location: Bothell, Washington, USA

Post by feyd »

rename() is used to move files too, however rename() shouldn't even be used. move_uploaded_file() should be used instead.
Post Reply