OOP clarification

[fjm]

Ok, I spent the night writing a simple upload script that pretty much demonstrates what I am talking about. My first example sucked but the following is a class that can actually be used. The main goal of the class is to take a file from a user, clean the filename and upload it to the server. There are also options that the user can toggle in an array that's passed to the constructor.

When I designed this class, I didn't see any other public method that was needed, other than a simple message that lets the user know that something is wrong. It's because of this that I have made all of the members private as well as the methods.

Please have a look at it and let's discuss how it can be made better. I would also like to know if this is a decent class design.


Thanks,
Frank

<?php
class FileUpload
{
    private $_upload = array();
    private $_config = array();
    private $_fileName;
    private $_fileMime;
    private $_fileSize;
    private $_fileTemp;
    private $_fileError;
    private $_setFormName   = 'userfile';
    private $_setExtension;
    private $_setNewName;
    private $_setUploadPath = null;
    private $_setMaxSize    = 0;
    public  $message;

    public function __construct( array $upload, array $config )
    {
        $this->_upload = $upload;
        $this->_config = $config;
        $this->setOptions();
        $this->initialize();
    }

    private function setOptions()
    {
        $opts = $this->_config;
        foreach($opts as $opt => $val)
        {
            if(!empty($val))
            {
                $this->{$opt} = $val;
            }
        }
    }

    private function initialize()
    {
        $upload = $this->_upload;
        $form   = $this->_setFormName;
        if(!empty($upload["$form"]['name']))
        {
            if($this->testUploadDir())
            {
                $this->_fileName  = $this->_upload["$form"]['name'];
                $this->_fileMime  = $this->_upload["$form"]['type'];
                $this->_fileSize  = $this->_upload["$form"]['size'];
                $this->_fileTemp  = $this->_upload["$form"]['tmp_name'];
                $this->_fileError = $this->_upload["$form"]['error'];
                $this->checkExtension();
            }
        }
        else
        {
            $this->message = 'Please Select A File To Upload...';
        }
    }

    private function testUploadDir()
    {
        $path = $this->_setUploadPath;
        if(file_exists($path))
        {
            if(!is_readable($path))
            {
                $this->message = 'Upload Directory Not Readable';
                return false;
            }
            if(!is_writeable($path))
            {
                $this->message = 'Upload Directory Not Writeable';
                return false;
            }
        }
        else
        {
            $this->message = 'Upload Directory Does Not Exist';
            return false;
        }
        return true;
    }

    private function cleanFile()
    {
        $temp = $this->_fileName;
        $temp = strtolower($temp);
        $temp = str_replace(' ', '_', $temp);
        $result = '';
        for ($i=0; $i<strlen($temp); $i++)
        {
            if (preg_match("([0-9]|[a-z]|_|.)", $temp[$i]))
            {
                $result = $result . $temp[$i];
            }
        }
        $this->_fileName = $result;
    }

    private function checkExtension()
    {
        $this->cleanFile();
        $file   = $this->_fileName;
        $getExt = substr($file, strpos($file,'.'), strlen($file)-1);
        $setExt = $this->_setExtension;
        if($getExt != $setExt)
        {
            $this->message = 'The File You Attempted To Upload Is Not Allowed.';
            return false;
        }
        $this->checkSize();
    }

    private function checkSize()
    {
        $size = $this->_fileSize;
        $max  = $this->_setMaxSize;
        if($max != 0)
        {
            if($size > $max)
            {
                $this->message = 'The File You Attempted To Upload Is Too Large.';
                return false;
            }
        }
        $this->renameFile();
    }

    private function renameFile()
    {
        $old = $this->_fileName;
        $new = $this->_setNewName;
        if(!empty($new))
        {
            $this->_fileName = $new;
        }
        $this->moveFile();
    }

