current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite
2

I am building a web app that needs to access various tables in a given MySQL db. To do that, I have created a generic class, see DAO_DBRecord.php below, and I create children classes for each table I want to access, e.g., DAO_User.php is used to access the users table. The only difference in the children classes is that I change the value of the variable which contains the name of the table I wish to access.

My questions are:
1 - Should I make the DAO_DBRecord.php class abstract? I'm not planning to instantiate it, and in fact as it is written I cannot access any table with it.
2 - Should I use constants in the DAO_DBRecord.php class (e.g., const USER_TABLENAME = 'users'; const ARTICLE_TABLENAME = 'articles';) and reference them in the children classes (e.g., private Stable = USER_TABLENAME;)?
3 - Anything else I should do?

Class_DB.php: 

<?php
    class DB {
        private $dbHost;
        private $dbName;
        private $dbUser;
        private $dbPassword;

        function __construct($dbHost, $dbName, $dbUser, $dbPassword) {
            $this->dbHost=$dbHost;
            $this->dbName=$dbName;
            $this->dbUser=$dbUser;
            $this->dbPassword=$dbPassword;
        }

        function createConnexion() {
            return new PDO("mysql:host=$this->dbHost;dbname=$this->dbName", 
                        $this->dbUser, $this->dbPassword);
        }
    }
?>

DAO_DBRecord.php 

<?php
    require_once('Class_DB.php');

    class DAO_DBrecord {
        private $dbh; // This is an instance of Class_DB to be injected in the functions.
        private $table=NULL;

        function __construct($dbh){
            $this->dbh=$dbh;
        }
        function dbConnect(){
            $dbConnection=$this->dbh->createConnexion();
            return $dbConnection;
        }
        function checkRecordExists($recordIdentifier, $tableColName){
            $dbConnection=$this->dbConnect();
            $query=$dbConnection->prepare("SELECT COUNT(*) FROM $table "
                . "WHERE $tableColName = :recordIdentifier");
            $query->bindParam(":recordIdentifier", $recordIdentifier);
            $query->execute();
            $result=$query->fetch(PDO::FETCH_ASSOC);
            if ($result["COUNT(*)"]>0){
                return true;
            } else return false;
        }
    }
?>

DAO_Users.php 

<?php
    class DAO_User extends DAO_DBRecord {
        private $table='users';
    }
?>
share|improve this question
    
Why not something like stackoverflow.com/a/368990/567663 ? –  Paul Dec 16 '11 at 2:18
    
Sorry Paul - I don't understand your link. –  JDelage Jan 3 '12 at 22:12

1 Answer 1

up vote 2 down vote accepted

If the only difference between subclasses of DAO_DBRecord is the table name I wouldn't use subclasses. I'd just pass the table name to to the constructor of DAO_DBRecord and use different instances for different tables.

If the only difference between subclasses of DAO_DBRecord is the table name I would pass the name of the table to the constructor of the parent class:

class UserDao extends DAO_DBRecord {

    public function __construct($db) {
        parent::__construct($db, 'users');
    }
}

Having different classes improves type safety. In Java you can't pass a UserDao to a method with an ArticleDao parameter(1), it does not compile. I don't know whether this kind of type safety exists in PHP or not, but it could be a good practice and could results more readable code.

(1) Of course except if UserDao is a subclass of ArticleDao

I don't feel that making DAO_DBRecord class abstract would make a big difference. If your clients wants to misuse the class they can create a dummy (non-abstract) subclass for that:

class AnyDao extends DAO_DBRecord {

    public function __construct($db, $tableName) {
        parent::__construct($db, $tableName);
    }
}

If you want to protect DAO_DBRecord from instantiation with unknown table names make its constructor private and create static factory functions for your tables inside the DAO_DBRecord:

class DAO_DBrecord {
        private $db;
        private $tableName;

        private function __construct($db, $tableName) {
            $this->db = $db;
            $this->tableName = $tableName;
        }

        public static function createUserDao($db) {
            return new DAO_DBrecord($db, 'users');
        }

        public static function createArticleDao($db) {
            return new DAO_DBrecord($db, 'articles');
        }
        ...
}

So, DAO_DBrecord cannot be used with other tables.

If you use your table names only once creating constants for them looks unnecessary in this case.

share|improve this answer
    
That's the way I had it before, then I thought it would be better programming to separate it, just in case I end up having different needs for different tables. –  JDelage Dec 16 '11 at 0:21
    
Sorry, you are right, I've forgotten type safety for a moment. I've updated the answer. –  palacsint Dec 16 '11 at 0:35
    
Thanks. Do you think the parent class should be abstract? –  JDelage Dec 16 '11 at 4:30
    
Check the update, please. –  palacsint Dec 16 '11 at 11:16
    
Why do you make the static functions createUserDao and createArticleDao private? –  JDelage Jan 3 '12 at 23:08

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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