1
\$\begingroup\$

I am building my own MVC and I have a couple questions about making a database connection and what is the best practice. I have a model User which interacts with the database and I wonder what is the best practice to instantiate the database connection class. Currently I made it so my User class extends the Database class because I don't feel like instantiating it in a constructor for example.

class User extends Database
{    

    public function data($userid)
    {        
        $getRow = $this->getConnect()->getRow("SELECT * FROM users WHERE userid = ?", [$userid]);

        return $getRow;
    }

}

And my Database class:

class Database
{

    private $dbh = NULL;
    private $instance = NULL;

    public function __construct($username = 'root', $password = '', $host = '127.0.0.1', $dbname = 'testdb', $options = [])
    {
        try  {
            $this->dbh = new PDO("mysql:dbname={$dbname};host={$host};charset=utf8", $username, $password);
            $this->dbh->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
            $this->dbh->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_OBJ);
        } catch (PDOException $ex) {
            throw new Exception($ex->getMessage());
        }

    }

    public function getConnect()
    {
        if($this->instance == NULL)
        {
            $this->instance = new self();
        }
        return $this->instance;
    }
}

Also, my getConnect method seems kinda hard coding. Do you think there is a smarter way to do this?

And one more thing, about public/private if anything is better to be private, I'd appreciate if you let me know.

\$\endgroup\$
1

3 Answers 3

1
\$\begingroup\$

Is a "user" a "database"?

That doesn't sound right. A database is a tool or service that provides fast access to data via queries, and a user sounds like a representation of a real-life user, described by a set of characteristics. A representation of a user may use a database to store its data, but it is not a database.

In short, User should not extend Database. A User could be a container for user data, and User instances could be created by a UserFactory or UserRepository which could be implemented to fetch the user data from a database or some other backend.

Getting fields from a database

Instead of SELECT * queries, it's recommended to name the required fields explicitly. This can reduce the amount of data transferred from the database, and it eliminates dependence on the ordering of columns.

What is getConnect ?

The getConnect function returns the Database singleton instance, not a "connect". Getting a "connect" seems meaningless. getInstance is a more common name in singleton implementations.

public or private

As a good rule of thumb, make everything private, and then change some of them public as necessary.

The less implementation details are visible, the better. Implementation details that are not visible give you the flexibility to change those details without affecting the users.

\$\endgroup\$
3
  • \$\begingroup\$ All of this makes sense and thank you for your response but I don't understand how to better instantiate the database class in this case. \$\endgroup\$ Jul 31, 2017 at 22:12
  • \$\begingroup\$ is a "database" really an "object" - you could use the same logic to argue against having a database object entirely. \$\endgroup\$ Jul 31, 2017 at 23:34
  • \$\begingroup\$ @Iwrestledabearonce. in OOP, everything is an object, so this doesn't seem too hard to accept. It helps that don't write anywhere extends object. It also helps that in a design diagram you will not draw a box for object and connect everything else to it. Asking the "is-a" question is a very common question to ask to verify if X should inherit from Y, and I find it comes very naturally. \$\endgroup\$
    – janos
    Aug 1, 2017 at 5:15
0
\$\begingroup\$

To sipplement the excelent answer from @janos

I don't understand how to better instantiate the database class in this case.

The ideal solution would be to supply a database service through constructor parameters.

class User
{
    protected $db;

    public function __construct(Database $db)
    {
        $this->db = $db;
    }
    public function data($userid)
    {        
        return $this->db->getRow("SELECT * FROM users WHERE userid = ?", [$userid]);

    }
}

And so you just instantiate your database class and then pass its instance into User class

$db = new Database();
$user = new User($db);
$data = $user->data($userid);

If you won't find this solution plausible, you will have to resort to some sort of static-based approach. But note that blunt singleton database class is frowned upon. It is good for the procedural code but not recommended to use in the OOP environment. The simplest tradeoff solution would be a class that can return its instance through static function.

\$\endgroup\$
1
  • \$\begingroup\$ this is not how you do you do models bro \$\endgroup\$ Aug 1, 2017 at 12:36
0
\$\begingroup\$

You're making an MVC.

A User would be a M, so it should extend a model class of some sort. That Model class would typically create a reference to the database so it would not be necessary to pass a database connection to every instance nor would it be needed to extend the DB for every model.

Make your database class static so you're not re-creating database connections every time you create a user, and don't make stuff extend the database class unless it's stuff that is directly and specifically related to databases. Note also that PDO can construct these classes for you, straight from the database (if you're using PDO).

Here's some quick n dirty untested sample code...

class Model{
    private static $db;
    public static function db(){
        if(empty(self::db)){
            self::$db = ///.. generate pdo connection from your
                        // custom MVC's config file
        }
        return self::$db;
    }
}

class User extends Model{
    private $name;
    private $phone;
    public function __construct($id){
        $q = self::db()->prepare('select name, phone from users where id = ?');
        $q->execute(array($id));
        $q->setFetchMode(PDO::FETCH_INTO, $this);
        $q->fetch();
    }
}

Check out PDO's FETCH_INTO and FETCH_CLASS for some more options

\$\endgroup\$
4
  • \$\begingroup\$ @user3628807 don't use this flawed approach or it will kill your database server. \$\endgroup\$ Aug 1, 2017 at 13:41
  • 1
    \$\begingroup\$ lol please explain common, as suspect you're just being typical common drama queen \$\endgroup\$ Aug 1, 2017 at 13:42
  • \$\begingroup\$ Creating a connection in the model is the obvious misconception, that will inevitable lead to multiple connections created from the same script. It is so obvious that I am really surprised why anyone would upvote this answer \$\endgroup\$ Aug 1, 2017 at 16:58
  • \$\begingroup\$ @YourCommonSense - look closer. this will only create a single connection, no matter how many models are created. do you understand what the static keyword does? \$\endgroup\$ Aug 1, 2017 at 18:56

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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