OOP clarification

[fjm]

Hey everyone. I'm back in search of a better understanding of OOP. I feel like these past couple of months have paid off for me because I am at a point where I am really beginning to understand how this all works. I'd like to get some clarification on a few things first.

My questions stem from wanting to use private members and methods. I know that there is a reason for making methods and members public and private and honestly I'm still not totally clear as to when to use one over the other. I have been refactoring some of old classes and in looking at them, I was mortified. I figured that when I was redoing them that I should try and stick with private members and methods where possible because that's a good thing I guess.

I had download an app a while back that used 5 classes. Each class was like 1500 lines or so and the code looked clean to me. I figured that was a good way to program. I started to do the same thing but after looking over some of my classes, I quickly noticed that my classes were nothing more than functions instead of objects.

In refactoring this code, I separated out the methods that required functionality inside the class and set my constructor to call a main method that would do what was needed and then return out the values I needed. I just want to make certain that I understand correctly before I continue any further.

If I have a class like so:

class Hello{
    public  $out;
    private $text;
 
    public function __construct($text)
    {
        $this->text = $text;
        $this->sayHello();
    }
 
    private function sayHello()
    {
        $text = $this->text;
        $var = 'Hello, my name is: ' . $text;
        return $this->out = $var;
    }
}
 
$hello = new Hello('Tom');
echo $hello->out;

To me, this seems like the correct way to write a class. I am returning out the values that I want. The way I was doing it in the past was to call the method directly and didn’t seem right.

Does this look correct? I have a few more questions but I'll wait on those for now to see what the feedback is on this. I'll also show you how I was doing before.

Thank!

Frank

[Dormilich]

I wouldn't have named the method "sayHello" because it's not 'saying' (aka printing/outputting) anything, but generally it's right. personally, I don't see the necessity of every method having a return value (esp. when you have printing methods) though if possible I let it return true/false.

on the other hand, you're not using the return value…

some simplifications I'd make

class Hello
{
    public  $out;
    private $text;
 
    public function __construct($text)
    {
        $this->text = $text;
        $this->out = $this->sayHello();
        // $this->sayHello();
    }
 
    private function sayHello()
    {
        return 'Hello, my name is: ' . $this->text;
        // $this->out = 'Hello, my name is: ' . $this->text;
    }
}

for this simple class there could even be more improvements, like omitting Hello->out and using sayHello() or even __toString() for output

[fjm]

@Dormilich
Ok, I understand that. Thanks. :)

In your example where you said that you would shorten it to sayHello(), isn't this acting more like a simple function than a class? This was the way that I was using it before. Because I had so many of these types of methods in a single class and would call them directly, I couldn't keep the methods private.

I know that in your example you are using the constructor to call the sayHello method but what if you have more than 1 or even 10 of these type of methods that require one or two variables? What do you do? How do you call them?

$foo = new MyClass();
echo $foo->myMethod($var);

My example above requires that the myMethod() method remain public. Is this not a big deal? Also, say that I have 20 of these exact type of methods in the class that each expect one or two variables passed. It seems to me that if I am passing variables directly into the methods that I should forgo the class and just use a function. No? I don't see the difference or the advantage in using your sayHello() example over a simple function.

Thanks for the help Dormi. :)

[Dormilich]

@fjm
well, yea, but do you want to handle any output through properties? (that may work for 1 or 2, but what about undefined/several different outputs?)

@fjm
methods are not supposed to be solely private, what use would the object then have (and what use would the interface have)?

private methods are good, if you need tasks done that shouldn't be initialized from the outside (like e.g. connecting to a database or loading a configuration)

@fjm
???

@fjm
no

@fjm
not every method expects a variable input, often you can use the properties to pass around variables.

in fact your example could be handled better by a function. but there are situations, where objects are better. I can think of writing an batch mail where the email addresses are stored in a DB.

OOP is more than using private methods. you also have (to name some):
- data encapsulation (prevents globals running wild)
- inheritance (share code, re-usable)
- patterns for common tasks (e.g. Singleton, Observer, Factory)
- magic methods (kind of event handling)

[Atli]

Hi.

A couple of things I'd like to comment on...

First.
I think your assumption that methods should preferably be private is not really correct.

