Object Oriented PHP - A beginner looking for feedback on code

PHP
1

I am learning Object Oriented Programming and as a tool to aid this I am re-coding my custom CMS, which is already working as intended and was originally developed using standard, procedual, PHP programming.

I have only just started putting “pen to paper” after several days of reading and watching videos about OOP. However I am somewhat confused still at the whole prospect. While the actual concept is fine and I can follow tutorials and examples when it actually comes to writing my own code I get lost very quickly!

Here is my “first” class for my CMS re-code, please provide some comments (constructive please!) and thoughts on this.

Can you see a bad habbit forming already?
Have I missed somthing obvious?
Is there any good elements to this?

<?php
class CMSCore {

    var $client_user_name;
    var $cms_version;
    var $client;
    var $options;

    function __construct($clientUserName,$cmsVersion,$sqli)
    {

        $this->client_user_name = $clientUserName;
        $this->cms_version = $cmsVersion;

    }

    function get_client_settings()
    {
        global $sqli;

        $mysqli_query = 'SELECT * FROM `cmsSiteSettings` WHERE `ss_userName`="' . $this->client_user_name . '" LIMIT 1';
        $query_result = $sqli->query($mysqli_query);

        if($query_result->num_rows == '1')
        {
            if($r = $query_result->fetch_assoc())
            {
                $this->client['id']            = $r['ss_id'];
                $this->client['options_id']    = $r['so_id'];
                $this->client['domain_name']   = $r['ss_domainName'];
                $this->client['admin_email']   = $r['ss_adminEmail'];
                $this->client['public_email']  = $r['ss_publicEmail'];

                return $this->client;
            }
            else
            {
                return 'Returned empty array: ' . $this->client;
            }

            $query_result->close();

        }
        else
        {
            return $sqli->error;
        }

    }

    function get_client_options()
    {
        global $sqli;

        $mysqli_query = 'SELECT * FROM `cmsSiteOptions` WHERE `so_id`="' . $this->client_options_id . '" LIMIT 1';
        $query_result = $sqli->query($mysqli_query);

        if($query_result->num_rows == '1')
        {
            if($r = $query_result->fetch_row())
            {
                $this->options                       = Array();
                $this->options['google_analytics']   = $r['so_googleAnalytics'];
                $this->options['site_name']          = $r['so_siteName'];

                return $this->options;
            }
            else
            {
                return 'Returned empty array: ' . $this->options;
            }

            $query_result->close();

        }
        else
        {
            return $sqli->error;
        }

    }

}
?>

Current usage:

$cmsCore = new CMSCore(CLIENT_USER_NAME,CLIENT_CMS_VERSION,$sqli);
$coreArray = $cmsCore->get_client_settings();

echo $coreArray['domain_name'];
// Produces the expected field value for the database however something feels wrong about how I have done this...

Thank you for your time.

Nutty…

2

A couple of tips below:

var $client_user_name; 
var $cms_version;
var $client;
var $options;

Why using “var”? You should use “public”, “protected”, “private” instead, anyway, if it is not some “special class” then attributes should always be “protected” or “private”.

function get_client_settings()

It is good habbit to define methods with “public”, “protected” or “private”. Methods are implicitly public, but even then it is good to use “public” keyword.

global $sqli;

There is usually no need to use globals, get rid of it.


function get_client_options()
{
	global $sqli;
        
    $mysqli_query = 'SELECT * FROM `cmsSiteOptions` WHERE `so_id`="' . $this->client_options_id . '" LIMIT 1';
    $query_result = $sqli->query($mysqli_query);
        
    if($query_result->num_rows == '1')
    {
    	if($r = $query_result->fetch_row())
        {
        	$this->options                       = Array();
            $this->options['google_analytics']   = $r['so_googleAnalytics'];
            $this->options['site_name']          = $r['so_siteName'];    
                
            return $this->options;
        }
        else
        {
        	return 'Returned empty array: ' . $this->options;
        }
            
        $query_result->close();
            
    } 
    else
    {
    	return $sqli->error;
    }                            
}

Code is more readable if you use just one “return statement” in your methods, so it is easy to read where is “exit point” of method.


