Object-oriented file upload handler for a CMS

Question

I've started with a bunch of functions, made the code work, then I spent a day rewriting it in an OO way, as much as I can.

I've tried to group up the task into objects, letting the relative data go to the relative function. But it still doesn't sound like OO, just several groups of functions.

There is an 'upload attachment form' in my CMS, which can be used for uploading attachments for [Category, Page, Content]. The form includes a file input (multiple) and some data for "where the attachment belongs to".

The code will be used at the handing page where the form POST to.

It should be able to:

1. Do the error checking, return error message if occur
2. Load some dynamic setting from database
3. Save the file into specific directory
4. Rename the file into random string
5. Create a thumb image
6. Store file data into the database, including some linking table
7. Handle both normal and ajax request, and return the correct feedback For the thumb image, it should only do whenever if the file is a image, but I didn't do it yet.

Please help me and let me know how to get it done in a nice OO way.

You can also see my code at codeviewer.

UploadHandeler

Class UploadHandeler{

    private $helper;
    private $files;
    private $dataModel;

    function __construct(){
        $this->helper    = new UHelper;
        $this->dataModel = new DataModel;
        $this->files     = new FilesControl;
    }

    public function init(){
        $this->helper->checkInput();
        $this->dataModel->setField();
        $this->dataModel->setObj();
        $this->files->setBaseDir('test_upload/');
        $this->files->setSetting($this->dataModel->fieldId);
        $this->files->checkDir();
        $this->files->loadfiles();
    }

    public function saveFiles(){
        $this->files->renameFiles();
        $this->files->saveFiles();
        $this->files->makeThumbImage(150,255);
    }

    public function insertDB(){
        $this->dataModel->loadData($this->files->files);
        $this->dataModel->insertDB();
        echo'<br>-----end-------<br>';
    }
}

FilesController

Class FilesControl{

    public $baseDir;
    public $files;
    public $filename_arr = array();
    public $setting = array();


    function __construct(){
        $this->files = UHelper::getfileArray();
    }

    public function setBaseDir($dir){
        if(!file_exists($dir)){
            return UHelper::exitError('Base-directory is not exists.');
        }
        $this->baseDir = $dir;
        return true;
    }

    function setSetting($fieldId){

        $result = Query::get_Setting($fieldId);

        if(!$result){
            return UHelper::exitError('AttachmentFeild not found.');
        }

        $this->setting['allowed_ext'] = explode(',' ,$result[0]['allowed_ext']);
        $this->setting['maxFileSize'] = $result[0]['maxFileSize'];
        $this->setting['maxFileNum'] = $result[0]['maxFileNum'];
        $this->setting['path'] = $this->baseDir.'/'.$result[0]['folder'].'/';
        return;
    }

    function checkDir(){
        $dir = $this->setting['path'];
        //create directory if not exists
        if(!file_exists($dir)){
            if(!mkdir($dir, 0777)){
                UHelper::exitError('Fail when create directory.');
            }
        }
        return true;
    }

    function loadFiles(){
        $new_obj_arr = array();
        foreach ($this->files as $itemData) {
            $this->checkFile($itemData);
            $file = new File;
            $file->loadFile($itemData);
            array_push($new_obj_arr, $file);
        }
        $this->files = $new_obj_arr;
        return true;
    }

    function checkFile($file){
        $allowed_ext = $this->setting['allowed_ext'];
        $exd = UHelper::get_extension($file['name']);
        if( $file['error'] !== 0){
            return UHelper::exitError('Something went wrong with your upload.');
        }
        if(!in_array($exd, $allowed_ext)){
            return UHelper::exitError('File format "'.$exd.'" is not allowed.');
        }
        return true;
    }

    /*-----------------------------------------------*/

    function renameFiles(){

        foreach ($this->files as $key => $file) {
            $exd = $file->fileType;
            do{
                $fileName = UHelper::randomString().'.'.$exd;
            }while( $this->fileNameIsRepeat($fileName) );
            $this->files[$key]->fileName= $fileName;
            array_push($this->filename_arr, $fileName);
        }
        return true;
    }

    function fileNameIsRepeat($fileName){
        if( file_exists($this->setting['path'].$fileName) ||
            in_array($fileName, $this->filename_arr)){
            return true;
        }else{
            return false;
        }
    }

    function saveFiles(){
        $dir = $this->setting['path'];
        foreach ($this->files as $file) {
            $tmp_name = $file->tmp_name;
            $path = $dir.$file->fileName;
            if(!move_uploaded_file($tmp_name, $path)){
                Helper::exitError('Fail when saving file!');
            }
        }
        return true;
    }


    function makeThumbImage($max_w, $max_h){
        $dir = $this->setting['path'];
        foreach ($this->files as $file) {
            $fileName = $file->fileName;
            $path = $dir.$fileName;
            $exd  = $file->fileType;
            $image = $this->imageResize($path, $exd, $max_w, $max_h);
            $this->saveImage($image, $fileName, $exd);
        }
        return true;
    }

    /*-----------------------------------------------*/

    function imageResize($path, $exd, $max_w, $max_h){

        //load image as resource 
        $src_image= $this->loadImage($path, $exd);

        //Get the image size
        $src_w=ImageSX($src_image);
        $src_h=ImageSY($src_image);
        $s = UHelper::getNewSize($src_w, $src_h, $max_w, $max_h);
        $dst_w=$s['w'];
        $dst_h=$s['h'];

        //do the resize
        $dst_image=ImageCreateTrueColor($dst_w,$dst_h);
        imagecopyresampled($dst_image,$src_image,0,0,0,0,$dst_w,$dst_h,$src_w,$src_h);
        imagedestroy($src_image);

        return $dst_image;
    }

    function loadImage($path, $exd){
        switch ($exd) {
            case 'jpg':
            case 'jpeg':
                $image=imagecreatefromjpeg($path);
                break;
            case 'png':
                $image=imagecreatefrompng($path);
                break;
            case 'gif':
                $image=imagecreatefromgif($path);
                break;                
            default:
                UHelper::exitError('Image type not support.');
                break;
        }
        return $image;
    }

    function saveImage($image, $fileName, $exd){
        $folder = $this->setting['path'];
        $dst_path = UHelper::makeNewPath($folder, $fileName);
        switch ($exd) {
            case 'jpg':
            case 'jpeg':
                ImageJpeg($image,$dst_path,95);
                break;
            case 'png':
                Imagepng($image,$dst_path,0);
                break;                
            case 'gif':
                Imagegif($image,$dst_path);
                break;            
            default:
                UHelper::exitError('Image type not support.');
                break;
        }
        imagedestroy($image);
        return true;
    }
}