Private members are only accessible from within the class itself, so any functionality or data you want accessible from the outside should (obviously) not be made private.
Private members should complement the public once, but never be a requirement. Ideally, you should be able to inline ALL private functions into the public functions without having to change how the outside code uses the class.

It's best to design classes in a way that allows you to redesign the entire interior of the class without having to change the publicly declared functions and variables. (Check out interfaces to see what I mean)

Second.
The way in which you designed your example class is very odd, really.
It has no public functions, which basically just makes it a complex array with a constructor.

Objects are meant to be more than just a collection of variables or a collection of anonymous functions. Arrays and normal functions work fine for that.

Class members should work together.
Public members being the controllers; the way the outside interacts with it.
Private members providing functionality for the public functions; the internal mechanism that the outside doesn't need to be bothered with.

Consider this alteration to your original example:
(Sorry about the doc blocks... can't seem to not add them :P)
(Edit... turns out I could :D)

<?php
class Hello {
    private $name;
     private $language;
 
    public function __construct($name, $language="enUS") {
        $this->name = $name;
        $this->language = $language;
    }
 
    public function sayHello() {
        echo "<h3>", $this->createHello(), "</h3>";
    }
 
    public function getHello() {
        return "<h3>". $this->createHello() ."</h3>";
    }
 
    private function createHello() {
        if($this->language == "isIS") {
            return "Halló, ég heiti {$this->name}.";
        }
        else { // Defaults to enUS
            return "Hello, my name is {$this->name}.";
        }
 
    }
}
 
// Say it in english
$eng = new Hello('Tom');
$ice = new Hello('Atli', 'isIS');
 
$eng ->sayHello();
echo "<center>", $ice->getHello(), "</center>";
?>

I've implemented both a "sayHello" and a "getHello" function, both using the "createHello" function to provide the message.

As the "createHello" function was only ever meant to be used by the other class functions, it is made private; inaccessible to the outside.
The other two, meant to provide functionality to outsiders, are public.

I've also added the ability to change languages of the message.
The "createHello" function is the only function that is actually aware there is a language choice (besides the constructor), but because of the cooperation between the functions, the effect is shared between them.

This also means that if I ever need to change the message, I only ever need to change it in one place.
Likewise, if I ever need to change the way the hello message is printed, that is in one place, rather than scattered all over your outside code, as it would have been with your example.

Do you see what I'm trying to say?

[fjm]

@Dormilich
Is this not acceptable?

Originally Posted by fjm
I know that in your example you are using the constructor to call the sayHello method but what if you have more than 1 or even 10 of these type of methods that require one or two variables? What do you do? How do you call them?

I am under the impression that each class should have a single function (and not in the literal sense). Let me give you an example. I have a class with two methods. The first method gets data from a database then internally passes the values to the second method where the data is checked then returned to the main script. In the constructor, I call the first method which starts the process. My thoughts are that the class should only do one thing. You can feed it the var(s) and it returns what you need.

class Hello{
    public  $out;
    private $text;
 
    public function __construct($text)
    {
        $this->text = $text;
        $this->sayHello();
    }
 
    private function sayHello()
    {
        $text = $this->text;
        $var = 'Hello, my name is: ' . $text;
        return $this->out = $var;
    }
 
    public function doSomething()
    {
        $val = 'Do something here';
        return $val;
    }
}
 
$hello = new Hello('Tom');
echo $hello->out;

In my original example, I added a doSomething method. Let's just say that this class was for nothing more than to say hello. I know that it isn't useful and I could come up with a better example but it conveys my point I think.

So, with this new public method, say that I wanted to call that method. I first have to create an instance but the constructor is looking for a var that has nothing to do with this doSomething method. How would I work this? Do you see what I mean?

private methods are good, if you need tasks done that shouldn't be initialized from the outside (like e.g. connecting to a database or loading a configuration)

Yes, I am seeing exactly what you mean. A good example of this would be a login script where the users' password should be made private.

[fjm]

Heya Atli. I'm glad that you joined in to help. :)

@Atli
OK. I understand this acutally. I was just thinking that maybe it would be better to make methods private where possible for added security but I see that isn't always a good thing or even realistic especially after seeing your example.

@Atli
OK, I understand that. I guess my question is what if a public method really has nothing to do with any of the other methods? Let's say that you have several methods that just draw data from the database and the sql requires a var for the WHERE statement. We would have to pass that variable into the method. I guess what I am trying to say here is that I am not seeing much difference between a regular function and a public method where the public methods sole purpose in life is to return data from a database. Now, if that public function actually called a private function for something, I could see it. Am I wrong? Thanks for the link on interfaces, I will check that out.


@Atli
OK. This goes back to what I was saying about the constructor. Instead of using a public method to do this, I thought the constructor was supposed to handle this which was why I designed it that way.

@Atli
If I'm understanding you right, you're saying that instead of using the constructor to interact with the private methods, I should be using public functions. Yes?

I think that maybe your example is even better than mine because it illistrates my point even better. In your example, if you wanted to use another public method that has NOTHING AT ALL to do with any of the other methods, what do you do? Do you make another class for it? Say for example that you wanted to have another function that said goodbye but instead of using the person's first name, you would be required to use their last name. I know, I know... it's a really dumb example but it makes the point. :) Would you use a new class for this or use this same class?

