I'm using CodeIgniter to build a php web application, and I'm trying to use good OO practices - of which there appears to be many schools of thought. I specifically have a class biography_model to interact with a MySQL table. This data model has some class properties representing the columns in the table, but it also has some properties not in the table such as $image_url
. The class constructor function accepts an optional record ID parameter which then fetches that record from the table and sets all object properties by calling the get_biography()
method, including the $image_url
property not in the table. This way I can instantiate a new biography_model object in the controller with all useful properties ready to go: $bio = new biography_model($id);
But, what is the best approach when we are returning a multi-row result set of records from the table? For each record I need to also set the $image_url
. I could do this in the controller, by querying the list of records in the table and then passing each id into the new biography_model($id) object. But then I would have a situation where the controller is directly querying the database bypassing the model.
Instead, I choose to return an array of biography_model objects from within the biography_model.
Example:
class Biography_model extends Model
{
/**
* This model manages biography information in the 'biography_content' table.
* If a biography ID is passed in when instantiating a new object,
* then all class properties are set.
*/
protected $id;
protected $person_name;
protected $title;
protected $image_file_name;
protected $image_url;
protected $biography_text;
protected $active;
/**
* Constructor
*
* If an id is supplied when instantiating a new object, then
* all class variables are set for the record.
*/
public function __construct($person_id = NULL)
{
parent::Model();
if(isset($person_id))
{
$this->set_property('id',$person_id);
$this->get_biography();
}
}
/**
* Sets supplied property with supplied value.
*/
public function set_property($property, $value)
{
// Set image path if $value is the file name
if($property == 'image_file_name')
{
$this->set_property('image_url',$this->get_bio_img_url($value));
}
$this->$property = $value;
}
/**
* Gets requested property value.
*/
public function get_property($property)
{
return $this->$property;
}
/**
* Returns the biography thumbnail image URL
*/
public function get_bio_img_url($image_name)
{
return $this->config->item('parent_url').'assets/img/biography/'.$image_name;
}
/**
* Get one or more biography entries
*/
public function get_biography()
{
// If the ID is set then set model properties.
if($this->get_property('id'))
{
$this->db->where('id',$this->get_property('id'));
$query = $this->db->get('biography_content');
if($query->num_rows() == 1)
{
foreach($query->row() as $key => $value)
{
$this->set_property($key, $value);
}
}
}
// Otherwise return result set of all biographies
else
{
// Get the list of record ID's
$this->db->select('id');
$query = $this->db->get('biography_content');
if ($query->num_rows() > 0)
{
// New array to return result set
开发者_JS百科 $biography_list = array();
// For each record, return a new biography_model object
foreach($query->result() as $value)
{
$biography_list[] = new biography_model($value->id);
}
}
return $biography_list;
}
}
}
// End of Biography_model Class
It works. But is it a reasonable approach? Are there other more accepted methods? I'm keenly aware that I am querying the database twice, but I was not sure of a better way to handle this. All suggestions are welcome!
Thanks, Wolf
Usually it's better for functions to have one job. Your get_biography() function has 2: get one biography and get all biographies. Consider splitting them up into 2 functions. Also there's no need for the multiple db access.
public function get_biography($id=null)
{
$this->db->where('id', $this->get_property($id))
$query = $this->db->get('biography_content');
foreach($query->row() as $key => $value)
{
$this->set_property($key, $value);
}
}
public function get_biographies()
{
$biography_list = array();
// don't limit this query to just id's - get everything
$query = $this->db->get('biography_content');
// For each record, return a new biography_model object
foreach($query->result() as $row)
{
$model = new biography_model();
// set the properties you already have straight onto the new model
// instead of querying again with just the id
foreach($row as $key => $value)
{
$model->set_property($key, $value);
}
$biography_list[] = $model;
}
return $biography_list;
}
Also you might want to take advantage of php's __get
and __set
magic methods:
public function __get($property)
{
if(!isset($this->$property))
return null;
return $this->$property;
}
public function __set($property, $value)
{
if(!property_exists($this, $property))
return;
if($property == 'image_file_name')
{
$this->image_url = $this->get_bio_img_url($value);
}
else
$this->$property = $value;
}
This will let you get properties on your model like this: $bio->title
instead of $bio->get_property('title')
while at the same time provide a place you can introduce new logic later.
Using an array to represent a set of records is a perfectly valid approach.
However the property image_url
directly depends on the value of another property, so it doesn't make sense to store it as a separate field. Just calculate it on the fly, in your case you'd have to do that in the get_property
method.
On the other hand should the model really be responsible for dealing with URLs? I don't think so. There should be a method outside the model that takes the Biography_model
object and generates the URL of the image based on its image_file_name
. If you already have some routing module responsible for mapping controllers to URLs, this code should probably land there.
精彩评论