Multiple file upload class

I've written this a few days back, which is not too big. Do you see anywhere I can improve my logic?

```
Process();
----------------------------------------------------------------

Basic:
----------------------------------------------------------------
$upload = new Upload();

if( $uploaded = $upload->Process() ) {

// Prettify output if you want single line success / error messages
$pretty = $upload->PrettyPrint( $uploaded );

// Get success and error messages
foreach ( $pretty as $data ) {

// Do something different with each message type (good, bad)

// Or output all the messages
foreach( $data as $message ) {
echo $message .'';
}
}
}
----------------------------------------------------------------

*/

// Make some protected defaults so you can extend it and change them
protected $file_directory = 'uploads/';
protected $file_max_size = 30; // MB
protected $file_keep_name = true;

// Keep track of errors to handle multiple files
private $errors = 0;
private $error_messages = array();

// Construct
public function __construct( $directory = NULL, $maxSize = NULL, $keepName = NULL )
{
// Directory where the files are stored
$this->file_directory = ( $directory ? $directory : $this->file_directory );

// Maximum size a file can be
$this->file_max_size = ( $maxSize ? ( ( $maxSize 1024 ) 1024 ) : ( ( $this->file_max_size 1024 ) 1024 ) );

// To keep the original name when uploading or use the tmp_name
$this->file_keep_name = ( $keepName ? $keepName : $this->file_keep_name);
}

// Process the files being sent to the server for upload
public function Process( $group = 'image', $element = 'upload' )
{

Usability

• Your example code should really also contain HTML code. Especially considering that the upload HAS to contain multiple files, as it will not work otherwise.

• The upload should also work with single files (or at the very least show a reasonable error message if it doesn't).

• mime groups should be customizable. If you want to keep it simple, just make the function protected/public, otherwise add setter/adder methods.

Security

$_FILES['file']['type'] is user controlled, and thus not trustworthy. An attacker can easily bypass your type check and upload files of dangerous type, such as PHP files.

What you should do is check the file extension as well as the actual file type. The functions that are generally recommended for this are pathinfo and finfo_file respectively.

Style

Generally, I like your style. Your code is quite readable, so just a couple of minor points:

• Function names should start with a lowercase character.

• You are not consistent with your spacing when calling functions. Sometimes you use a space, eg file_exists ( and sometimes you do not, eg array_reverse(. Consistency would increase the readability of your code. Personally, I would recommend the second style without space.

• Same thing with arrays, sometimes it's array ( and sometimes array(, or with elseif: sometimes elseif(, sometimes elseif (.

• You use quite a lot of superficial parentheses. Eg file_max_size = ( $maxSize ? ( ( $maxSize 1024 ) 1024 ) : ( ( $this->file_max_size 1024 ) 1024 ) ). It's not necessarily bad, but I think sometimes it's too much and actually hurts readability, eg here: 'size' => ( ( $file['size'] / 1024^3 ) / 1024 ),

Comments

The usability of your class would increase if you would add PHPDoc style comments, especially for the Process function and its arguments.

A lot of your in-code comments aren't really needed, such as Construct, Directory where the files are stored, Check for post, etc.

These kinds of comments are not necessary, as they just repeat what your code already says. They also distract from the actually important comments, thus making your code less readable.

Most of these comments, you can just remove. But some are in places where a comment would actually be helpful, and should be extended.

For example: Sort the files into a more readable array for SortFiles. Here, a comment describing what "more readable" is would be very helpful, because the method name is very generic.

Another example is Run through some checks for Verify. What are "some" checks?

Naming

• PrettyPrint doesn't print, which is confusing. Something like formatMessages might fit better.

StackExchange Code Review Q#105800, answer score: 3