My first real attempt with OOP.

Morning peeps,

I was wondering if people could advise of better ways to do what i've attempted to.

Its just a small class to carry out a select query, as i'm learning i guess i need critism for me to learn from my mistakes so fire away. Ignore how i have displayed the data as thats not what im intrested in, that was just to show it worked.
Beleive me, its nothing fancy!

Class:

Code PHP:

<?php
class select {
  var $query; // will store the query string only.
  var $result; // will store the results after the execute function is called.
 
  function __construct($columns, $table, $clause){
    // Create the query and store in var "query"
    $query = "SELECT ";
    $query .= implode(", ", $columns); // separate the colums.
    $query .= " FROM ".$table;
    $query .= $clause;
    $this->query = $query;
  }
 
  function execute (){
    //execute the query and store the results into the var "results";
    $this->result = mysql_query($this->query);
 
  }
}
?>

view page:

Code PHP:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
<title>OOP in PHP</title>
<?php include('class_lib.php'); ?>
<?php include('conn.php'); ?>
</head>
<body>
<?php
//setup query parameters
$columns = array("username","email"); // select these columns.
$table = "users";// tables to select from
$clause = " WHERE user_id !='1'"; //class query, options.
 
$q_get_usrname_email = new select($columns, $table, $clause); // create a new object for select.
$q_get_usrname_email->execute(); // execute the query.
//output the query.
 
while ($row = mysql_fetch_assoc($q_get_usrname_email->result)){
  echo $row['username']." ".$row['email']."<br>";
 
}
 
?>
 
 </body>
</html>

Thanks for any pointers in advance.

Steve

Why use the php4 style?

I would recommend using 'protected' keyword instead of 'var'
and definetely dont make the $result public. You should either make it protected and have accessor method OR just have your execute method return the result.

that is simple class but what about JOINs UNIOINs and in general for complex queries.

My opinion is that this class is not so usefull. Try to make business entities and capsulate business logix in classes.

I'm with Kuzmanin. I think you're trying to replicate functionality that already exists as a baseline in PHP.

oh i know i know, i wont be using this code this is just me making up more or less, pointless and useless classes just as a challenge for me. I just want to know if my coding techniques are terrible and need improving etc.

I'll work on something else thats more relevant i guess.

Thanks anyways.

Well properties should always be protected or private. Properties should only be accessible to the outside world indirectly through getter and setters. Furthermore, if a method only needs to be accessed from within the scope of the object then you should consider having those be protected or private also. Limiting the methods which are accessible outside the scope of the object leads to a simplified public interface. The simpler the public interface the easier the class is to use and understand months or years down the line.

It's better practice to keep access to variables through methods (functions) instead of direct access.

Maybe you could try something like....

PHP Code:

class Select{ protected $Table, $Columns = array(), $Clauses = array(), $Result = false, $LastQuery = ''; public function __Construct($Table, array $Columns = null, array $Clauses = null){ $this->setTable($Table); $this->addClause('1 = 1'); if($Columns != null){ $this->addColumns($Columns); } if($Clause != null){ $this->setClause($Clause); } } public function setTable($Table){ $this->Table = (string)$Table; } public function addClauses(array $Clause){ $this->Clauses = array_merge($this->Clause, $Clause); } public function addColumn($Clause){ $this->Clauses[] = (string)$Clause; } public function addColumns(array $Columns){ $this->Columns = array_merge($this->Columns, $Columns); } public function addColumn($Column){ $this->Columns[] = $Column; } public function getResult(){ return $this->Result; } public function getLastQuery(){ return $this->LastQuery; } public function fetchArray(){ if($this->Result !== false){ return MySQL_Fetch_Array($this->Result); }else{ return false; } } public function execute(){ $Query = SPrintF('SELECT %s FROM %s WHERE %s', count($Columns) > 1 ? implode(', ', $Columns) : '*', $this->Table, implode(' AND ', $this->Clauses)); $Result = MySQL_Query($Query); return $this->getResult(); } }

I've just written it out here so it's untested, but it's implementation would be:

PHP Code:

<?php $Select = new Select('Food', array('Name')); $Select->execute(); echo '<h3>All Food:</h3>'; echo '<ul>'; while($row = $Select->fetchArray()){ printf('<li>%s</li>', $row['Name']); } echo '</ul>'; $Select->AddClause("Type = 'Fruit'"); $Select->execute(); echo '<h3>Just Fruit:</h3>'; echo '<ul>'; while($row = $Select->fetchArray()){ printf('<li>%s</li>', $row['Name']); } echo '</ul>'; $Select->AddClause("Name LIKE 'a%'"); $Select->execute(); echo '<h3>Just Fruit Beginning with A:</h3>'; echo '<ul>'; while($row = $Select->fetchArray()){ printf('<li>%s</li>', $row['Name']); } echo '</ul>';

A query class doesn't have many uses, but if you were to use one and base it on MySQL (PDO would be a much better option) then the above should be a good example.

You should also try to separate the dependency on MySQL - as the Select object should only be concerned with building the right SQL string.

Pass the Select object to an executor of sorts, the executor should accept a Select object and return an array of results.

This would further allow you to utilise many other storage mediums without modifying the Select object's code.