I understand what you are saying, more so now then ever before. That's why I'm nitpicking here. I'm honestly getting this for a change. :)

[fjm]

Atli,

I have been doing a bit more reading on this and have looked over the link on interface. Not certain exactly how that would help me out just yet but of course it's because I don't fully inderstand it yet. I will re-read it and look for some additional info on that.

Regarding post #5, in your example... Your private method acts as a mutator and the public getHello acts as an accessor, right?

Could we say that by having a mutator and accessor that we are encapsulating the data within the class? That's what it looks like to me. Am I right?

[Dormilich]

@fjm
interfaces are a way for an outside developer to use your classes (think of the interface as API). conforming to an interface lets you use a class, without knowing the (internal) structure of this class.

when implementing an interface in a class definition, you make sure, that the required functionality is given (otherwise there will be an error).

an example may be found here

interfaces can also be used to check if an object has a (set of) public method you want to use later. (esp. when there are user defined classes involved)

(Atli can probably descibe that better…)

[Atli]

OK, I understand that. I guess my question is what if a public method really has nothing to do with any of the other methods? Let's say that you have several methods that just draw data from the database and the sql requires a var for the WHERE statement. We would have to pass that variable into the method. I guess what I am trying to say here is that I am not seeing much difference between a regular function and a public method where the public methods sole purpose in life is to return data from a database. Now, if that public function actually called a private function for something, I could see it. Am I wrong?

Depends on how you look at it, and might well be a matter of personal preference.

Would the function be using any class variables?
Would it even be related to the class itself?

If both are false, then this function might indeed not belong to that class, but rather by itself or in a different class.
If only the second one is true, the you should consider making it a static method. (A method available even without creating an instance of the class.)

Like, say, in a classic animal class:

• A method "speak" would be specific to an instance of the class, using the members for that instance. Clearly belongs to the class.
• A method "getAllByType" would not be specific to an instance, nor related in any way to any of the other members or methods. It should, however, belong to the animal class, as it does provide functionality relating to the class. (A perfect example of a static method.)
• A method "setUpAminalDatabase", despite being somewhat related to the Animal class, provides no real functionality for the class itself. It should really belong to a separate, more database related, class... or just in a solo function somewhere.

I would imagine structuring a Animal class somewhat like:

class Animal {
    /** Members **/
    private $type;
    private $name;
 
    /** Constructors / Destructors **/
    public function __construct($type, $name, $insert=false){
        $this->type = $type;
        $this->name = $name;
        if($insert) {
            $this->addToDatabase();
        }
    }
 
    /** Instance methods **/
    public function speak() {
        echo ucfirst($this->getSound()), ", my name is {$this->name}, the {$this->type}. <br>\n";
    }
 
    private function getSound() {
        switch($this->type)
        {
            default: return "yum yum"; break; // Couldn't think of anything else :P
            case "dog": return "woof woof"; break;
            case "cat": return "meow"; break;
        }
    }
 
    public function addToDatabase() {
        $sql = "INSERT INTO animals(name, type) VALUES('{$this->name}', '{$this->type}')";
        return @mysql_query($sql) == true;
    }
 
