Is this correct object oriented programming in php? [closed]

Question

Could this be classified as correct OOP programming?

class Greeting {      public $greet = array('Hi','Hello', 'Howzit', 'Ola', 'Whats up');      function __construct($name) {         $this->name = $name;         shuffle($this->greet);     } }  $hi = new Greeting('INSERTNAMEHERE'); /*NAME OF PERSON GOES HERE*/ echo $hi->greet[1] .' '. $hi->name; 

Answer 1

For the sake of Keeping it Simple, I'd say it's okay. It's not too proper OOP though, nor is it particularly easy to understand code. Having working code is better than having no code at all though.

Let's go through your code:

1 class Greeting { 2 3    public $greet = array('Hi','Hello', 'Howzit', 'Ola', 'Whats up'); 4 5    function __construct($name) { 6        $this->name = $name; 7        shuffle($this->greet); 8    } 9 } 

Line 1: says this class represents the concept of a Greeting. What is a Greeting? I'd say something like "Hello John" or "Hi John" or "Howdy John" is a greeting. And in fact, you seem to agree because in …

Line 3: … you do have a list of similar greetings, just without a name. But that property warrants the question why your class is named Greeting, when it really encapsulates multiple greetings already. Shouldn't the class be called Greetings (mind the plural) then?

Line 3: Naming the property "greet" wasn't a too good idea either. It's a property, so dont give it the name of a verb. Verbs are for methods.

Line 3: Although there is people who will tell you different, making the property public is rarely a good idea. Properties are about internal state and that state should not be accessible directly, but only through methods.

Line 5: Your constructor then tells me a Greeting has to have a name. If I wouldn't be looking at the source code already, I'd falsely assume this to be the name of the Greeting. But you really mean a Person's name. The argument should reflect that and be named something more indicative, like $greetedPersonsName.

Line 6: Assigning properties on the fly is a boo boo. If I look at the class definition, I want to see the properties immediately. Discovering them inside some method makes the code hard to understand. This will also not getting picked up when generating API docs. Avoid it.

Line 7: The shuffle is another unexpected thing. It's a non-obvious side-effect. If I was to instantiate a new Greeting, I'd expect the greetings to appear in the order they are listed. It's against the Principle of Least Astonishement to shuffle them in the ctor. The shuffling should happen from a public method that does nothing but shuffling, e.g.

public function shuffleGreetings() {     shuffle($this->greetings); } 

Assuming the idea of the class was really to be a single Greeting that just initializes itself with one of the default possible values, we can also add a Getter like this:

public function getGreeting() {     return $this->_greetings[0] . ' ' . $this->name; } 

This is better than doing

echo $hi->greet[1] .' '. $hi->name; 

because it hides the implementation details. I don't need to know that the Greeting object has an array of possible greetings. I just want to get the Greeting combined with the set name. It's still far from perfect though, because you still would use it like

$hi = new Greeting('John'); // A Greeting named John? Why $hi then? $hi->shuffleGreetings();    // Shuffling Greetings in a Greeting? echo $hi->getGreeting();    // Why is it "Hello John" all of a sudden? 

As you can see, the API is still pretty much full of WTF. A developer will still have to look at your source code to understand what's going on.

More on Side-Effects

While it may be tempting to put the shuffle into getGreeting you should not do so. A method should return the same thing for the same input. When I call getGreeting twice in a row, I can expect it to return the same result. You would expect 1+1 to return 2 always, so make sure your methods do too.

Likewise, if you want to have a single method to return a random item from the greetings property, dont shuffle the greetings array. If you would use the shuffle method instead, you would also change the greetings property. That ripples to any function reading from the property, e.g. when you do

public function getRandomGreeting() {     $this->shuffleGreetings();     return $this->getGreeting(); } 

a developer will experience something like this:

$hi = new Greeting('John'); $hi->shuffleGreetings(); echo $hi->getGreeting();       // for example "Hello John" echo $hi->getRandomGreeting(); // for example "Hi John" echo $hi->getGreeting();       // for example "Howdy John" <-- WTF!! 

