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

I'm looking to make some improvements to a deliberately crude API written in PHP, it's designed to handle simple requests and talk to the DB.

I have the following DB class:

class DB
{
    public function connect()
    {
        $serverName = ".";
        $connectionInfo = array("Database"=>"Demo","UID"=>"sa","PWD"=>"qwe123");
        $connection = sqlsrv_connect( $serverName, $connectionInfo);

        return $connection;
    }

    public function close($connection)
    {
        return sqlsrv_close($connection);
    }

    public function query($connection, $query, $params = null)
    {
        return sqlsrv_query($connection, $query, $params);
    }
}

And then for requests I do:

$db = new DB();
$conn = $db->connect();

$sql = "INSERT INTO Applications (name, friendly_name, description, enabled, visible, icon, video) VALUES (?, ?, ?, ?, ?, ?, ?)";

$params = array($name, $friendly_name, $description, $enabled, $visible, $icon, $video);

$query = $db->query($conn, $sql, $params);

if( $query === false )
{
    $response = array('error' => 'Error');
}
else
{
    $response = array('success' => 'Success');
}

$db->close($conn);

return $response;

I was wondering how I could improve this further and not have to repeat the db connect and creating an instance of the class and close the connection.

I know I could and should be using PDO, but for this example I want to keep it old school.

share|improve this question

1 Answer 1

up vote 1 down vote

When you are using a class you can utilize a private/protected class property. As said you know about PDO and since I am not very familiar with the old school sqlsrv_* functions I have used PDO in the examples. An example could be.

class Database {

    /*
     * Having a private/protected property ensures that only
     * the class itself can change/removed the connection. This way calling
     * code cannot mess something up regarding the connection.
     */
    private $pdo;

    public function __construct($dsn, $username, $password, array $options)
    {
        $this->pdo = new PDO($dsn, $username, $password, $options);
    }

    public function __destruct()
    {
        $this->close();
    }

    /**
     * This is an utility method to expose the PDO instance. This
     * enables calling code to use the full toolset of PDO.
     *
     * @return PDO
     */
    public function pdo()
    {
        return $this->pdo;
    }

    public function query($sql) 
    {
        /*
         * I do not enclose this inside a try-catch block
         * as only the calling code can decide if the application
         * can recover from the exception state.
         */
        return $this->pdo->query($sql);
    }

    public function close()
    {
        /*
         * Since PDO is an object closing a connection
         * is done by assigning the value NULL.
         */
        $this->pdo = null;
    }

}

If you look at the Database::close() method you can see that no parameters are required. We already have the property storing the current connection available. I am also using a __destruct() method to ensure the connection is closed as soon as the object loses all references. This frees up server resources earlier than if the connection was closed when the script ended.

You can also store the connection details/credentials inside the class an lazy load the connection only when it is actually required. There are plenty of tutorials around the web about this.

Hope this can help, happy coding!

share|improve this answer

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.