    /** Static methods **/
    public static function getAllByType($type="dog") {
        $sql = "SELECT name FROM animals WHERE type = '{$type}'";
        if($result = @mysql_query($sql)) {
            $output = array();
            while($row = mysql_fetch_assoc($result)) {
                $output[] = new Animal($type, $row['name']);
            }
            return $output;
        }
        else {
            return array();
        }
    }
}
?>

To be used like:

<?php
// Create a dog and a whale
$cat = new Animal("cat", "Michell"); // Not added to the DB
$whale = new Animal("whale", "Bob", true); // Added to the DB
 
// Get all dogs already in the database
$dogs = Animal::getAllByType("dog");
 
// Make em all speak
foreach($dogs as $_dog) {
    $_dog->speak();
}
$cat->speak();
$whale->speak();
?>

The method "getAllByType" there being a key part of the class, despite the fact that it is detached from the rest of it, and that it only really performs a simple database query.

In your example, if you wanted to use another public method that has NOTHING AT ALL to do with any of the other methods, what do you do? Do you make another class for it? Say for example that you wanted to have another function that said goodbye but instead of using the person's first name, you would be required to use their last name.

Even if the method does nothing but take a name and echo it back to you in a "You have been logged out, $lastname. Goodbye." sort of way, If it is related to the class (a User class, in this scenario), then it should belong to it.
Even tho it may seem pointless to add this to the class, it may become imporant later on.

Imagine if, after using this goodbye method in 200 pages, the requirements are changed. You are now required to print a language specific message depending on the language set in the constructor. It's a simple change if the method was in the class from the start. Not so simple if it was a rogue function.

OK. This goes back to what I was saying about the constructor. Instead of using a public method to do this, I thought the constructor was supposed to handle this which was why I designed it that way.

The constructor is only supposed to handle the initialization of the class; making sure all the members are set and ready to be used by the methods.
Likewise, destructors are meant to unset members and free resources.

This includes tasks like opening/closing database connections and running security algorithms. Anything not meant to be accessed by the outside, but needs to be run every time the object is created.

You can of course go beyond that if you want to, but be very careful. Bloated constructors are never good.
Providing a public method is usually better, and much cleaner.

It's also common to use constructor parameters as shortcuts to execute commonly used methods. (Like I do in the Animal example above.)

I have been doing a bit more reading on this and have looked over the link on interface. Not certain exactly how that would help me out just yet but of course it's because I don't fully inderstand it yet. I will re-read it and look for some additional info on that.

Using Interfaces isn't vital, nor really needed, in any one class.
Especially not since the class itself serves as a blueprint for how the class is meant to be used.

Interfaces are used to describe a group of classes, who are all meant to provide the same... well, interface to be used by other parts of the code.

Kind of like cars... They all share the same interface. A wheel, a gear shift, pedals, etc..
We can always be sure every car is built around this same basic interface, even tho they all implement it differently beneath the surface, and most of them add additional functionality on top.

This is exactly what interfaces do for classes. They ensure that classes provide at least the methods specified in the Interfaces they implement.
They help maintain the integrity of the classes from a design perspective, but add little to how the classes themselves are actually built or used.
(Any class built upon an interface could simply remove the implements keyword and function exactly the same afterwards.)

[dlite922]

Frank,

To summarize, Classes should sort of "group" certain behavior or functionality together. For formatting an output (ie your hello example) a function would do, but let's say you had a formatter class that contained several of these things and were used through out your application. To this point, one of the core values of OOP is code reuse.

Also use OOP to define "objects" instead of "functions". For example your application might have a User class, and ATM class, and a Storage class instead of having function classed like "Deposits", or "Transaction" classes. These functions will fall into behaviors of the objects themselves, ie

User::setBalance("5000");
User::changeAddress("123 Main");
ATM::getMenuOptions();
Storage::recordTransaction();

To me, it looks like you're on the right track. As you continue to read and write code, you'll be able to see what the best way to implement classes keeping the values of OOP in mind: Encapsulation, Code Reuse, Divide and concur the problem, and Bug Containment.

Next topic I recommend for you is Exceptions/Error handling.

BTW: I'd like to point out this has been the best OOP thread on bytes and see how others could benefit from it.

