Understanding OOP How Does My Script Look?

[Korferer]

Hi.

 

I haven't done a lot of OOP. Hardly any really. I've had a go at writing a very small class that outputs a greeting depending on what time of day it is. What do the OOP experts here make of it? What do you like, what do you hate? Is there anything I could do to make it more useful?

 

Here is the code;

class Greeting {

  // the __construct didn't do what I originally wanted. Denfine the hour outside the method (I don't know why this is a good/bad idea)
  /*
  public function __construct()
  {
    $hour_of_day = date('G');
  }
  */
  
  public function callGreetingPhrase()
  {
      return $this->getGreetingPhrase();
  }

  // the setter doesn't seem to have a purpose here. I tried using it so that I could pass in a value of my choosing (for testing purposes) can't get it to work though
  /*
  public function setGreetingPhrase($value)
  {
      $this->hour_of_day = $value;
  }
  */
  
  private function getGreetingPhrase()
  {  
	  $hour_of_day = date('G');
	  
	  if($hour_of_day < 12 ) { // if it's before 12pm
		
		$greeting_phrase = "good morning";
		
	  } elseif($hour_of_day >= 12 && $hour_of_day < 18 ) { // if it's after 12pm but before 6pm
		  
		$greeting_phrase = "good afternoon";
		  
	  } else { // what is left over - after 6pm until midnight
		
		$greeting_phrase = "good evening";
		
	  }

	  return $greeting_phrase;
  }
  
}


$greeting = new Greeting;

echo $greeting->callGreetingPhrase();

It annoyed me that I could 't figure out how to use the __constructor here to store the hour. But should that have bothered me?

 

Can someone maybe explain a bit about the setter that I tried to use setGreetingPhrase. I only put it in because I've seen other Classes with one. Could I use a setter method here for anything useful?

 

 

Any feedback appreciated!

 

[KevinM1]

Wow, there's a lot here that pretty much misses the point entirely.  I'm not trying to be rude, but there's a definite lack of understanding as to why certain class structures are used.

 

First and foremost, something like this would likely never be put into a class, at least, not by itself.  It's too specific, too small to justify the overhead of writing a class and instantiating an object.  OOP is primarily about dealing in abstractions.  A more robust greeting system could warrant a class, but not as it stands now.

 

Putting that aside, the class is overwrought.  Why would you want to store the current hour when you can simply get it from PHP whenever you wanted?  Why do you have a public getter, then a private getter that does the work for you?  There's no need to go through so many hoops.

 

Your constructor didn't work because you never created the property you're trying to assign the value to.  The $this keyword means 'current' object, but you never declare a $hour_of_day property for the object to use. You need to declare something before you can attempt to use it, like:

 

class Example
{    
    private $x; // <-- notice that we created a property here     

    public function __construct($blah)    
    {        
        $this->x = $blah; // <-- the value of $blah is now contained in this object's $x property    
    }
}

 

So, all that said, your class can be reduced to:

 

class Greeting
{    
    public function getGreeting()    
    {        
        $hour = date('G');
    
        if ($hour < 12 ) { // if it's before 12pm
            $greeting = "good morning";
        } elseif ($hour >= 12 && $hour < 18 ) { // if it's after 12pm but before 6pm
            $greeting = "good afternoon";    
        } else { // what is left over - after 6pm until midnight     
            $greeting = "good evening";    
        }

        return $greeting;    
    }   
}

Which isn't really useful. Classes are not intended to just be groups of functions. Edited June 19, 2014 by KevinM1

[maxxd]

OK - it's actually not a bad start at all for a beginner.

 

Couple things (please keep in mind that this is how I code and there are a million other opinions and ways of working)

First, your constructor method actually was doing what it was supposed to do, it just wasn't storing the result anywhere or returning anything so it looked like it wasn't doing anything. Notice in the code below that there are two property declarations before the constructor - these set up object-scope variables (also called properties). The scoping here means that the methods in the Greeting class can access the values in the properties, but, because they're declared private, an external class will *not* be able to access them directly. The same visibility caveat applies to the functions (called methods) in the class itself - an external class won't be able to call setGreetingPhrase() without an error.

 

