patternphpMinor
PHP class design with methods requiring database access
Viewed 0 times
withphpdesignmethodsdatabaseclassaccessrequiring
Problem
I've been a procedural PHP programmer for a long time but I'm getting to learn OOP and would like some advice. I'm designing a class which currently is composed mainly of simple getters/setters, but I'm not quite sure if I'm designing my class in the best way possible.
The getters require accessing a database to pull the requested information, so I'm checking if a property has been set first before running the query which seems like a good idea to me but I'm not sure.
I think I could make use of DI by type hinting the $connect parameter and using a DAL but this is not a public facing API and we're never going to use a different database engine so I'm not sure it's worth the trouble. I'm using a proprietary database engine which has a bug preventing me using PDO and parametrised queries. I'm also not sure if I should be try/catching/throwing exceptions inside class methods.
Here is a sample of the code below, there are more methods/properties but they all follow the same basic structure. I would very much appreciate any and all commentary.
```
class Patient {
// Database connector
private $connect;
protected $_connection;
public $dmg_ID;
public $LNK_ID;
public $dailyLNK_ID;
public function __construct($connect, $lnkid, $dlkid = NULL) {
$this->_connection = $connect;
unset($connect);
// Patient requires a LNK_ID but not always a daily link ID
$this->LNK_ID = $lnkid;
if (!is_null($dlkid) && is_numeric($dlkid)) {
$this->dailyLNK_ID = $dlkid;
}
}
public function getDmgID() {
if ($this->dmg_ID) return $this->dmg_ID;
$sql = "SELECT lnk_dmgID FROM LINK WHERE lnk_ID=".$this->LNK_ID;
try {
$result = odbc_exec($this->_connection,$sql);
if($result) {
$dmgID = odbc_fetch_array($result);
}
else {
throw new RuntimeException("Failed to connect.");
}
}
catch (RuntimeException $e) {
prin
The getters require accessing a database to pull the requested information, so I'm checking if a property has been set first before running the query which seems like a good idea to me but I'm not sure.
I think I could make use of DI by type hinting the $connect parameter and using a DAL but this is not a public facing API and we're never going to use a different database engine so I'm not sure it's worth the trouble. I'm using a proprietary database engine which has a bug preventing me using PDO and parametrised queries. I'm also not sure if I should be try/catching/throwing exceptions inside class methods.
Here is a sample of the code below, there are more methods/properties but they all follow the same basic structure. I would very much appreciate any and all commentary.
```
class Patient {
// Database connector
private $connect;
protected $_connection;
public $dmg_ID;
public $LNK_ID;
public $dailyLNK_ID;
public function __construct($connect, $lnkid, $dlkid = NULL) {
$this->_connection = $connect;
unset($connect);
// Patient requires a LNK_ID but not always a daily link ID
$this->LNK_ID = $lnkid;
if (!is_null($dlkid) && is_numeric($dlkid)) {
$this->dailyLNK_ID = $dlkid;
}
}
public function getDmgID() {
if ($this->dmg_ID) return $this->dmg_ID;
$sql = "SELECT lnk_dmgID FROM LINK WHERE lnk_ID=".$this->LNK_ID;
try {
$result = odbc_exec($this->_connection,$sql);
if($result) {
$dmgID = odbc_fetch_array($result);
}
else {
throw new RuntimeException("Failed to connect.");
}
}
catch (RuntimeException $e) {
prin
Solution
OOP is definitely the way to go. The best advise I can offer is to read the book "Clean Code" by Uncle Bob. The code there is in Java but all principles apply equally to any other language.
I would definitely abstract the DB interface so I can use for any other database, even if you don't plan it in the next future. I have posted small data store architecture code that may help you.
From a look at your code, I am a bit confused by the naming
-
Why are you unsetting
-
The names such as
-
I would reserve a separate class dealing with the database via thin interface (see my post) and let other classes only talk to that class instead of DB directly.
I would definitely abstract the DB interface so I can use for any other database, even if you don't plan it in the next future. I have posted small data store architecture code that may help you.
From a look at your code, I am a bit confused by the naming
connect and connection and the latter stores $connect in the constructor. Maybe give a more descriptive names (one of the top recommendations in the mentioned book).-
Why are you unsetting
$connect?-
The names such as
$LNK_ID are cryptic for reader.-
I would reserve a separate class dealing with the database via thin interface (see my post) and let other classes only talk to that class instead of DB directly.
Context
StackExchange Code Review Q#41067, answer score: 2
Revisions (0)
No revisions yet.