File

Class File{

    public $tmp_name;
    public $org_name;
    public $fileName;
    public $fileType;
    public $size;    

    function loadFile($row){
        $this->tmp_name = $row['tmp_name'];
        $this->org_name = $row['name'];
        $this->size  = $row['size'];
        $this->fileType = UHelper::get_extension($row['name']);
    }
}

UHelper

Class UHelper{


    public function exitError($errorMsg){
        exit($errorMsg);
        return false;
    }

    public function checkInput(){
        if(strtolower($_SERVER['REQUEST_METHOD']) != 'post'){
            UHelper::exitError('Wrong HTTP method.');
        }
        if(!array_key_exists('Upload',$_FILES)){
            UHelper::exitError('Something went wrong with your upload.');
        }
        return true;
    }

    public function getfileArray(){
        $new_arr = array();
        $files = $_FILES["Upload"];
        $num = count($files["name"]);
        for ($i=0; $i<$num ; $i++){
            $new_arr[$i] = array();
        }

        foreach ($files as $key => $row) {
            for ($i=0; $i<$num ; $i++){
                $new_arr[$i][$key] = $files[$key][$i];
            }        
        }
        return $new_arr;
    }

    public function get_extension($file_name){
        $ext = explode('.', $file_name);
        $ext = array_pop($ext);
        return strtolower($ext);
    }

    public function randomString() {
        $length = 5;
        $characters = "0123456789ABCDEFGHIJKLMNOPQRSTUVWZYZ";

        $charactersNum = strlen($characters) ;     
        $string='';
        for ($p = 0; $p < $length; $p++){
            $string .= $characters[mt_rand(0, $charactersNum-1)];
        }
        return dechex(time()).strtolower($string);
    }

    public function getNewSize($w, $h, $max_w, $max_h){

        if($w <= $max_w && $h <= $max_h){
            $new_w = $w;
            $new_h = $h;
        }
        if($w > $max_w || $h > $max_h){
            $new_w = $max_w;
            $w_radio = $w / $new_w;
            $new_h = $h/$w_radio;
        }
        if($new_h > $max_h){
            $new_h = $max_h;
            $h_radio = $h / $new_h;
            $new_w = $w/$h_radio;
        }
        return array("w"=> $new_w, "h"=> $new_h);
    }

    public function makeNewPath($folder, $fileName){
        $pop = explode('.', $fileName);
        $fileName = $pop[0];
        $exd = $pop[1];

        $newPath = $folder.$fileName.'_s'.'.'.$exd;

        return $newPath;
    }
}