    private function moveFile()
    {
        $temp      = $this->_fileTemp;
        $file      = $this->_fileName;
        $path      = $this->_setUploadPath;
        $error     = $this->_fileError;
        if($error == 0)
        {
            if(move_uploaded_file($temp,$path.$file))
            {
                $this->message = 'Your File Upload Was Successful.';
            }
            else
            {
                $this->message = 'Your File Upload Failed While Moving It To Its Permanent Location.';
            }
        }
        else
        {
            $this->message = 'Your File Upload Failed. Errno: '.$error;
        }
    }
}

$file = $_FILES;
$config = array('_setFormName'=>'test','_setExtension'=>'.jpg','_setNewName'=>'new.jpg','_setUploadPath'  => './upload/','_setMaxSize'=>'0');
$test = new FileUpload($file,$config);
echo $test->message;
?>
<form method="post" action="upload.php" enctype="multipart/form-data">
  <input type="file" name="test" size="25">
  <input type="submit" name="submit" value="Upload">
</form>

[Atli]

Ok. A few comments on that.

First: Having all the members private and passing their values via an array into the constructor.

The first thing that worries me about that is the fact that for this to work, the outside code must be aware of how the private internals of the class work (the private member names).
Because of this, you can not alter the internals of the class without having to change how the class is used by the outside.
You should be able to change every privately declared member or method without any change to how the class is used from the outside.

It's best to identify the values that must be provided and require them as parameters for the constructor.
Then identify additional values that can be provided and either add them to the constructor parameter list as optional parameters (parameters with default values) or add setters for them.

Ideally all the required values should have getters, and setters if possible, and the optional values should have both getters and setters.
That way the outside can at least read the values the class is using, and when possible, change them if that is needed.

You can of course add global setter method that takes a list of values to be set, but it is not a good idea to tie the keys used by the list to the actual names of the private members. Better to use aliases that are linked to the members, so the members themselves can be changed without affecting the outside.

Second: Chaining the private methods, starting at the constructor.

What I mean by this is; having one method trigger another when it is done, leading to the end.

The trouble with this is that the members are doing more then their names would suggest. For example, your "renameFile " method triggers the "moveFile" method when it is done, which is confusing at best.

You are basically doing something like this:
[code=php]
class example {
function trigger() {
// Some code...
task1();
}
function task1() {
// Some code...
task2();
}
function task2() {
task3();
}
function task3() {
// Some code...
}
// etc...
}[/code]
If you plan to have one function trigger several others, like your code does, it would be better to execute them one by one from that function. Like:
[code=php]
class example {
function trigger() {
task1();
task2();
task3();
}
function task1() {
// Some code...
}
function task2() {
// Some code...
}
function taks3() {
// Some code...
}
// etc...
}[/code]
This way each of the functions in the chain is only responsible for a single task; the one it's name indicates, which makes it re-usable and not tied to the trigger chain.

This also allows you better control over how they are executed. Exceptions and error codes could be used to alter the chain once it has started.

Third: Having all the functionality of the class executed by the constructor.

Even tho I see why you would want this, it's generally not a good idea to have a class act as a function. It limits the class, requiring all future use of it to at least include the functionality the constructor is tied to.

What if you choose to add FTP support to your FileUpload class later on?
You can't do that in any decent way, without changing the class in such a way that would break every previous use of it.

The constructor should be used to initialize the object, and then public methods should be used to execute whichever functionality of that class you intend to use.

This allows you to add to the class later without having to rewrite the outside code.


And lastly, I would avoid relying on a public member to relay the class methods error messages. It's better to have the methods themselves return boolean success values or error numbers.
Or better yet; have them throw exceptions.

It just makes for much cleaner code.

That doesn't mean you can't have your class provide past error messages via a public member or a getter. I would just not force users of the class to use them.

[fjm]

Hold on.... I think I understand what you are saying here Atli.. So you mean.. it isn't good to tie all of my "functional ity" to the constructor because I limit the class. Yes? Here's what I think I am understanding.. By not tying al of my functionality to the constructor and by using getters and setters, I just use those for any current functionality I have or need and then if I require more functionality, I just add more getters and setters. With these getter and setter methods, I don't limit the outside code. Am I right?