if($r = $query_result->fetch_assoc())
{
	$this->client['id']            = $r['ss_id'];
    $this->client['options_id']    = $r['so_id'];
    $this->client['domain_name']   = $r['ss_domainName'];
    $this->client['admin_email']   = $r['ss_adminEmail'];
    $this->client['public_email']  = $r['ss_publicEmail'];  
                
    return $this->client;    
}
else
{
	return 'Returned empty array: ' . $this->client;
}
  • You should put the shorter block first, it makes code more readable.
  • If there is just one line after “if” or after “else” you can write the line without “{}”
  • you shouldn’t use if($r = $query_result->fetch_assoc()) { … }, I think it is called “assignment in condition” in English, it is source of bugs, your code should look like this:

$r = $query_result->fetch_assoc();

if(!$r)
	$return = 'Returned empty array: ' . $this->client;
else 
{
    $this->client['id']            = $r['ss_id'];
    $this->client['options_id']    = $r['so_id'];
    $this->client['domain_name']   = $r['ss_domainName'];
    $this->client['admin_email']   = $r['ss_adminEmail'];
    $this->client['public_email']  = $r['ss_publicEmail'];  
                
    $return =  $this->client;    
}

return $return;
3

@NuttySkunk

Hi, great that you are learning OOP and picking a project that you are already familiar with how you want it to work.

Coming from procedural background it is common to still code classes procedural like. I notice this about your classes. Some of the tenants of OOP are to encapsulate and and promote reuse with your classes; therefore reducing the amount of code needed; you will need to continue to refactor your application with these things in mind.

OOP can at times be this way, often times tutorials don’t show the best way, they show ‘a way’. I would recommend that you read the articles on MartinFowler.com. Most of his examples are in JAVA but because like PHP it is a C derivative language it is quite easy to follow and understand from a PHP perspective.

When designing objects it is best isolate how properties and methods can be accessed. To this end you want to declare what properties and methods are private, protected, or public (You are using PHP5.X, the ‘var property’ syntax is not used in PHP5+). All of your properties here can be ‘protected’. I have also added an ‘sqli’ protected property (more on why later):


class CMSCore { 
  protected $client_user_name; 
  protected $cms_version;
  protected $client;
  protected $options;
  protected $sqli;

Then your constructor will be:


function __construct($clientUserName,$sqli, $cmsVersion = null){  
  $this->client_user_name = $clientUserName;
  if($cmsVersion){    $this->cms_version = $cmsVersion; 
  }
  $this->client = null;
  $this->options = array();
  $this->sqli = $sqli;}

You should never use global variables in OOP objects. The Singleton and Registry patterns are global type pattern for OOP but have to be used very carefully and knowledgeably. In this case, your sqli protected property is available to all of your object methods, so the global is not needed. Now in your methods refer to your sqli object like:


 $this->sqli;

You slightly revised Class looks like this now:


<?php

class CMSCore {      
  protected $client_user_name;   
  protected $cms_version;  
  protected $client;  
  protected $options;  
  protected $sqli;  
   
  function __construct($clientUserName,$sqli, $cmsVersion = null){
    $this->client_user_name = $clientUserName;
    if($cmsVersion){
      $this->cms_version = $cmsVersion;     
    }    
    $this->client = null;    
    $this->options = array();    
    $this->sqli = $sqli;  
  }    