DataModel

Class DataModel{

    public $baseDir;
    public $objName;
    public $objId ;
    public $fieldId;
    public $files;

    public $objType_arr = array(
        '1'=>'categoryItem',
        '2'=>'page',
        '3'=>'content'
    );

    public function setField(){
        if(!isset($_POST['attachmentField_id'])){
            return UHelper::exitError('Parameter missing.');
        }
        $this->fieldId = $_POST['attachmentField_id'];
        return true;
    }

    public function setObj(){
        $objType=$_POST['objType'];
        if(!isset($this->objType_arr[$objType])){
            return UHelper::exitError('Object type incorrect.');
        }    
        $name = $this->objType_arr[$objType];
        $idName = $name.'_id';
        if(!isset($_POST[$idName])){
            return UHelper::exitError('Parameter missing.');
        }

        $this->objName = $name;
        $this->objId = $_POST[$idName];

        return true;
    }

    /*-----------------------------*/

    public function loadData($fileData){
        $this->files = $fileData;
        return true;
    }

    public function insertDB(){
        $files    = $this->files;    
        $fieldId  = $this->fieldId;
        $objId    = $this->objId;
        $user     = User::getCurrentUser();
        $datetime = date("Y-m-d H:i:s");
        $tableName = ucfirst($this->objName).'AttachmentFeild_Value';
        $fieldName = $this->objName.'_id';

        /** Get $attachment_id if already have record **/
        $attachment_id = Query::get_AttachmentId($tableName, $fieldName, $objId, $fieldId);

        if(!$attachment_id){

            $folder = $this->setting['dir'];
            Query::insert_Attachment($folder);

            $attachment_id = Yii::app()->db->getLastInsertID();
            Query::insert_Obj_to_Field($tableName, $objId, $fieldId, $attachment_id);

        }

        foreach ($files as $file) {

            $filename = $file->fileName;
            $org_name = $file->org_name;
            $exd      =  $file->exd;

            // insert into AttachmentItem
            Query::insert_AttachmentItem($attachment_id, $filename, $org_name, $exd);
        }
    }
}

Query

Class Query{

    public function get_Setting($fieldId){
        $sql = 
            "SELECT * 
            FROM {{AttachmentFeild}} 
            WHERE id = :fieldId 
            AND status = '6'";
        $command = Yii::app()->db->createCommand($sql);
        $command->bindParam(":fieldId", $fieldId, PDO::PARAM_STR);
        $result = $command->queryAll();
        return $result;
    }

    public function get_AttachmentId($tableName, $fieldName, $objId, $fieldId){
        $sql = 
            "SELECT * 
            FROM {$tableName} 
            WHERE $fieldName = :objId
            AND attachmentFeild_id =:fieldId";
        $command = Yii::app()->db->createCommand($sql);
        $command->bindParam(":objId", $objId, PDO::PARAM_STR);
        $command->bindParam(":fieldId", $fieldId, PDO::PARAM_STR);
        $result = $command->queryAll();
        if(!$result){
            return false;
        }
        $attachment_id = $result[0]['attachment_id'];
        return $attachment_id;
    }

    public function insert_Attachment($folder){

            $user     = User::getCurrentUser();
            $datetime = date("Y-m-d H:i:s");

            $sql = 
                "INSERT INTO {{Attachment}}
                 VALUES(null, 1, null, null, :folder ,null, 6,  :user, :user, :datetime, :datetime, null)";
            $command = Yii::app()->db->createCommand($sql);
            $command->bindParam(":folder",   $folder, PDO::PARAM_STR);
            $command->bindParam(":user",     $user,   PDO::PARAM_STR);
            $command->bindParam(":datetime", $datetime, PDO::PARAM_STR);
            $command->execute();
            return true;
    }

    public function insert_Obj_to_Field($tableName, $objId, $fieldId, $attachment_id){

        $user     = User::getCurrentUser();
        $datetime = date("Y-m-d H:i:s");

        $sql = 
            "INSERT INTO {$tableName} 
            VALUES(null, :objId, :fieldId, :attm_id, :user, :user, :datetime, :datetime, null)";
        $command = Yii::app()->db->createCommand($sql);
        $command->bindParam(":objId",    $objId,   PDO::PARAM_STR);
        $command->bindParam(":fieldId",  $fieldId, PDO::PARAM_STR);
        $command->bindParam(":attm_id",  $attachment_id, PDO::PARAM_STR);
        $command->bindParam(":user",     $user,     PDO::PARAM_STR);
        $command->bindParam(":datetime", $datetime, PDO::PARAM_STR);
        $command->execute();
        return true;
    }

    public function insert_AttachmentItem($attachment_id, $filename, $org_name, $exd){

        $user     = User::getCurrentUser();
        $datetime = date("Y-m-d H:i:s");

        $sql = 
            "INSERT INTO AttachmentItem
             VALUES (null, :attchId, null, null, 1, :filename, :org_name, :exd, '6', 1000, :user, :user, :datetime, :datetime, null)";
        $command = Yii::app()->db->createCommand($sql);
        $command->bindParam(":attchId",  $attachment_id, PDO::PARAM_STR);
        $command->bindParam(":filename", $filename, PDO::PARAM_STR);
        $command->bindParam(":org_name", $org_name, PDO::PARAM_STR);
        $command->bindParam(":exd",      $exd, PDO::PARAM_STR);
        $command->bindParam(":user",     $user, PDO::PARAM_STR);
        $command->bindParam(":datetime", $datetime, PDO::PARAM_STR);
        $result = $command->execute();

        return true;        
    }
}

