Is there a better way of doing this?

[yukimushu]

I need to get loads of data out of the database for a product (for example) and want to show it on a detail page, at present I’m getting the data from the database and setting turning them into properties then using getters to get them from the class (see below for an example), but surely there has to be a better way that I’m not aware of?

class.product.php

class Product {

private $db;

public function __construct($db) {
 if (!$db instanceof Data_IDatabase) {
  throw new Exception("Unknown database object.");
 } else {
  $this->db = $db;
 }
}

public function getProduct($productId) {

 $res = $this->db->query("SELECT title, keywords, description, info, live FROM product WHERE pid = '" . (int) $productId . "' LIMIT 1");
 if ($this->db->numRows($res) > 0) {
  $row = $this->db->fetchAssoc($res);
  $this->title = $row['title'];
  $this->keywords = $row['keywords'];
  $this->desc = $row['description'];
  //etc etc
 }

}

public function getTitle() {
 return $this->title;
}

public function getKeywords() {
 return $this->keywords;
}

public function getDescription() {
 return $this->desc;
}

 //other get functions which get the above properties

}

product.php

<?php $product = new Product($db); ?>

<h1><?php echo $product->getTitle(); ?></h1>

<?php echo $product->getDescription(); ?>

<!-- rest of get methods here -->

?>

[system]

I felt the same way… so I put together a framework that I’m finishing right now (QiPHP) that as a DB-access layer that auto-accesses the DB schema and generates classes for you as you request them.

for example, if you have a table called “product” with a reference to another table called “category”, you could do something like this:

