Page 1 of 1

OOP Best Practices - Queries

Posted: Tue Jul 29, 2008 12:25 pm
by psurrena
I have class called generalClass which runs queries, connects to the db and so on. I have another class for a specific section of a website, in this case peopleClass. Right now peopleClass only has queries but may include functions later.
Is it a good or bad idea to keep my queries for this section in the class? How is the overall approach?

Code: Select all

##General Class
class generalClass{
    protected $dbc;
    protected $errorMsg="";
        
    public function __construct(){
        if (stristr($_SERVER['HTTP_HOST'], 'local')||(substr($_SERVER['HTTP_HOST'],0,7)=='192.168')){
            $local=TRUE;
        }else{
            $local=FALSE;
        }
   
        if($local){
            $this->dbc=mysqli_connect('localhost','root','','db') or die (mysql_error());
        }else{
            $this->dbc=mysqli_connect('remote','abc','abc','db') or die (mysql_error());
        }
    }
   
    public function singleResult(){
        $result=mysqli_query($this->dbc,$this->query) or die (mysql_error());
        if($result){
            $row=mysqli_fetch_assoc($result);
        }else{
            $this->errorMsg="Sorry, the result was empty.";
        }       
        return $row;
    }
}
 
### more functions...
 

Code: Select all

##People Class
require_once('./library/class/general.class.php');
 
class peopleClass extends generalClass{
    protected $query;
 
    public function peopleQuery(){
    $this->title=($_GET['title']);
 
    $this->query="SELECT people_fname, people_lname WHERE people_title='this->$title' ORDER BY people_lname ASC";
    
    return $this->query;
}

Code: Select all

##Sample Page
 
#People
require_once './library/class/people.class.php';
 
//Template
$template="default.template.php";
 
//Content
$people=new peopleClass();
$people->peopleQuery();
$item=$people->singleResult();
 
$tags['content'].=$item['people_fname'];
 

Re: OOP Best Practices - Queries

Posted: Tue Jul 29, 2008 4:37 pm
by Eran
What you described here is generally referred to as Models (classes that abstract domain functionality). In web application, most models are indeed database based, and usually extend a base class that defines an API to access the database - as you did with peopleClass extending generalClass.
So definitely, the queries belong in the peopleClass, as they describe a part of the 'people' domain (how to fetch its data).

I have a couple of other suggestion for you though -
- Don't initiate the database connection in the contructor, rather do it on demand (only when a query needs it). A peopleClass object will not always send out a query - so its wasteful to connect to the database every time.
- The generalClass might be better designed as a composed class - acting as a mediator between peopleClass and a separate database class. This way you are not tying yourself to the database specific implementation, but rather to an API (common API for database access is having methods like query(), insert(), update(), delete() etc.)

Re: OOP Best Practices - Queries

Posted: Tue Jul 29, 2008 7:09 pm
by Christopher
I think your code looks really screwy. First there are names like "generalClass" which just seems horribly nondiscriptive in every way I can think of. I would reather see something like this:

Code: Select all

class People extends TableDataGateway {
 
    public function findPersonByTitle($title){
        $title = $this->escape($title);
        $sql = "SELECT people_fname, people_lname WHERE people_title='$title' ORDER BY people_lname ASC";
    
        return $this->singleResult($sql);
    }
}

Re: OOP Best Practices - Queries

Posted: Wed Jul 30, 2008 10:40 am
by psurrena
arborint, thanks for cleaning it up a bit, I'm still getting a feel for this.

pytrin, I realize a class doesn't "need" a constructor but should it have one? In theory, is there always something to be done? Also, are you saying the composed class would have insert, update...or is that the db class?

Re: OOP Best Practices - Queries

Posted: Wed Jul 30, 2008 11:54 am
by Eran
A class should only have a constructor if there is something to be done when its instanced. In this case, creating the database connection every time the class is instanced is wasteful, since you won't necessarily be connecting to the database.

What I was suggesting with the composed class is that it should redirect database calls to a more generic database class. Your generalClass (which I agree with arborint is not the best name) is basically a table class.

Maybe something like:

Code: Select all

 
class DatabaseTable {
    /**
     * Database connection
     * @var Database instance
     */
    protected $_db = null;
    
    /**
     * Table name
     * @var string
     */
    protected $_name;
    
    protected function _connect() {
        if(is_null($this -> _db)) {
            $this -> _db = new Database('Mysql');
        }
        return $this -> _db;
    }
    
    public function fetchAll($sql) {
        return $this -> _connect() -> fetch($sql);
    }
    
    public function insert(array $data) {
        return $this -> _connect() -> insert($data,$this -> _name);
    }
}
 
class People extends DatabaseTable {
    protected $_name = 'people';
    
    public function getPeopleByTitle($title) {
        $sql = "SELECT people_fname, people_lname FROM " 
            . $this -> _name 
            . " WHERE people_title='" 
            . $this -> _connect() -> escape($title)
            . "' ORDER BY people_lname ASC";
        return $this -> fetchAll($sql);
    }
}
 
$people = new People();
$items = $people -> getPeopleByTitle($_GET['title']);
 

Re: OOP Best Practices - Queries

Posted: Wed Jul 30, 2008 1:58 pm
by psurrena
This is a bit more abstract than I'm used to so please bear with me.

Does this code:

Code: Select all

$this -> _db = new Database('Mysql');
assume something like:

Code: Select all

class Database{
    public function __construct($database){
        $dbc=mysqli_connect('localhost','root','','$this->database') or die (mysql_error());
        return $dbc;
    }
}
??

Re: OOP Best Practices - Queries

Posted: Wed Jul 30, 2008 6:20 pm
by Eran
Yet, but here the database connection is lazy-loaded. The _connect() delegates database connection to a generic database class on demand.

Code: Select all

 
 protected function _connect() {
         if(is_null($this -> _db)) { //Has a database connection class been created yet?
             $this -> _db = new Database('Mysql'); //Create database object (handles database connections)
         }
         return $this -> _db;
     }
 
and other methods use the database object to query the database:

Code: Select all

 
  public function fetchAll($sql) {
         return $this -> _connect() -> fetch($sql);
     }
    
     public function insert(array $data) {
         return $this -> _connect() -> insert($data,$this -> _name);
     }
 
You could add update() and delete() methods of course;