Use an implementation that doesnt change the property, e.g.

public function getRandomGreeting() {     $randomKey = array_rand($this->greetings);     return $this->greetings[$randomKey] . ' ' . $this->name; } 

That is free of side-effects:

$hi = new Greeting('John'); $hi->shuffleGreetings(); echo $hi->getGreeting();       // for example "Hello John" echo $hi->getRandomGreeting(); // for example "Hi John"    echo $hi->getGreeting();       // still "Hello John". Expected! 

The API is still far from pretty though. If I think about the properties of a Greeting, I just dont think "Person's name". Just saying "Hi" oder "Hello" is still a valid greeting. It doesn't require a name. How about

public function greetPerson($personName) {     return $this->getGreeting() . ' ' . $personName; } 

and then we can do

$hi = new Greeting; $hi->shuffleGreetings(); echo $hi->greetPerson('John'); 

And to finally hide that our Greeting contains an array that needs to be shuffled, let's move our shuffleGreetings method back to the ctor and rename the class to RandomGreeting.

class RandomGreeting …      public function __construct()     {         $this->shuffleGreetings();     } 

This might seem counterintuitive at first, because I told you not to shuffle in the ctor. But with the class renamed to RandomGreeting, it is much more to be expected that there is something happening behind the scenes. We just don't need to know what exactly. To reflect that, we should also make the shuffleGreetings method protected now. We just hide it from the public interface completely. Now our code reads like this:

$hi = new RandomGreeting; echo $hi->greetPerson('John'); // ex "Howdy John" 

This doesn't give you any WTFs because your code clearly communicates you'll get a random greeting. The classname clearly communicates what it does.

This is a little bet better now and we could end here, but one could still argue that a Greeting should not be able to greet on it's own, but is rather something that is done by a Person instead.

Improving it

Our findings should lead us to the conclusion that a Greeting should rather be a dumb Type encapsulating the Greeting message and nothing else. All it should do is return that message. Since the Greeting doesn't have any real behavior next to storing the message string, the easiest would be to create a Value Object, e.g. an object that is equal to another object by value of a property:

class Greeting {     protected $value;      public function __construct($value)     {         $this->value = $value;     }      public function getValue()     {         return $this->value;     } } 

The other way to do it would be to make the various available greetings into separate types. There is very little gain to do that when your objects don't have behavior. You only need that if you want to take advantage of Polymorphism. However, having concrete subtypes does draw a few additional things to consider later on, so let's just assume we need it.

The proper way to do that in OOP would be to define an interface

interface Greeting {     public function getGreeting(); } 

that defines a class that wants to behave like a Greeting has to have a getGreeting method. Since interfaces dont implement any logic, we also add an abstract type that contains the greeting property and the logic to return it:

abstract class GreetingType implements Greeting {     protected $greeting;     public function getGreeting()     {         return $this->greeting;     } } 

When there is an abstract class, there also needs to be concrete classes derived from the abstract class. So let's use Inheritance to define our concrete Greeting types:

class HiGreeting extends GreetingType {     protected $greeting = 'Hi'; }  class HelloGreeting extends GreetingType {     protected $greeting = 'Hello'; }  class HowdyGreeting extends GreetingType {     protected $greeting = 'Howdy'; } 

It's not absolutely necessary to have an Interface and an Abstract implementing the interface. We could have made our concrete Greetings not extend from the GreetingType. But if we had just reimplemented the getGreeting method on all the various Greeting classes, we would be duplicating code and were much more prone to introducing errors and should we ever need to change something, we'd have to touch all these classes. With the GreetingType it's all centralized.

The other way round is true as well. You dont necessarily need an interface. We could have used just the abstract type. But then we would be limited to the GreetingType whereas with an interface, we could potentially add new Types with much more ease. I admit I can't think of any right now, so it is probably YAGNI. But it's so little to add that we can just as well keep it now.