Would it be possible to have you, in your spare time of course, maybe rework this class the way that you think it should be? Because I made that, I would be able to easily follow along on your changes and may be able to ask you more direct questions. The animal class was cool and I gained a lot of insight from it, but it isn't real world stuff. In fact, I looked it over and thought I was doing the same thing whan I made the upload class.

[QUOTE=Atli;3490 078]You should be able to change every privately declared member or method without any change to how the class is used from the outside.

Ok, is this what you mean by change privately declared member:

class Color
{
    private $_color;

    public function __construct()
    {
    }

    function getColor()
    {
      return $this->_color;
    }

    function setColor($color)
    {
      $this->_color = $color;
    }
}
$foo = new Color();
$foo->setColor('red');
echo $foo->getColor(); // outputs red

$foo->setColor('blue);
echo $foo->getColor(); //outputs blue

Notice that I didn't make the get and set methods private because they would throw an error when I tried to access them so I left them public but that isn't right, is it?

constructor parameter list as optional parameters (parameters with default values) or add setters for them.

Could you elaborate on this a bit? What is a parameter list? Are these the values passed into the constructor?

Better to use aliases that are linked to the members, so the members themselves can be changed without affecting the outside.

Can you provide a short example of this?

Chaining the private methods, starting at the constructor.

I kind of felt that was incorrect. I actually made like a trigger method as you had done in your example but was having a problem getting the methods to work correctly. method C was dependant on B and B was dependant on A. When I just listed them like you showed, All of the methods would execute. In order for me to get them to work like that, I had to nest them based on their return values. Was this right?

The constructor should be used to initialize the object, and then public methods should be used to execute whichever functionality of that class you intend to use.

When you say "initialize the object" what exactly does that mean? What if there are no params required for the constructor? Do you mean that I should initialze all my class members in the constructor?

It's better to have the methods themselves return boolean success values or error numbers.

Of course, if I did it this way, I'd have to make these methods public, right?

Or better yet; have them throw exceptions.

I've been using exceptions now for a couple of weeks. I'm still not 100% certain how an exception is really any different than a if/else statement. I'm sure there is a huge difference but I can't see it yet.

Thanks again Atli for all the help!

Frank

[Dormilich]

I've been using exceptions now for a couple of weeks. I'm still not 100% certain how an exception is really any different than a if/else statement. I'm sure there is a huge difference but I can't see it yet.

When an exception is thrown, code following the statement will not be executed, and PHP will attempt to find the first matching catch block.

that means, the Exception will descend through the function/method stack not executing any code until it is caught.

taking Atli's 2nd example, if an Exception is thrown in task1(), task2() and task3() are not executed and if the Exception is not caught at all a catchable fatal error is thrown.

[Atli]

Ok, this is a bit different from your class, but should at help explain what I'm talking about.

This is how I would have written a class like the one you posted.
(Been meaning to make a proper uploader class for ages anyways :P)

It is made so that you can move multiple files with one instance, so I removed most of the private members you used and added getter and setters for the once I left and added.

I also inlined most of the private methods into a public 'moveFile' method and a private 'validateFile' method, which do all the work.
The constructor just initializes the members and makes sure everything is ready.

I'm not sure how much you've used exceptions, but I use them in this class for error handling. Hope that doesn't get in your way.

I may have "over-commented" it a bit, but I always do that :P
(See an example of how to use it at the bottom)
[code=php]<?php
/**
* Handles file uploads from a HTTP POST request.
* @throws Exception
*/
class FileUpload
{
/** Property values **/
protected $_uploadDir; // Where to save the files
protected $_maxFileSize; // In bytes
protected $_allowedTypes; // A list of allowed mime types and their file extensions.

/**
* The constructor. All parameters are optional.
* @param [string] $uploadDir The directory where files should be moved.
* @param [integer] $maxFileSize The maximum size of a uploaded file in bytes.
* @throws Exception
*/
public function __construct($up loadDir="", $maxFileSize=20 97152)
{
// Verify that there were some files uploaded
if(!$_FILES || count($_FILES) <= 0) {
throw new Exception("No files were uploaded.");
}

// Initialize the properties
$this->setUploadDir($ uploadDir);
$this->_maxFileSize = $maxFileSize;
$this->_allowedType s = array(
'image/jpeg' => array('jpeg', 'jpg'),
'image/jpg' => array('jpeg', 'jpg'),
'image/png' => array('png'),
'image/gif' => array('gif')
);
}

/**
* Moves a file from it's temporary upload location to the upload dir.
* @param string $elemName The name given to the <input> element of the form
* @param [string] $newName A new name for the file, excluding the extension.
* @throws Exception
*/
public function moveFile($elemN ame, $newName=null)
{
try {
// Make sure the file is valid
$this->validateFile($ elemName);

// Parse the file name and extension
$baseName = basename($_FILE S[$elemName]['name']);
$fileName = substr($baseNam e, 0, strripos($baseN ame, "."));
$fileExt = substr($baseNam e, strripos($baseN ame, ".") + 1);

// Set and clean the name
if($newName != null) {
$fileName = $newName;
}
$fileName = $this->cleanFileName( $fileName);

// Move the file
$newPath = $this->_uploadDir . $fileName . $fileExt;
if(!move_upload ed_file($_FILES[$elemName]['tmp_name'], $newPath)) {
throw new Exception("Coul d not save file '{$newPath}'");
}
return true;
}
catch(Exception $ex) {
throw new Exception("File relocation failed: " . $ex->getMessage() );
}
}

/**
* Converts a filename into a 'clean' format.
* @param string $fileName The filename to clean
* @return string The result of the cleaning
*/
private function cleanFileName($ fileName)
{
$temp = $fileName;
$temp = strtolower($tem p);
$temp = str_replace(' ', '_', $temp);
$result = '';
for ($i=0; $i<strlen($temp ); $i++)
{
if (preg_match("([0-9]|[a-z]|_|.)", $temp[$i]))
{
$result = $result . $temp[$i];
}
}
return $result;
}

/**
* Makes sure a uploaded file is valid and ready to be moved.
* @param string $elemName The name given to the <input> element of the form
* @return bool
* @throws Exception
*/
private function validateFile($e lemName)
{
// Make sure the element exists
if(!isset($_FIL ES[$elemName])) {
throw new Exception("No upload named '{$elemName}' was found");
}

// Make sure the file was uploaded without a error
if($_FILES[$elemName]['error'] != 0) {
throw new Exception("File upload failed with code #". $_FILES[$elemName]['error']);
}

// Make sure the file extension and mime type are valid
$fileExt = substr($_FILES[$elemName]['name'], strripos($_FILE S[$elemName]['name'], ".") + 1);
$fileMime = $_FILES[$elemName]['type'];

if(!in_array($f ileMime, array_keys($thi s->_allowedTypes) ) ||
!in_array($file Ext, $this->_allowedType s[$fileMime]))
{
throw new Exception("File mime or extension is not valid.");
}

// Make sure the file size is correct and within limits
if($_FILES[$elemName]['size'] != filesize($_FILE S[$elemName]['tmp_name'])) {
throw new Exception("Uplo aded size and actual size do not match");
}
if($_FILES[$elemName]['size'] > $this->_maxFileSize ) {
throw new Exception("File size is to big");
}
return true;
}

/** UploadDir and MaxFileSize properties (getters and setters) **/
public function setUploadDir($n ewPath) {
// Format the path
$newPath = str_replace('\\ \\', "/", $newPath);
if($newPath[strlen($newPath )-1] != "/") {
$newPath .= "/";
}

// Make sure the path is accessible
if(file_exists( $newPath) && is_writeable($n ewPath)) {
$this->_uploadDir = $newPath;
}
else {
throw new Exception('Uplo ad directory is inaccessible.') ;
}
}
public function getUploadDir() {
return $this->_uploadDir;
}
public function setMaxFileSize( $newValue) {
if(is_integer($ newValue) && $newValue > 0) {
$this->_maxFileSize = $newValue;
}
else {
throw new Exception("The max file size must be a positive integer.");
}
}
public function getMaxFileSize( ) {
return $this->_maxFileSize ;
}

/** Getters and setters to add / retrive allowed mime types and extensions */
public function addAllowedMimeT ype($mime, $extensions) {
foreach($extens ions as $_ext) {
$_ext = strtolower($_ex t);
if(!array_key_e xists($mime, $this->_allowedType s) ||
!array_search($ _ext, $this->_allowedType s[$mime]))
{
$this->_allowedType s[$mime][] = $_ext;
}
}
}
public function getAllowedMimeT ypes() {
return array_keys($thi s->_allowedTypes) ;
}
public function getAllowedMimeE xtensions($mime Type) {
if(array_key_ex ists($mimeType, $this->_allowedTypes) ) {
return $this->_allowedType s[$mimeType];
}
}
public function getAllowedExten sions() {
$outputArray = array();
foreach($this->_allowedType s as $_type) {
$diff = array_diff($_ty pe, $outputArray);
$outputArray = array_merge($ou tputArray, $diff);
}
return $outputArray;
}
}[/code]
Which could be used like so:
[code=php]<form method="post" action="?" enctype="multip art/form-data">
<input type="file" name="imageFile " /><br />
<input type="file" name="mp3File" /><br />
<input type="submit" name="submit" value="Upload" />
</form>
<?php
if(isset($_POST['submit'])) {
require("FileUp load.php");
try {
// Create a FileUploader, specifying the upload path in the constructor
$uploader = new FileUpload("ima ges");

// Move the image file using the default settings
$uploader->moveFile("imag eFile");
echo "<h3>First file uploaded</h3>";

// Change the the upload path and max file size.
$uploader->setUploadDir(" music/");
$uploader->setMaxFileSize (10485760); // 10MB

// Allow the Mp3 mime and extension and move the Mp3 file.
$uploader->addAllowedMime Type("audio/mpeg", array("mp3"));
$uploader->moveFile("mp3F ile");
echo "<h3>Second file uploaded</h3>";

// List all allowed mime types and their extensions
echo "<pre>";
foreach($upload er->getAllowedMime Types() as $_mime) {
echo "<strong>{$_mim e}</strong> (";
$extensions = $uploader->getAllowedMime Extensions($_mi me);
echo implode(", ", $extensions), ")\n";
}
echo "</pre>";

}
catch(Exception $ex) {
// Show error message
echo "<div><h3>< span style='color: red;'>Exception cought!</font></h3>
<pre>", $ex->getMessage() , "</pre></div>";
}
}
?>[/code]

[Atli]

To answer a couple of your questions...

Ok, is this what you mean by change privately declared member:

In a way, yes.
What I mean is that you should be able to completely change the interior of the class (the private members and methods) without the outside code ever knowing about it.

Using properties (setters and getters) can go a long way towards that.

Could you elaborate on this a bit? What is a parameter list? Are these the values passed into the constructor?

Yea, that's exactly what I mean.
If you specify a default value for one of these parameters it is considered optional.
(See the constructor and moveFile method in the code in my previous post)

I kind of felt that was incorrect. I actually made like a trigger method as you had done in your example but was having a problem getting the methods to work correctly. method C was dependant on B and B was dependant on A. When I just listed them like you showed, All of the methods would execute. In order for me to get them to work like that, I had to nest them based on their return values. Was this right?

If you are going to use return values for error handling, then yes, that is how you should do it.
If the success or failure of function A affects whether function B should be executed, you will need to capture it's return value and check it before executing function B, just as you probably did to begin with :)

Exceptions give you an alternative to this, so you don't neccissarilly have to check the return values, but rather try to catch the exception.
It can make the code easier to manage, especially if you are nesting function calls very deeply.

When you say "initialize the object" what exactly does that mean? What if there are no params required for the constructor? Do you mean that I should initialze all my class members in the constructor?

That is the general idea, yes.
PHP is a loosely typed language, so initializing members isn't as vital as in other languages (C/C++ and such), but you should at least always keep it in mind when writing the constructor.

Of course, if I did it this way, I'd have to make these methods public, right?

Not really, no.
Private methods can return values just as public once can. They just can't be called by anything outside the class.

The return values can still be very useful within the class itself.

[Dormilich]

just another note on Exceptions:

the Exception::getT race() and Exception::getT raceAsString() methods can be very valuable in the debugging process. (these values should not be visible to any visitors, because they reveal quite some info about your script).

[dlite922]

Glancing over the discussions since my last post i'd like to add a couple of comments:

1. Frank, I think you're treating a class like it's a script. Two things lead me to this; one is the quote:

The main goal of the class is to take a file from a user, clean the filename and upload it to the server.

. And second is how your constructor is sort of like a trigger and creates a domino affect. A class is used BY another program or another class. This I think is explained well in Atli's "Third" point several posts back.

2. I do not use getters and setters if all they do is directly alter the private member. Might as well make it public anyway. You should have functions that are like "verbs" or actions to the class. For example if the class was a car, accelerate(valu e) , turnOnHeadlight s() etc. while gas-pedal, headlight, wheel where private members. Say to yourself if this was a real life object what would it "do".

Those are the two points I had. See you guys later,




Dan

[fjm]

Thanks for the replies guys. I think I will re-read this entire post and see if I can rework this example class trying to correct the mistakes and repost it. Thanks for helping me out on this. :)

