Overview
This isn't OOP. To me, object-oriented programming requires thinking about your programs in terms of classes, their responsibilities, their attributes, and behaviours. This means:
- Information hiding
- Encapsulation
- Polymorphism
- Collaboration
Config Class
Dearth of source code comments aside, let's look at the Config class. Here are some questions about the configuration options:
- What are the options?
- What do each of the options do?
- How do the options affect the application?
- How are the options communicated to the end user?
In this case, the Config "class" as written is nothing more than a functional structure (i.e., it is a dynamic array) wrapped in the syntax of a PHP class. A class, by definition, allows instances to have state and behaviour. The Config class has no discernible behaviour. To give it behaviour, we'd have to contemplate the purpose of the class: its raison d'être.
The name "Config" indicates that the class is responsible for some kind of configuration. (Note: I absolutely abhor abbreviations.) I would further imagine that the configuration is specifically for the CMS application. So I would have named the class CMSConfiguration, which inherits from a more generic Configuration class. Let's look at that generic class.
What can an application configuration class do? I imagine a general configuration class can do the following:
- Load options (from a stream, a file, command line switches)
- Save options (to maintain current state, or save preferences)
- Answer whether an option is set
- Answer whether an option is equal to a given value
- Export help for the options in an display-device-neutral markup language
That's its behaviour. The single responsibility is clear: the Configuration class loads and queries the state of configuration options. The implementation might resemble:
<?php
/**
* Responsible for loading and saving configuraiton options.
*/
class Configuration {
/**
* Loads the configuration options from application's database connection.
*/
public function load() {
Database database = Database::getInstance();
$result = database->call( "get_configuration_options" );
// Set the configuration options using the result set.
}
/** Saves the current state of the configuration options. */
public function save() {
}
/**
* Returns true to indicate that the option with the given name is set.
*
* @param $option The name of the option to check.
* @return true The option has a value that is not false.
*/
public function isOptionSet( $option ) {
return $this->getOption( $option );
}
/** ... */
public function isOptionEqual( $option, $value ) {
return $this->getOption( $option ) === $value;
}
/** Writes the help for all the options as an XML document. */
public function exportHelp() {
foreach( $this->getOptions() as $option ) {
// ...
}
}
}
?>
There seems to be a trend of not declaring attributes, but using magical __set and __get functions combined with dynamic arrays. This will lead to unmaintainable code. If the variables that are used by a class are not defined in a single location, how do you expect anyone else to understand how the code works?
You could, for example, have ClassA and ClassB. What is stopping ClassB from using ClassA as its container for attribute values? Nothing. And that's a terrible, terrible thing, as it completely violates both information hiding and encapsulation.
SQL Class
Judging by its name, the SQL Class seems to be some kind of holder for SQL statements? This will directly violate encapsulation again, as the class that needs to use the SQL will have to call the SQL class to get it. The SQL class should know nothing about the SQL that other classes need to perform.
I actually go a bit further than this. I don't like exposing SQL in my applications at all. SQL changes a lot over the lifetime of a project. When you have many different fingers all creating their own SQL statements, you wind up with a mess: duplicate code, inefficient queries, a burdened query cache, brittle code that will break anytime a table changes, and no clean way to unit test the queries.
The problem, in my mind, is that there is no contract prototype between a CRUD (Create, Read, Update, or Delete) statement and the calling code. This means that the two cannot easily vary independently.
To resolve this, I move the SQL code into stored procedures and stored functions (collectively stored routines). Some people will despise this approach, others will laud it. I prefer it because it means I can create a contract between the stored routine and the underlying code. (It also means that if I wanted to switch from PHP to another language, there's less code to change.)
Once all the SQL code is written in stored routines, it becomes trivial to write a generic database class that can call those routines with arbitrary parameters.
<?php
use PDO;
use PDOException;
/**
* Used for interacting with the database. Usage:
* <pre>
* $db = Database::get();
* $db->call( ... );
* </pre>
*/
class Database {
private static $instance;
private $dataStore;
/**
* Sets the connection that this class uses for database transactions.
*/
public function __construct() {
global $dbhost;
global $dbname;
global $dbuser;
global $dbpass;
try {
$this->setDataStore(
new PDO( "pgsql:dbname=$dbname;host=$dbhost", $dbuser, $dbpass ) );
}
catch( PDOException $ex ) {
$this->log( $ex->getMessage() );
}
}
/**
* Returns the singleton database instance.
*/
public function get() {
if( self::$instance === null ) {
self::$instance = new Database();
}
return self::$instance;
}
/**
* Call a database function and return the results. If there are
* multiple columns to return, then the value for $params must contain
* a comma; otherwise, without a comma, the value for $params is used
* as the return column name. For example:
*
*- SELECT $params FROM $proc( ?, ? ); -- with comma
*- SELECT $proc( ?, ? ) AS $params; -- without comma
*- SELECT $proc( ?, ? ); -- empty
*
* @param $proc Name of the function or stored procedure to call.
* @param $params Name of parameters to use as return columns.
*/
public function call( $proc, $params = "" ) {
$args = array();
$count = 0;
$placeholders = "";
// Key is zero-based (e.g., $proc = 0, $params = 1).
foreach( func_get_args() as $key => $parameter ) {
// Skip the $proc and $params arguments to this method.
if( $key < 2 ) continue;
$count++;
$placeholders = empty( $placeholders ) ? "?" : "$placeholders,?";
array_push( $args, $parameter );
}
$sql = "";
if( empty( $params ) ) {
// If there are no parameters, then just make a call.
$sql = "SELECT $proc( $placeholders )";
}
else if( strpos( $params, "," ) !== false ) {
// If there is a comma, select the column names.
$sql = "SELECT $params FROM $proc( $placeholders )";
}
else {
// Otherwise, select the result into the given column name.
$sql = "SELECT $proc( $placeholders ) AS $params";
}
$db = $this->getDataStore();
$statement = $db->prepare( $sql );
for( $i = 1; $i <= $count; $i++ ) {
$statement->bindParam( $i, $args[$i - 1] );
}
try {
$result = null;
if( $statement->execute() === true ) {
$result = $statement->fetchAll( PDO::FETCH_ASSOC );
$this->decodeArray( $result );
}
else {
// \todo Send an e-mail.
$info = $statement->errorInfo();
$this->log( "SQL failed: $sql" );
$this->log( "Error: ". $info[2] );
}
}
catch( PDOException $ex ) {
// \todo Send an e-mail.
$this->log( $ex->getMessage() );
}
return $result;
}
/**
* Converts an array of numbers into an array suitable for usage with
* PostgreSQL.
*
* @param $array An array of integers.
*/
public function arrayToString( $array ) {
return "{" . implode( ",", $array ) . "}";
}
/**
* Recursive method to decode a UTF8-encoded array.
*
* @param $array - The array to decode.
* @param $key - Name of the function to call.
*/
private function decodeArray( &$array ) {
if( is_array( $array ) ) {
array_map( array( $this, "decodeArray" ), $array );
}
else {
$array = utf8_decode( $array );
}
}
private function getDataStore() {
return $this->dataStore;
}
private function setDataStore( $dataStore ) {
$dataStore->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
$this->dataStore = $dataStore;
}
}
The code is used in one of three ways:
// Returns a boolean value
$result = $db->call( "is_existing_cookie", "existing", $cookie_value );
// Calls a stored procedure
$this->call( "procedure_insert", "", $this->getId(), $action_name_id );
// Returns a tuple pair of ids and labels
return $this->call( "get_list", "id, label", $this->getId() );
The Database class has behaviours (make a database call) and attributes (the singleton instance). There is certainly room for improvement, but this should give you more ideas to ponder.
All of the text is great, but the example code... That database class violates just about every OOP principle there is. Importing global variables? Constructing a PDO instance in the constructor? A singleton? What? Why? Globals have no place in modern PHP, nor do singletons. Dependency injection is the way to go here. And suppressing exceptions? Nooooo. The consuming code needs to know something exceptional happened. Logging is not enough (and I would argue that either an extension, wrapper or consumer of the class should be responsible for logging).
Response to Corbin
That database class violates just about every OOP principle there is.
The Database class violates neither encapsulation nor information hiding. This is apparent from its intended usage:
return $this->call( "get_list", "id, label", $this->getId() );
The underlying implementation (of the call method) can change without any impact to the client code. No information about the internal workings of the Database class is being exposed.
Importing global variables?
True, this is poor form. However, it can be mitigated by using a configuration file, or dependency injection. Again, the calling classes will not need to change.
Constructing a PDO instance in the constructor? A singleton? What? Why?
Globals have no place in modern PHP, nor do singletons.
True, globals are poor form. The Singleton pattern was employed for its simplicity. In my application, every single class uses the Database, but most of the subclasses do not use it directly.
This code is also fairly temporary, as the application will be rewritten using Haxe.
And suppressing exceptions? Nooooo.
Depends on if the calling code needs them. In my case, the calling code never needs to care about the error. Users don't care if there was a database error. There is no point bubbling an error message to the user, as they'll never understand "ORA-10168: INSERT violates duplicate key" or whatever. Ergo, the message is suppressed and the application must handle an empty result set.
In this particular case, all empty result sets yield a relatively empty XML document, which is rendered using XSLT, to produce a web page for the user. There is little difference between the following:
try {
$result = $this->call( ... );
}
catch( Exception $e ) {
$result = $this->getErrorXML();
}
And this:
$result = $this->call( ... );
if( !isset( $result ) ) {
$result = $this->getErrorXML();
}
They are functionally equivalent, although the exception handling allows slightly more flexibility for eliminating duplicate code (by allowing the exception to bubble naturally and get handled in a single location).
As I mentioned, there are a number of minor issues with the Database class, however it does not violate either encapsulation or information hiding.
From a big picture perspective, I'd rather see:
return $this->call( "get_list", "id, label", $this->getId() );
Sprinkled throughout the codebase, than hundreds of SELECT statements married to PDO calls.