We will also add a Null Object that returns an empty string. More on that later.

class NullGreeting extends GreetingType {     protected $greeting = ''; } 

Object creation

Because I do not want to litter new classname all over my consuming classes and introduce coupling, I will a use simple Factory instead to capsule object creation:

class GreetingFactory {     public function createGreeting($typeName = NULL)     {         switch(strtolower($typeName)) {             case 'hi':    return new HiGreeting;             case 'howdy': return new HowdyGreeting;             case 'hello': return new HelloGreeting;             default:      return new NullGreeting;         }     } } 

The Factory is one of the few pieces of code where you actually can use a swich/case without having to check if you can Replace Conditional with Polymorphism.

Responsibility

With object creation out of the way, we can finally start to add our Greetings class:

class Greetings {     protected $greetings;     protected $nullGreeting;     public function __construct(NullGreeting $nullGreeting)     {         $this->greetings = new ArrayObject;         $this->nullGreeting = $nullGreeting;     }     public function addGreeting(Greeting $greetingToAdd)     {         $this->greetings->append($greetingToAdd);     }     public function getRandomGreeting()     {         if ($this->hasGreetings()) {             return $this->_getRandomGreeting();         } else {             return $this->nullGreeting;         }     }     public function hasGreetings()     {         return count($this->greetings);     }     protected function _getRandomGreeting()     {         return $this->greetings->offsetGet(             rand(0, $this->greetings->count() - 1)         );     } } 

As you can see, Greetings is really just a wrapper for an ArrayObject. It makes sure that we cannot add anything else but objects implementing the Greeting interface to the collection. And it also allows us to pick a random Greeting from the collection. It also makes sure that you always get a Greeting from the call to getRandomGreeting by returning the NullGreeting. This is awesome, because without that you would have to do

$greeting = $greetings->getRandomGreeting(); if(NULL !== $greeting) {     echo $greeting->getMessage(); } 

in order to avoid a "Fatal Error: Trying to call method on non-object" when the getRandomGreeting method did not return a Greeting object (when there is no Greeting in the Greetings class yet).

The class has no other responsibility than that. If you are unsure if your class is doing too much or has methods that should better be on some other object, look at the methods in that class. Do they work with a property of that class? If not, you should probably move that method.

Getting it done

Now to finally put all that code to use, we add our Person class now. Since we want to make sure we can call a getName method on it, we create an interface before doing so

interface Named {     public function getName(); } 

We could have named that interface IPerson or whatever but it only has one method getName and the most fitting name is Named then, because any class implementing that interface is a named thing, including, but not limited to our Person class:

class Person implements Named {     protected $name;     protected $greeting;     public function __construct($name, Greeting $greeting)     {         $this->name = $name;         $this->greeting = $greeting;     }     public function getName()     {         return $this->name;     }     public function greet(Named $greetable)     {         return trim(sprintf(             '%s %s',             $this->greeting->getGreeting(),             $greetable->getName()         ));     } } 

Our Person has a required name and we demand it to have a Greeting as well. All it can do besides returning it's name is greet another Named thing, likely another person. And that's it.

To put that all together now:

$greetings->addGreeting($greetingsFactory->createGreeting('Hi')); $greetings->addGreeting($greetingsFactory->createGreeting('Howdy')); $greetings->addGreeting($greetingsFactory->createGreeting('Hello')); $john = new Person('John Doe', $greetings->getRandomGreeting()); $jane = new Person('Jane Doe', $greetings->getRandomGreeting());  echo $john->greet($jane),       PHP_EOL,       $jane->greet($john); 

Live Demo on Codepad

Granted that's quite a lot of code for a very simple thing to do. Some will call it overengineered. But you asked for correct OOP and while I am sure there is still room for improvement, it's quite proper and SOLID in my book. And it's easy to maintain now, because the responsibilities are closer to where they should be.