  function get_client_settings(){
    $mysqli_query = 'SELECT * FROM `cmsSiteSettings` WHERE `ss_userName`="' . $this->client_user_name . '" LIMIT 1';     
    $query_result = $this->sqli->query($mysqli_query);      
    if($query_result->num_rows == '1'){        
      if($r = $query_result->fetch_assoc()){
        $this->client['id'] = $r['ss_id'];
        $this->client['options_id'] = $r['so_id'];
        $this->client['domain_name']  = $r['ss_domainName'];
        $this->client['admin_email']   = $r['ss_adminEmail']; 
        $this->client['public_email']  = $r['ss_publicEmail'];            
        return $this->client;
      } else {
         return 'Returned empty array: ' . $this->client;        
      }        
      $query_result->close(); 
    } else {
       return $this->sqli->error;      
    }  
  }      

function get_client_options(){
  $mysqli_query = 'SELECT * FROM `cmsSiteOptions` WHERE `so_id`="' . $this->client_options_id . '" LIMIT 1';    
  $query_result = $this->sqli->query($mysqli_query);
  if($query_result->num_rows == '1'){
      if($r = $query_result->fetch_row()){
        $this->options['google_analytics'] = $r['so_googleAnalytics'];
        $this->options['site_name'] = $r['so_siteName'];           
        return $this->options;      
     } else {      
       return 'Returned empty array: ' . $this->options;      
     } 
     $query_result->close();    
  } else {      
     return $this->sqli->error;    
  }
 }
}?>

Maybe also consider picking up php|architect’s Guide to PHP Design Patterns written by Jason E. Sweat. He is a infrequent visitor here at Sitepoint but for each pattern the book poses a problem, then solution and full code on the patterns in action. (ISBN 0-9735898-2-5)

Steve

4

Wow, what can I say? Outstanding replies, thank you.

Your comments and advice are appreciated. Thank you for taking the time to write those replies.

5

I think you are making a classic mistake, you are trying to turn your whole cms into a class.

You are starting to work from the top down, when you should be starting from the bottom up (or using a known PHP framework).

The giveaway is that you are writing bare sql in your class, whereas most classes deal with an abstraction, they tell another class to do something.

They delegate responsibility.

Classes are normally very small, though they may belong to a family of classes or collaborate with fellow classes.

I only know this because I remember trying to do the same as you have done. You need to find some good books, I can recomment Jason’s book though it is looking dated. Anything by Fowler is heavenly, but it might be too much for a new oop-er to swallow.

Go to a good bookshop and browse for an afternoon 'till you find a book which appeals to you.

This one did if for me, but I can well understand how many would find it condescending and trite.

Be prepared to buy more than one book, and please, do not let my reply put you off – you are on the right line for sure.

6

Thank you for your reply.

I understand what you are saying and it makes sence. Looking over the class I am writing at the moment, while it does look nice, organised and should be simple to update, it is just procedual code in the form of classes and methods. However I believe I am working towards a more “object oriented” section now, all be it in a mess of non OOP code…

Here is my “pages” class so far. Not tested or finished yet (obviously).

I am going even further in the wrong direction here right? and living up to the notion of putting my whole cms in a class…

<?php
class CMSPage {
    
    protected $page_name;
    protected $sub_page_name;
    protected $page_id;
    protected $curpage;
    protected $sqli;
    
    function __contruct($pageName,$subPageName,$sqli) {
        
        $this->page_name = $this->sqli->real_escape_string($pageName);
        $this->sub_page_name = $this->sqli->real_escape_string($subPageName);
        $this->sqli = $sqli;
        
        $this->page = (isset($this->page_name)) ? $this->page_name:'home';
        $this->subpage = (isset($this->sub_page_name)) ? $this->sub_page_name : NULL;
        $arr = $this->pages_array();
        
        if(!$arr) {
        
            $this->curpage = NULL;
            
        }else{
        
            if(in_array($this->page,$arr['page']) && in_array($this->subpage,$arr['subpage'])) {
                
                $mysqli_query = 'SELECT `p_id` FROM `cmsPages` WHERE `p_pageUrl`="' . $this->subpage . '" AND `p_pageParent`>="1" AND `p_pageStatus`="1" LIMIT 1';     
                $query_result = $this->sqli->query($mysqli_query); 
                     
                if($query_result->num_rows == '1'){    
                    
                    $r = $query_result->fetch_assoc();
                    
                    $this->page_id = $r['p_id'];
                    
                }
                
                $query_result->close();        
                
            }elseif(in_array($this->page,$arr['page'])) {
                
                $mysqli_query = 'SELECT `p_id` FROM `cmsPages` WHERE `p_pageUrl`="' . $this->page . '" AND `p_pageParent`="0" AND `p_pageStatus`="1" LIMIT 1';     
                $query_result = $this->sqli->query($mysqli_query); 
                     
                if($query_result->num_rows == '1'){    
                    
                    $r = $query_result->fetch_assoc();
                    
                    $this->page_id = $r['p_id'];
                    
                }
                
                $query_result->close();   
                
            }
        
        }
    }
    