// Get an ArchetypeIterator (a list of products)
$products = new ArchProduct();
$products = $products->findBy(array("id" => array("BETWEEN", 1, 100));

// Loop through the iterator (retrieve ArchProduct objects, which extend the Archetype abstract)
foreach ($products as $product) {
    print($product->price);
    print($product->title);
    $category = $product->getParent("category_id");
    print($category->title);
};

Yiou can insert a new record pretty easily too… assume your form fields match your column names:

$product = new ArchProduct($_POST);
if ($product->save()) {
    print($product->id); // Auto-inserted id is added to the object
    print($product->created); // Timestamp of creation added to the object
}

To extract formatted data, you can do sometihing like this:

$product->getData(DataFormat::ASSOC_ARRAY);
$product->getData(DataFormat::OBJECT);
$product->getData(DataFormat::XML);
$product->getData(DataFormat::JSON);

Let’s say you have encryption on a field in your DB… just define it in your settings file, and you never have to worry about it again… when you get / set, it will automatically encrypt or decrypt the data per your settings.

There’s lots of other cool stuff I automated in the framework, too… I’ll post an update here when I get everything documented and polished to a level where it’s ready to publish.

cheers!

[kyberfabrikken]

This:

public function __construct($db) {
 if (!$db instanceof Data_IDatabase) {
  throw new Exception("Unknown database object.");
 } else {
  $this->db = $db;
 }
}

is better represented as:

public function __construct(Data_IDatabase $db) {
  $this->db = $db;
}

You have declared $db as private. Unless you have a reason for this, I would suggest that you default to declare variables as protected. Other people will have different opinions on this though.

Instead of this:

$res = $this->db->query("SELECT title, keywords, description, info, live FROM product WHERE pid = '" . (int) $productId . "' LIMIT 1");

… I would suggest that you use bound parameters.

[yukimushu]

@kyberfabrikken

This:

public function __construct($db) {
 if (!$db instanceof Data_IDatabase) {
  throw new Exception("Unknown database object.");
 } else {
  $this->db = $db;
 }
}

is better represented as:

public function __construct(Data_IDatabase $db) {
  $this->db = $db;
}

You have declared $db as private. Unless you have a reason for this, I would suggest that you default to declare variables as protected. Other people will have different opinions on this though.

Instead of this:

$res = $this->db->query("SELECT title, keywords, description, info, live FROM product WHERE pid = '" . (int) $productId . "' LIMIT 1");

… I would suggest that you use bound parameters.

Thank you for your advice, especially regarding the protected. It makes much more sense protecting it.

Regarding the prepared statement, from my understanding doing a single query as I’ve done it’s a bit unnecessary to use a prepared statement? I’m under the impression that an ideal time to use a prepared statement would be if I were performing a query whilst iterating through a while loop (for instance).

[system]

So the best way to do this would probably be to model your generic DB access stuff in an abstract parent class, and inherit that functionality in derivative classes, e.g.

// Create a Singleton DB connector
$conn;
class DBConnector {
    const DSN = "mysqli://whatever:whatever@whatever/foobar";
    private function __construct() {
    }
    public static function getConnection() {
        // Conditionally return established connection or new conn
        // Look up singleton patterns for more info.
        $GLOBALS["conn"] = mysqli_connect();
        return $GLOBALS["conn"]
    }
}
abstract class DAO {
    protected $conn;
    public function __construct() {
        $this->conn =& DBConnector::getConnection();
    }
    protected function query($sql) {
        $result = mysql_query($sql, $this->conn);
        // Handle the result
    }
}

class ProductDAO extends DAO {
    public function __construct($id) {
        $this->query("SELECT title, keywords, description, info, live FROM product WHERE pid = '" . (int) $productId . "' LIMIT 1")
    }
}

// Etc etc.

[oddz]

(untested)

/*
* Product domain object
*/
class Product {
	public
	$id
	,$title
	,$keywords
	,$description
	,$info
	,$live;
}

/*
* Product mapper
*/
class ORMProduct {

	protected	
	$db;

	public function __construct(Data_IDatabase $db) {
		$this->db = $db; 
	}
	
	/*
	* Get single product by id
	*
	* @param int products id
	* @return Product product domain object
	*/
	public function fetchProductById($id) {
	
		$res = $this->db->query(sprintf(
			'SELECT
			       pid
			      ,title
			      ,keywords
			      ,description
			      ,info
			      ,live 
			   FROM 
			      product 
			  WHERE 
			      pid = %u 
			  LIMIT 1'
			,(int) $id
		));
		
		if ($this->db->numRows($res) > 0) {
			return $this->_loadProduct( $this->db->fetchAssoc($res) );
		}
		
	}
	
	/*
	* Map datadase product row to domain object
	*
	* @param array product row
	* @return Product domain object
	*/
	protected function _loadProduct($row) {
	
		$product = new Product();
		$product->id = $row['pid'];
		$product->title = $row['title'];
		
		/*
		* In the case keywords is a comma separated list of
		* keywords it could be transformed into an array of
		* each separate keyword here.
		*/
		$product->keywords = $row['keywords'];
		
		$product->description = $row['description'];
		$product->info = $row['info'];
		
		/*
		* In the case live is a boolean it could be properly
		* transformed into a true boolean value here.
		*/
		$product->live = $row['live'];
		
		return $product;
	
	}

}

[TomB]

I’d add a bit more reusability to that

<?php 
abstract class DataMapper {
	
	protected $pdo;
	protected $findStmt;

	abstract protected function getTable();
	abstract protected function getType();
	abstract protected function getPrimaryKey();

	
	public function __construct(PDO $pdo) {
		$this->pdo = $pdo;
	}	

	public function findById($id) {
		if (!$this->findStmt) {
			$reflect = new ReflectionClass($this->getType());
			$properties = $reflect->getProperties(ReflectionProperty::IS_PUBLIC);
			
			$fields = array();
			foreach ($properties as $property) $fields[] = $property->getName();
			
			$this->findStmt = $this->pdo->prepare('SELECT ' . implode(',', $fields) . ' FROM ' . $this->getTable() . ' WHERE ' . $this->getPrimaryKey() . ' = :id');
		}
		
		$this->findStmt->execute(array(':id' => $id));
		return $this->findStmt->fetchObject($this->getType());
	}
}


class Product {
	public $id;
	public $title;
	public $keywords;
	public $description;
	public $info;
	public $live;
}


class ProductMapper extends DataMapper { 	
	protected function getType() {
		return 'Product';		
	}
	
	protected function getTable() {
		return 'product';
	}

	protected function getPrimaryKey() {
		return 'id';
	}
}
?>

That way you can extend the logic to other tables without nearly as much work.

Edit: there was a good discussion here: http://www.sitepoint.com/forums/php-application-design-147/new-php-data-mapper-library-687271.html regarding the next logical step which is handling relations in the mapper.

[djomla]

Also it would be god to implement something like cache on your fetched results.

class Product {
 
private $db;

private $cached;

public function __construct($db) {
 // constructor
}
 
public function getProduct($productId) {
 if (isset($this->cached[$productId])) { return $this->cached[$productId]; }

}

[kyberfabrikken]

@yukimushu

Regarding the prepared statement, from my understanding doing a single query as I’ve done it’s a bit unnecessary to use a prepared statement? I’m under the impression that an ideal time to use a prepared statement would be if I were performing a query whilst iterating through a while loop (for instance).

Prepared statements have two purposes. A) They improve performance when issuing the same query multiple times (although they impose a slight overhead when only used once) and B) they prevent injection type attacks by transmitting query and data on separate channels.

In this case, it’s the latter I’m after. Yes, in theory it doesn’t matter, because you have casted the value to an int, but as a best-practice I always use bound parameters. The overhead of doing so is very small, and I prefer being consistent rather than optimise for specific cases.

[TomB]

And also in this case findById() may well be called multiple times within the same page.

[system]

