2
\$\begingroup\$

I have made this class for database connections to my applications. It eases out the task of making connections to the database with knowing the credentials in advance and provides a way to execute the queries.

Class dbConnection {

    public $CONN;
    private $dbservername, $dbusername, $dbpassword, $dbname;

    public function __construct() {

        $this->dbservername = "localhost";
        $this->dbusername = "dbroot";
        $this->dbpassword = "pass";
        $this->dbname = "test";
        $this->open(); //open the connection on instantization
    }

    public function open() {

    // Create connection
        $this->CONN = new mysqli($this->dbservername, $this->dbusername, $this->dbpassword, $this->dbname);

        // Check connection
        if ($this->CONN->connect_error) {
            die("Connection failed: " . $CONN->connect_error);

        }

    }

    public function prepare($query) {
    //prepare the data
        return $this->CONN->prepare($query);

    }

    public function close() {
    //close the connection
        $this->CONN->close();

    }

}

I am using prepared statements in the queries, of course.

I would like a review to determine if the function is feasible in all situations with performance and security on top considerations. I want to know if I should make any adjustments to it. Also, a comment on the coding style would be of great use.

\$\endgroup\$
1
  • \$\begingroup\$ Don't do that. This class is just a mysqli, with another interface. The simplest purpose of a wrapper should be that you can change the underlying implementation later, and you can't do that with this. Search for simple ORMs or query builders, they will help you better. \$\endgroup\$
    – vfsoraki
    Aug 15, 2016 at 20:34

1 Answer 1

2
\$\begingroup\$

Structure

Currently, there is no reason to have prepare or close, they are just delegating to the public field. A user of your class may just as well use that, and may be confused about the difference.

There is also no need for open() to be public, as there is no need to ever reopen the connection.

If you think you do need a wrapper around mysqli, it should: 1) actually add new functionality or make the existing functionality considerably easier to use 2) be an extensive wrapper, so that there is no need to make CONN public so that it can be accessed directly.

Also, don't die in a class, it makes it difficult to (re-)use. What if you want to fall back on something else? Or show a proper error message? Throw an exception instead.

Also, it would be a good idea to pass the credentials to the class, so it is more flexible (you may eg want to load them from a config file or at least a file only containing the credentials, so you can exclude them from versioning, etc).

Style

Your indentation is off, and you have quite a bit of unnecessary vertical whitespace.

Variable names should all follow a common naming scheme, for example camelCase. I see no apparent reason why $CONN is all uppercase.

Class names should start with an upper-case character.

Comments

Your comments just repeat what the code already told me (close() closes, prepare() prepares, and so on). Comments like these will lead me to believe that all your comments will not be useful to me, which leads to me missing comments that actually are important.

Result

If you follow all my advise, your code may look like this:

class DBConnection {

    public $connection;

    public function __construct($dbServername, $dbUsername, $dbPassword, $dbNam) {
        $this->connection = new mysqli($dbServername, $dbUsername, $dbPassword, $dbName);
        if ($this->connection->connect_error) {
            throw Exception("Connection failed: " . $connection->connect_error);
        }
    }
}

Wrapping mysqli like this may still give you some flexibility later on, but personally, I'm not sure that it is worth it. I would either use mysqli directly, or wrap it in such a way that no element of mysqli is actually required outside of the class.

\$\endgroup\$
2
  • \$\begingroup\$ How come my indentation is off? \$\endgroup\$
    – twodee
    Aug 16, 2016 at 2:11
  • 1
    \$\begingroup\$ I agree with the comment here that this class really adds no value. \$\endgroup\$
    – Mike Brant
    Aug 22, 2016 at 20:40

Your Answer

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

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