    function pages_array() {
        
        $mysqli_query = 'SELECT `p_pageUrl` FROM `cmsPages` WHERE `p_pageStatus`="1" AND `p_pageParent`="0" AND `p_pageStatus`="1"';     
        $query_result = $this->sqli->query($mysqli_query); 
             
        if($query_result->num_rows >= '1'){    
            
            $r = $query_result->fetch_assoc();
            $i = 0;
            
            while($r) {
                $this->pages_array['page'][$i] = $r['p_pageUrl'];
                $i++;
            }
            
        }
        
        $query_result->close();
        
        $mysqli_query = 'SELECT `p_pageUrl` FROM `cmsPages` WHERE `p_pageStatus`="1" AND `p_pageParent`>="1" AND `p_pageStatus`="1"';     
        $query_result = $this->sqli->query($mysqli_query); 
             
        if($query_result->num_rows >= '1'){    
            
            $r = $query_result->fetch_assoc();
            $i = 0;
            
            while($r) {
                $this->pages_array['subpage'][$i] = $r['p_pageUrl'];
                $i++;
            }
            
        }
        
        $query_result->close();
        
        return $this->pages_array;
        
    }
    
    function get_page_data() {
        
        if($this->curpage == NULL) {
            
            $return = 'Page not found, error 404.';    
            
        }else{
            
            $mysqli_query = 'SELECT `pt_id`,`pd_id`,`pc_id`,`p_pageUrl` FROM `cmsPages` WHERE `p_pageStatus`="1" AND `p_id`="' . $this->page_id . '" LIMIT 1';     
            $query_result = $this->sqli->query($mysqli_query); 
                 
            if($query_result->num_rows == '1'){    
                
                $r = $query_result->fetch_assoc();
                $this->page['pt_id'] = $r['pt_id']; 
                $this->page['pd_id'] = $r['pd_id']; 
                $this->page['pc_id'] = $r['pc_id']; 
                $this->page['p_pageUrl'] = $r['p_pageUrl']; 
                $return = $this->page; 
                
            }
            
            $query_result->close();    
            
        }
        
        return $return;  
        
    }
    
    function get_page_details() {
        
        
        
    }
    
    function get_page_content() {
        
        
        
    }
    
    function build_page() {
        
        $this->pdata_arr = $this->get_page_data();
        $this->pdetails_arr = $this->get_page_details();
        $this->pcontent = $this->get_page_content();
        
    }
    
}

class PhunkCMSTemplate extends PhunkCMSPage {
    
    private $template_id;
    private $template_file;
    
    function __contruct($templateId) {
        
        $this->template_id = $templateId;
           
    }
    
    function process_template_file($templateFile) {
        
        $this->template_file = $templateFile;    
        
    }
    
    function get_page_details() {
        
        
        
    }
    
    function get_page_content() {
        
        
        
    }
    
}
?>
7

Here’s another way adding a little separation, and focus, to your objects. You introduce a mapper, which acts like a gateway between your database and the object you need. In this case, CMSPageMapper provides CMSPage object from you. This allows any other object to be completely unaware of how they are created, built or saved.


<?php
class CMSDatabase
{
	public function query($query){
		
	}
}


class CMSPage
{
	protected
		$id,
		$title,
		$content;
		
	public function getId(){
		return $this->id;
	}
	
	public function setId($id){
		$this->id = $id;
		return $this;
	}
	
	public function getTitle(){
		return $this->title;
	}
	
	public function setTitle($title){
		$this->title = $title;
		return $this;
	}
	
	public function getContent(){
		return $this->content;
	}
	
	public function setContent($content){
		$this->content = $content;
		return $this;
	}
}


class CMSPageMapper
{
	protected
		$database;
		
	public function __construct(CMSDatabase $database){
		$this->database = $database;
	}
	
	public function getAll(){
		$pages = array();
		foreach($this->database->query('SELECT id, title, content FROM page;') as $recordset){
			$pages[] = $this->fromRecordSet($recordset);
		}
		return $pages;
	}
	
	public function findById($id){
		$recordset = $this->database->query('SELECT id,title, content FROM page WHERE id = ? LIMIT 1', array($id));
		return $this->fromRecordSet($recordset);
	}
	
	protected function fromRecordSet($recordset){
		$page = new CMSPage;
		$page->setId($recordset['id']);
		$page->setTitle($recordset['title']);
		$page->setContent($recordset['content']);
		return $page;
	}
}
8

Thank you ^^

That is a fascinating read! I didn’t realise classes were supposed to be short/concise and it now gives more meaning to some of the examples I have seen.