Answer 1

Cohesion

The concept of cohesion is about how closely related are some operations, for example the statements in a function. It's good when statements are as closely related as possible. This kind of code is not a good practice:

public function saveFiles(){
    $this->files->renameFiles();
    $this->files->saveFiles();
    $this->files->makeThumbImage(150,255);
}

The statements in this function are executed in a sequence. In fact they must be executed in this specific sequence otherwise it won't work. This is because the second statement is expecting a certain side effect to happen due to the first statement. Side effects are not a good thing, because they are not obvious when you look at this code, you have to check the implementation of these functions to verify. As such, the dependence on the specific sequence is not visible here, making it a semantic rule, which is not so good.

Consider this alternative:

public function saveFiles($files){
    $renamed = $this->renameFiles($files);
    $saved = $this->saveFilesRenamed($renamed);
    $this->makeThumbImage($saved, 150,255);
}

This way the dependence between the statements is clear.

Down with 777 permission already

Coding horror:

        if(!mkdir($dir, 0777)){
            UHelper::exitError('Fail when create directory.');
        }

Permission 777 is dangerous, and completely unnecessary 99% of the time. Please try your best to avoid it.

I looked up in the official docs of php, sadly the example uses 777 too, so I guess there's little hope for now that this archaic and very dumb practice will die anytime soon.

Dead code

In UHelper::exitError, there is an exit statement followed by return false. After exit, execution stops, so any code after that will not be executed, and effectively dead code.

It's especially confusing that in other parts of the program you do:

return UHelper::exitError(...)

The return is pointless, as nothing will be returned, it's just compelling readers to read the implementation of UHelper::exitError to find out how its return value might be relevant, only to realize it's actually pointless.

Bogus return statements

Several methods return something when the return values are never used, and pointless anyway.

For example FilesController.setBaseDir either crashes on error or returns true. You could simply omit the return statement.

In other methods where there is return; as the last statement, you can omit that too, it's completely unnecessary.

API ergonomics

Query::get_Setting is not ergonomic: it would be intuitive if it returned an associative array of key-value pairs. But it actually returns a list of arrays, which callers have to know, and use [0] to get to the relevant content. This is not intuitive, and imposes unnecessary boilerplate code on every caller.

It would be better to encapsulate this detail, and return to callers an object that's ready to use, without the [0] indexing. Like you did in Query::get_AttachmentId: it returns an attachment id (more or less), callers don't have to know implementation details.

API sanity

The Query::get_AttachmentId returns two types of values:

• An attachment id, if found
• false (boolean) if not found

A routine with two kinds of return types is nuts, very poor design. Instead of false, it would be more sane to return NULL.

Abrupt returns

It's not a good practice to crash the program with an exit statement in the middle of execution. It's especially bad to have many places in a program that exit. It's better to reorganize the code in a way to allow a more graceful mechanism to exit, for example by breaking out of loops on errors, relinquishing control to caller methods, that in turn relinquish control to their callers, all the way up until the main execution loop.

Use elseif

These two if statements cannot happen at the same time:

    if($w <= $max_w && $h <= $max_h){
        $new_w = $w;
        $new_h = $h;
    }
    if($w > $max_w || $h > $max_h){
        $new_w = $max_w;
        $w_radio = $w / $new_w;
        $new_h = $h/$w_radio;
    }

The second if should be an elseif.