See you around!




Dan

[Markus]

@Atli
Very well said. Great read :)

[fjm]

There is some really great information here. Thanks for all the help guys. I am going to digest this and respond this weekend when I have a little more free time. I've already read this over once and will re-read it again this weekend before I respond. I'm not going to let this thread die!! :)

[Markus]

@fjm
Go to your local library and find some books on programming design patterns / OOP. It doesn't really matter what language they're in; the concepts are relatively the same.

[fjm]

Thanks for the advice Markus. I'll do that. I seem to do better with books anyhow.

[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:

class example {
  function trigger() {
    // Some code...
    task1();
  }
  function task1() {
    // Some code...
    task2();
  }
  function task2() {
    task3();
  }
  function task3() {
    // Some code...
  }
  // etc...
}

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:

class example {
  function trigger() {
    task1();
    task2();
    task3();
  }
  function task1() {
    // Some code...
  }
  function task2() {
    // Some code...
  }
  function taks3() {
    // Some code...
  }
  // etc...
}

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 "functionality" 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;3490078]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?


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

@Atli
Can you provide a short example of this?

@Atli
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?

@Atli
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?

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

@Atli
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]

@fjm

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)

<?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($uploadDir="", $maxFileSize=2097152)
    {
        // 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->_allowedTypes = 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($elemName, $newName=null)
    {
        try {
            // Make sure the file is valid
            $this->validateFile($elemName);
 
            // Parse the file name and extension
            $baseName = basename($_FILES[$elemName]['name']);
            $fileName = substr($baseName, 0, strripos($baseName, "."));
            $fileExt = substr($baseName, strripos($baseName, ".") + 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_uploaded_file($_FILES[$elemName]['tmp_name'], $newPath)) {
                throw new Exception("Could 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($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];
            }
        }
        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($elemName)
    {
        // Make sure the element exists
        if(!isset($_FILES[$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($_FILES[$elemName]['name'], ".") + 1);
        $fileMime = $_FILES[$elemName]['type'];
 
        if(!in_array($fileMime, array_keys($this->_allowedTypes)) ||
            !in_array($fileExt, $this->_allowedTypes[$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($_FILES[$elemName]['tmp_name'])) {
            throw new Exception("Uploaded 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($newPath) {
        // 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($newPath)) {
            $this->_uploadDir = $newPath;
        }
        else {
            throw new Exception('Upload 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 addAllowedMimeType($mime, $extensions) {
        foreach($extensions as $_ext) {
            $_ext = strtolower($_ext);
            if(!array_key_exists($mime, $this->_allowedTypes) ||
               !array_search($_ext, $this->_allowedTypes[$mime]))
            {
                $this->_allowedTypes[$mime][] = $_ext;
            }
        }
    }
    public function getAllowedMimeTypes() {
        return array_keys($this->_allowedTypes);
    }
    public function getAllowedMimeExtensions($mimeType) {
        if(array_key_exists($mimeType, $this->_allowedTypes)) {
            return $this->_allowedTypes[$mimeType];
        }
    }
    public function getAllowedExtensions() {
        $outputArray = array();
        foreach($this->_allowedTypes as $_type) {
            $diff = array_diff($_type, $outputArray);
            $outputArray = array_merge($outputArray, $diff);
        }
        return $outputArray;
    }
}

Which could be used like so:

<form method="post" action="?" enctype="multipart/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("FileUpload.php");
    try {
        // Create a FileUploader, specifying the upload path in the constructor
        $uploader = new FileUpload("images");
 
        // Move the image file using the default settings
        $uploader->moveFile("imageFile");
        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->addAllowedMimeType("audio/mpeg", array("mp3"));
        $uploader->moveFile("mp3File");
        echo "<h3>Second file uploaded</h3>";
 
        // List all allowed mime types and their extensions
        echo "<pre>";
        foreach($uploader->getAllowedMimeTypes() as $_mime) {
            echo "<strong>{$_mime}</strong> (";
            $extensions = $uploader->getAllowedMimeExtensions($_mime);
            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>";
    }
}
?>

[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::getTrace() and Exception::getTraceAsString() 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(value) , turnOnHeadlights() 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()
getAllowedExtensions()

Was this intentional?

[Markus]

@fjm
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]

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

[Markus]

@Dormilich
Yes. A model is a data class (think MVC).

 
class UserInfoModel extends DatabaseAbstraction {
 
    public function userIDByName(string $name);
    public function nameByUserID(int $id);
 
}
 
 

[dlite922]

frank,

Like you said, you seem to gravitate towards a single-function class. If me and you were to design a CAR class:

testDrive.php // uses car class
car.class.php // the car class

you'd be more likely to create one that would start, drive forward, turn left, turn right, reverse and stop all with one call to the class.

If I designed it, I would do it so that it can do it in that same order you did AND most importantly in /any/ other combination possible. For example I could make it start, make 4 left turns and stop WITHOUT even changing my class file, i'd make another file that uses it and call it testDriveTwo.php.

My point demonstrates two important benefits of OOP: Extensibility and Reuse.

Think like the manufacture of the object. Would one design a microwave that /only/ reheated pizzas?

Hope that clarifies that subject of Reuse.




Dan

[Atli]

@Markus
Can't really agree with you on that one.
Models, and the concept of separating data-interaction from the business logic, belong to the MVC and tier-3 design patterns, not OOP.
(Although, OOP designs tend to end up in that territory.)

OOP is just the tool used to implement these patterns.
You don't have to know how to read a blueprint to operate a hammer :P

I do agree tho that once you start building OOP applications, using these design patterns is a very very good idea. The separation of the data, business and presentation layers is essential in any large application.

[Atli]

@dlite922
My microwave only makes popcorn... probably not by design tho :P

But yea, that's exactly the point I am trying to make.

Classes are more like function libraries than single functions.
They should provide the option of performing tasks, but not simply perform a single task on creation.

[Markus]

@Atli
I mean encapsulation and abstraction. I use the term 'OOP' too loosely.

Mark (also still a newbie(ish) at OOP).

[Atli]

@Markus
Well, PHP is a loosely typed language.
Maybe I just need to loosen up a bit :)

But, yea.
Encapsulation and abstractation / interfaces are very important OOP concepts.
This thread covers the first one. Next thread: Abstractation :)

Inheritance and polymorphism should also make that list.
Although you can't really talk about abstractation without inheritance, can you...

[Dormilich]

@Atli
as long as we're not discussing class based (PHP) and prototype based (JavaScript) inheritance… just had a look at OOP @ wikipedia

[Atli]

@Dormilich
JavaScript doesn't have inheritance (or abstractation, for that matter)... It has extensibility.
It has no concept of classes (object blueprints), so there is nothing to inherit.

You can only extend preexisting objects, which are always initially created as blank objects, and then initialized by either adding prototypes or just adding properties (which are essientially just array elements gone wild).

... or so I have been lead to believe. I'm no Javascript expert, really :P

[Dormilich]

@Atli
I didn't state that for nothing…

if JavaScript would have had classes, it wouldn't be prototype-based… and looking from this side, what is the difference between inheritance and extensibility then (I dimly remember there is a keyword "extends" in PHP)?

[Atli]

@Dormilich
O, so we were actually saying the same thing?
I may have misunderstood you.
(I was/am very tired :P)

@Dormilich
Well, the way I see it...

Classes aren't objects. They are blueprints for creating objects.
So when you extend a class, you are inheriting instructions on how to build an object, which you can use at runtime to build the object the child class describes.

Prototype-based objects are already objects.
Extending such an object involves "physically" cloning the object and then modifying the newly existing clone.

It's like... Say there is an old car you want to upgrade.
On one hand (prototype-based), you could take the old car, build another one exactly like it, and then modify it to apply your upgrades.
Or (class-based), you could modify the blueprint for the old car and build a new one based on the modified blueprint.

[Dormilich]

@Atli
well, objects are object, whether they come from a blueprint or not. you can also look at inheritance like that: if an object uses a member, that is not part of it's own declaration, then there is inheritance. how that member was attached to the object is of no concern.

@Atli
or creating the object from scratch.

var obj = {}; // empty object like stdClass

@Atli
and in either case you have a new car with extended functionality. and your neighbour wouldn't know if you used a paper plan or daddy's old car for the blueprint.

EDIT: what I wanted to say, is inheritance depending only on classes? or is it like OOP more of a concept?

[Atli]

You've got a point there.

Inheritance or extensibility... I guess that's just semantics. Both achieve the same thing, even tho they use different methods; an object that inherits it's base functionality from another object/class.

[Dormilich]

just because I'm reading about JS inheritance… an example

// by Gavin Kistner
// because there is no native "extend" functionality
Function.prototype.extends = function( parentClassOrObject )
{
    if ( parentClassOrObject.constructor == Function )
    {
        //Normal Inheritance
        this.prototype = new parentClassOrObject; // this is the inheritance
        this.prototype.constructor = this; // reset the constructor name
        this.prototype.parent = parentClassOrObject.prototype; // attaching prototype chain
    }
    else
    {
        //Pure Virtual Inheritance
        this.prototype = parentClassOrObject;
        this.prototype.constructor = this;
        this.prototype.parent = parentClassOrObject;
    }
    return this;
}

/* and now for the example part */
 
Lebewesen = { 
    die: function() {
        this.state = "dead";
    },
    born: function() {
        this.state = "alive";
    } 
}
 
function Animal()
{
    this.born();
    this.type = "Animal";
}
 
Animal.extends(Lebewesen);
 
Animal.prototype.toString = function()
{
    return this.state;
}
 
Animal.prototype.getType = function()
{
    alert(this.type);
}
 
function Horse(name)
{
    this.born();
    this.type = "Horse";
    this.name = name;
}
 
Horse.extends(Animal);
 
var x = new Horse("Binky");
x.getType();
x.die();
alert(x);

[dlite922]

So what's the object oriented way of getting rich?

$$$$$$$$$$

[Atli]

@dlite922

$this->money++;

8-)

[fjm]

Hey!! I never got notification that this post was updated since I last posted here!!! :(

Well, I'm glad to see that this thread is still going. I came back today to tell you guys that I now see the point about designing classes where it won't affect your application code. Here's what happened to me this past weekend....

I needed to add some functionality to my app and when I went into the class to make the change, everything was cool..... that is, until I started the app again and saw that everything was broken. Designing a class with a single action is a very bad thing. It had a rippling effect through the app where this class was being used. Luckly for me though, I had a handy-dandy search/replace function built into my trusty IDE so I was back up and running in no time. :D

So listen up kids... don't design your classes to do single functions. dlite22 summed it up pretty well with his car class comparason. Actually, all you guys are great and are truly helpful. Thanks!

[fjm]

And, BTW: what's all the fuss about using getters and setters? I have read a lot of things about how a "true" OO system really isn't OO if you use setters and getters. I'm sure you guys know what they are talking about but its a bit over my head. Can someone please explain?

If needed, I will grab some of these links.

[fjm]

Hey Dan, thanks for this great comparison. Let me ask you a few questions about the methods you chose plus I'd like to respond to a few of your comments.
@dlite922
Yes, agreed... I see it now only because it happened to me where a small change created this huge ripple effect throughout my program. A lesson well learned. The good thing is that I never seem to get tired of refactoring my code. :) I'll now go back and redo these classes the right way.

@dlite922
All of my methods are using verbs like you suggest. Only thing that I'd like a bit of clarification on is that I noticed that you have a method for accelerate(). In my mind, there are other things that need to happen before actually accelerating such as pressing down the accelerator pedal. I know it may seem like I'm splitting hairs here but I'm just trying to understand is all. Would you just combine the pressAccelerator() method in with the accelerate() method? Is there such a thing as taking something out too far? I mean, you could conceiveably say that the accelerator pedal should only move downward a certain amount of distance. Should that be its own method? I know that my examples are corny but I hope you see my point in it.


Thanks,
--Frank

[Dormilich]

@fjm
I think it's about having control. you'll see that if you look at an exception backtrace. if there's a (g|s)etter involved, you know the property had (not) been touched.

and on the other hand, how else would you code read-only properties?

[fjm]

@Dormilich
I agree with you Dormi. Here.... I found an excerpt of why these are *supposedly* so bad:

Supposedly, the getters and setters reveal too much implementation detail because they are typed. What happens if the type changes in the future then everything that calls those getters and setters might have to be updated.