Is it bad practice to have a database query in the constructor for a class in order to load it upon creating a new instance of it?
class Home
{
private $home_id = null;
private $home_name = null;
private $home_number = null;
private $home_street = null;
function __construct($home_id)
{
$do_query = $mysql_con->query("SELECT * FROM home WHERE home_id = '$home_id'");
while ($home_data = $do_query->fetch_assoc())
{
// Set all of the items in the object
$this->home_id = $home_data["home_id"];
$this->home_name = $home_data["home_name"];
$this->home_number = $home_data["home_number"];
$this->home_street = $home_data["home_street"];
}
}
}
I have been told before that this might be bad practice, to have a query that builds up the object in the constructor.
Your Home class is a domain object and those should ideally not be aware of how they're persisted.
Separating the concerns allows for flexibility. This is also referred to as the Data Mapper pattern.
class Home
{
public $home_id;
public $home_name;
public $home_number;
public $home_street;
}
interface HomeMapperInterface
{
public function get($id);
}
class HomeMapper implements HomeMapperInterface
{
public function __construct($db)
{
$this->db = $db;
}
public function get($id)
{
$query = $this->db->query(...);
if (($row = $do_query->fetch_assoc()) === false) {
throw new RecordNotFoundException();
}
$home = new Home;
$home->home_id = $row['home_id'];
// ...
return $home;
}
}
To use it:
$mapper = new HomeMapper($db);
$home = $mapper->get(123);
You can improve this by using Identifier Map to avoid loading the same record twice into separate objects.
Btw, this is only a partial data mapper; it would also be used to update, insert and delete objects from your database.
The Repository Pattern is a useful tool to help mitigate this issue, take a look at this previous SO Discussion.
It doesn't have to be bad practise if you are careful. For example, what happens when the query fails? Does the constructor return a half-baked object or does it throw an exception? Throwing an exception is considered better because operating on half-constructed objects can cause errors that are hard to predict.
Another issue is performance: a database query can be an expensive operation. Some people might be surprised by the constructor taking a long time to complete, but if it's clearly documented there should be no problem.
Another performance issue is batching: if you have to fetch several objects from the database with a design like this you have to make many separate queries to the database when you could make just one and construct objects based on the results.