[fjm]

Atli, I have been looking over your upload class and I am curious as to why you allow the moveFile method to be accessible from the outside? I'm not disagreeing with you but only looking to understand your logic.

To me, the moveFile method should probably be made part of the internal class itself away from the outside, because the whole job of a file upload class is to move and upload a file anyway, right?

Why would you tell a file upload class to upload the file from the outside? It just doesn't make sense to me. Could you please explain?

Thanks. :)

[Atli]

You should never assign classes a single job. They are supposed to provide functionality for some specific area, providing methods to do specific tasks.

It this case, the class provides methods to control file uploads.
The class itself, however, is not confined to just moving a single file.

This specific class might not be the best example of this.

But picture a MusicPlayback class.
It would provide methods to control the playback of music files.
Including methods to: play, pause, stop, rewind, etc...

The class itself is not tied to a single task, and therefore must provide publicly declared methods to trigger the tasks.

That's why I declare the moveFile method public; because that is the way the outside triggers that particular task.

[fjm]

Heya Atli,

I totally understand where you are coming from with your example. For some reason... My mind wants to gravitate toward making a class for each function to be performed. In your example, instead of making a music playback class, I would have wanted to make a play class, a pause class, a stop class, etc. To me, it just seems cleaner to do it like that. In fact, I went back through a program I made where I had originally designed 5 classes about 500 lines each so not very big. I went back and seperated out all of my sql and made them their own classes. There were methods in these larger files that had nothing to do with the database so I figured it would be cleaner to seperate these database methods into its own class.

Do you see any problem in creating db models like I am suggesting? For example, a customer class that handles only the CRUD, etc.

Of course, I'm certain that I'm wrong but that's why I'm here. :)

[fjm]

Atli, I forgot to ask in my last post but in your upload class, I see that you have multiple getters and setters but that only 2 of them are actually being called. Am I reading this right? If I am, I am wondering why you have them in there?

For example.. You have the following that are not being used, or called:
getUploadDir()
getMaxFileSize( )
getAllowedExten sions()

Was this intentional?

[Markus]

Atli, I forgot to ask in my last post but in your upload class, I see that you have multiple getters and setters but that only 2 of them are actually being called. Am I reading this right? If I am, I am wondering why you have them in there?

For example.. You have the following that are not being used, or called:
getUploadDir()
getMaxFileSize( )
getAllowedExten sions()

Was this intentional?

Just because they aren't called in his example, doesn't mean they won't be called in others. You've got to think about the long-term, and what users will need from the class.

Also, you will do your best to separate any data stuff (SQL, file reading, etc) out of your classes into models. This is a key concept in OOP, and very helpful.

[Dormilich]

Also, you will do your best to separate any data stuff (SQL, file reading, etc) out of your classes into models.

How do I have to imagine that? simply by making another class?