Now, I have a tendency to break my functionality into the smallest units I can think of when I create my methods. Some would say they're too small, but I like to know exactly what I'm looking at when I have to debug a method. So, in this example, your constructor explicitly calls setTime() to set the hour of the day in the local property, then explicitly calls setGreetingPhrase() which uses the value in hour_of_day to set the string value you eventually want to return.

 

Finally, you've got the getGreetingPhrase() method which acts as the public access point in order to return the string built by the constructor.

 

Obviously, there's a ton of different things you can do here - it's just a quick and dirty example. For instance, make setGreetingPhrase() and setTime() public and other objects can modify the eventual output, you can do value checking, etc.

class Greeting{

	/**
	 *	The hour
	 *	@var	int
	 */
	 private	$hour_of_day = 0;
	 /**
	  *	The actual greeting phrase
	  *	@var	string
	  */
	  private	$greeting_phrase;

	/**
	 *	Class constructor.
	 *	@param	string	$dateString	Date string
	 */
	public function __construct($dateString=null){
		if(!is_null($dateString)){
			$this->setTime($dateString);
			$this->setGreetingPhrase();
		}
	}

	/**
	 *	Sets the time of day.
	 *	@param	string	$value	Valid date string to parse and store
	 *	@return	void|string		Error code and message on DateTime exception
	 */
	private function setTime($value){
		try{
			$dt = new DateTime($value);
		}catch(Exception $e){
			print("<p>Error: {$e->getCode()} :: {$e->getMessage()}</p>");
			die();
		}
		$this->hour_of_day = $dt->format('G');
	}
  
	/**
	 *	Sets the actual phrase to return as a greeting.
	 *	@return	void
	 */
	private function setGreetingPhrase(){
	   if($this->hour_of_day < 12){
		   $this->greeting_phrase = 'Good morning!';
	   }elseif($this->hour_of_day >= 12 && $this->hour_of_day < 18){
		   $this->greeting_phrase = 'Good afternoon!';
	   }else{
		   $this->greeting_phrase = 'Good evening!';
	   }
	}
	
	/**
	 *	Returns the greeting phrase if set up.
	 *	@return	string|null
	 */
	public function getGreetingPhrase(){
		if(!empty($this->greeting_phrase)){
			return $this->greeting_phrase;
		}
		return null;
	}
}

$greeting = new Greeting('2014-06-19 12:45:00');
//$greeting = new Greeting('12:45pm today');
echo $greeting->getGreetingPhrase();

So, that's my two cents - hope it helps!

Edited June 19, 2014 by maxxd

[Korferer]

I get that it's a bit simple. I don't plan to actually use this for anything. I am just trying to practice OOP at the moment.

I'm starting to get the point though that Classes are for BIG things. Like a Database class, or a User class, or a Validation class. And not small things like my Greeting class above.
 

Putting that aside, the class is overwrought. Why would you want to store the current hour when you can simply get it from PHP whenever you wanted? Why do you have a public getter, then a private getter that does the work for you? There's no need to go through so many hoops.

 
I wanted a way to pass a set hour to the getGreetingPhrase, in a variable. But I didn't want to adjust the code in the method. That would give me the option of not always having to use the current hour.
 

Your constructor didn't work because you never created the property you're trying to assign the value to.  The $this keyword means 'current' object, but you never declare a $hour_of_day property for the object to use.

 

Ok, that makes sense.

[KevinM1]

I wanted a way to pass a set hour to the getGreetingPhrase, in a variable. But I didn't want to adjust the code in the method. That would give me the option of not always having to use the current hour.

You can just pass the value through the method's argument list. There's nothing saying that getters can't receive an argument. Example:

 

public function getGreetingPhrase($hour = null) // we set the default to null
{
    $hour_of_day = ($hour) ? $hour : date('G'); // if something IS sent through as a parameter, use it, otherwise, use the current hour

    // do stuff with $hour_of_day
}

The above isn't really OOP specific, but it should illustrate a cleaner way to do what you want.

 

What resources are you using to learn OOP? I have a couple of books I generally recommend:

 

PHP Objects, Patterns, and Practice

 

Design Patterns: Elements of Reusable Object Oriented Software

 

They should get you on the right track. Start with the PHP-specific one first.