3
\$\begingroup\$

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) { 
          print("Exception caught: $e");
      }

      return $this->dmg_ID = $dmgID['LNK_DMGID'];
  }

  public function setDmgID() {

      if (!$this->dmg_ID) return false;

      $sql = "UPDATE LINK SET lnk_dmgID=".$this->dmg_ID." WHERE lnk_ID=".$this->LNK_ID;
      try { 
          $result = odbc_exec($this->_connection,$sql); 
          if(!$result) { 
              throw new RuntimeException("Failed to connect."); 
          } 
      } 
      catch (RuntimeException $e) { 
          print("Exception caught: $e");
      }

      return true;
  }
...
\$\endgroup\$
2
\$\begingroup\$

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 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.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks for the response. This is for a company that is very heavily invested in the current database engine and cannot change without rewriting their entire software suite. Would not building a DAL in this instance violate YAGNI? I'm unsetting $connect because it contains the data necessary to connect to the DB and was taught it's good practice to get unset once it's no longer needed. $LNK_ID is literally the name of the database field it refers to although I do agree the mixing of upper/lower case with/without underscores is confusing. Thanks for your responses :) \$\endgroup\$ – Roy Feb 6 '14 at 14:56
  • 1
    \$\begingroup\$ @Roy PHP is a scripting language with scripts usually run for limited time, after which all your memory will be cleared, so really no need to unset unless you have a serious reason to. Especially your variable is even local to the function. I'd even say it is a dangerous practice - too easy to create a bug by switching rows. This is more of a JavaScript kind of problem but then again, not with local variables. \$\endgroup\$ – Dmitri Zaitsev Feb 6 '14 at 18:52
  • 1
    \$\begingroup\$ @Roy Concerning database field names, in a well structured code, that belongs to your model that mirrors your DB structure. Ideally this should be encapsulated into classes dealing with your models and the rest of your code should not know about their specifics, rather talking to your model classes via well-named API that you can understand without knowing the interior of those classes. \$\endgroup\$ – Dmitri Zaitsev Feb 6 '14 at 18:56
  • \$\begingroup\$ Thanks for your comments! I'll remove the unset. For my education, what would be a situation where unset() would be useful then? I inherited the database and have no say in the structure but again I am 100% sure the db engine will not be changing for various business reasons. In this situation is there a point to abstracting away the DB so much? Seems very much YAGNI - direct SQL queries remove all overheard from constructing some sort of bespoke ORM. \$\endgroup\$ – Roy Feb 7 '14 at 9:21
  • \$\begingroup\$ @Roy unset can be useful to quickly remove keys (or move to different keys) on your model (or its clone) before passing it to persistent storage. Or whenever you want to remove keys of objects or assoc. arrays. \$\endgroup\$ – Dmitri Zaitsev Feb 7 '14 at 12:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.