This is EXACTLY what I mean when I talk about pointless object functions; You have a perfectly good variable reference, what are you doing wrapping it in functions for?

Why would you introduce the overhead of a call with:

public function getTitle() {
 return $this->title;
}

when you could just call $product->title when you output the H1?

I mean, if you were running some sort of other processing when you got that data, sure… You didn’t declare it private, you didn’t declare it protected, you aren’t doing anything extra to it… so why the method?

Though honestly this reeks of “object for nothing” – objects have their place, but with php having auto-complex arrays that function like records/structs, isn’t this kind-of a waste of time?

Though I’d probably just have the getproduct function RETURN $row as it’s result, and then process $row instead of wasting time on all that complex variable assignment. I mean I’m seeing three functions for nothing here. Four really. It’s object for nothing…

function getProduct($db,$productId) {
	if (!$db instanceof Data_IDatabase) {
		throw new Exception("Unknown database object.");
	} else {
		$res=$this->db->query("
			SELECT title, keywords, description, info, live 
			FROM product
			WHERE pid = '".(int)$productId."' LIMIT 1
		");
		return ($this->db->numRows($res)>0) ? $this->db->fetchAssoc($res) : false;
 	}
}

Then you just: (assumes you’re passing an ID, none of the code presented actually seems to CALL getProduct)

<?php 

if ($product=getProduct($db,$id)) {
	echo '
		<h1>',$product['title'],'</h1>
		',$product['description']';
} else echo '
	<h1>Product not found</h1>';
	
?>

Objects have their place, but they shouldn’t be used as be-all catch-alls or have all that extra code and overhead added for a simple record retrieval. DB is passed by reference anyhow so making a permanant copy is just a wasted pointer and memory. Don’t use objects when they don’t provide anything useful. As the old joke goes about memory management, “The heap is your friend, the stack is your enemy.”

[TomB]

I agree with you about getters being mostly pointless.

However, your solution has several drawbacks:

The presentation logic contains the findProduct() call making it no longer reusable as the product must come from this function.

You say “object for nothing” but you have lost:

-Any chance at polymorphism. You cannot have different types of product within the system

-You need to pass the database object around to every function that needs it, making the database a dependency anywhere that calls it. Case in point: Your presentation logic has a dependency on the $db variable.

-Your product must now come from that query as the only way to get a product into the presentation logic is that method which references the database. This severely limits re-usability. For instance, I may want to display the users most recently ordered product.

-A clear API for the product. Who knows what the $product array contains without looking at the code which creates it? By making $product an object it’s possible to look at the class definition and see exactly what fields are available.

-Your logic is tied to products and only products, you can’t re-use any of the data fetching logic for other tables/entity types

I suggest you do a little research into the benefits of OOP before suggesting they’re a “waste of time” and that it’s “object for nothing”.

[system]

@TomB

I agree with you about getters being mostly pointless.

that was really the main point – I’ve seen a LOT of people doing that in their code the past few months – is there some new book out there or website tutorial telling people to do that or something? It makes no sense and there HAS to be an origin for it.

@TomB

However, your solution has several drawbacks:

Mostly dependent on how you handle the code and what else is going on in the program… Most of your points are valid if the program is huge/complex…

@TomB

The presentation logic contains the findProduct() call making it no longer reusable as the product must come from this function.

Which is how I like it since it’s unlikely I’ll store the result for long term handling and if I do, I can call it once, do what I’m doing with it, and dispose it.

You say “object for nothing” but you have lost:

@TomB

-Any chance at polymorphism. You cannot have different types of product within the system

Yes, but different products would likely overwrite enough functionality there (given what we’re seeing) that the code to inherit from the product class may be larger/harder to maintain than just writing a new “getproduct” function for a different type.

Though honestly I’d use prepared queries and pass the appropriate one to the function to handle different ‘product types’ just as I do when supporting more than just mySQL… since ALL we’re really talking about doing so far is grabbing one record from a table…

… and that’s literally ALL we’re talking about. If we don’t waste time processing it in the object there’s no reason by passing a different query you couldn’t grab any number of different field layouts based on the table you’re taking it from. Net change zero.

@TomB

You need to pass the database object around to every function that needs it, making the database a dependency anywhere that calls it. Case in point: Your presentation logic has a dependency on the $db variable.

… and so does creating the object in the first place – net change zero. Admittedly passing by reference on EVERY occurance might be four extra characters and likely a long pointer on the heap (assuming php operates like other languages and passes values on a ‘heap’) – but IMHO that’s better than storing a copy indefinately inside the object where it likely never gets released (since people are lax on doing their own garbage collection).

@TomB

-Your product must now come from that query as the only way to get a product into the presentation logic is that method which references the database. This severely limits re-usability. For instance, I may want to display the users most recently ordered product.

Which would involve a second method on the object or a second function – net change ZERO. There’s no difference!

@TomB

A clear API for the product. Who knows what the $product array contains without looking at the code which creates it? By making $product an object it’s possible to look at the class definition and see exactly what fields are available.

Maybe, but really you’re grabbing records from a table – is it THAT hard to go find the table definition or to SHOCK look at the query since it’s declaring WHAT fields to grab, not just “*”? Notice that – there’s no difference.

@TomB

Your logic is tied to products and only products, you can’t re-use any of the data fetching logic for other tables/entity types

Which is the same procedure or object – you’d still have to make another ‘method’ or pass that extra information – new method is the same thing, zero change.

@TomB

I suggest you do a little research into the benefits of OOP before suggesting they’re a “waste of time” and that it’s “object for nothing”.

They are very useful – I’m a object pascal/modula/smalltalk guy at heart; I’m just not convinced they are useful HERE. I’m not saying that objects are a waste of time, I’m saying that from what we’re seeing for code, they MAY be a waste of time IN THIS INSTANCE. It LOOKS like objects for nothing.

[TomB]

Now I know you’re just on sitepoint for the sake of being argumentative and have little real knowledge of the topic at hand. Enjoy place #1 on my ignore list.

[system]

@TomB

Now I know you’re just on sitepoint for the sake of being argumentative and have little real knowledge of the topic at hand. Enjoy place #1 on my ignore list.

Ah, the “Is not!”, “I know more about it and I’ll prove it by saying nothing” response.

Are you saying the above points are invalid? do you have counterpoints for those? Or are you just going to throw a tantrum and not support any of your claims with facts?

… yeah, and I’m the troll.

[Michael_Morris1]

DS, the style of coding you are putting forth as pure only works when the machine is limited and there is only one programmer - maybe two. It falls apart rapidly when multiple programmers are at work.

Black box abstraction exists to allow programmers to get work done in teams on code bases too large for any one person to fully absorb and comprehend. It is more efficient for the humans who work with it and a cost of being inefficient for the computer itself.

Given the attitude you’ve displayed on this board I doubt you have much, if any, team experience. I can’t imagine any group putting up with you for very long if you act the way you’ve acted on this board in person. I mean, I though I was bad… yeesh.

[system]

@Michael_Morris

DS, the style of coding you are putting forth as pure only works when the machine is limited and there is only one programmer - maybe two. It falls apart rapidly when multiple programmers are at work.

Really. REALLY? So large programming groups do not establish style guides, or take the time for inter-developer communication to take the time to say to the guy maintaining the code you aren’t “Hey, what gives?”

Sounds to me more like the ‘one programmer’ approach of treating a bunch of programmers in their own little cubicle as inviolate, instead of establishing communication, setting up and enforcing rules to smooth workflow, etc, etc…

A team working together has to work together, not have each person off in their own little world doing their own thing and treating what everyone else is working on as ‘untouchable’.

… which is basically what it sounds like you just said.

Though I also smell the 'rules? what do you mean we should have rules" nonsense. Each person off working on their own thing their own way is NO WAY to run a team effort. ANY project director worth their salt will tell you that too.

[Michael_Morris1]

@deathshadow60

Really. REALLY? So large programming groups do not establish style guides, or take the time for inter-developer communication to take the time to say to the guy maintaining the code you aren’t “Hey, what gives?”

They do, but they don’t write the code so that every programmer has to be as familiar with each block of code as the guy who wrote it. It just isn’t practical after the group gets large enough.

Groups face the same complete graph problem the code itself does. As the organization grows it has to subdivide the programmers into manageable teams. After awhile no one has the time to go over the whole.

To make it manageable both the code and the groups working on it limit their contact lanes through established channels – API’s in the code, and hierarchy in groups.

Businesses fail at two critical junctures - the most obvious is when they are getting started. The second is when they try to make the leap from small company where all 2 to 6 employees can have a meeting to a large enterprise with 20 to 60 employees. As companies grow they start losing the agility of small businesses because of the overhead incurred. But this is preferable to not being able to grow.

The reason is that techniques that work for small businesses don’t scale. I bring this up because this also applies to coding.

Your nostalgic coding approach you cry is superior to mine and Tom’s simply doesn’t scale DS. A modern computer spends a not inconsiderable amount of its resources simply tracking its resources, but that’s unavoidable. In a small program on a TRS 80 there might be a handful of components with only a couple dozen interactions. You simply can’t fit more into such a computer.

Modern computers have no such size constraints. Current processors have more transistors on them than the human population of the earth. Without dependency management and class black boxes every part of the code touches every other. The progression - Kn - spirals out of control. K(100) is 5,050 - and adding even 1 node adds another 100 connections and forces a retest of all now 5,150 connections. It simply can’t be done.