Object-oriented file upload handler for a CMS

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:

• Do the error checking, return error message if occur

• Load some dynamic setting from database

• Save the file into specific directory

• Rename the file into random string

• Create a thumb image

• Store file data into the database, including some linking table

• 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);

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 = $max_w;
        $w_radio = $w / $new_w;
        $new_h = $h/$w_radio;
    }

The second if should be an elseif.

StackExchange Code Review Q#74456, answer score: 5