1
\$\begingroup\$

I've got two classes for getting a connection to the database and executing things, Class 'A' was made by me and Class 'B' was made by a colleague, we were wondering which one is better for use (Security, efficiency etc), that's why we are asking it here. Which one is better, and why is it? And how can we improve that one?

Class A:

class databaseHandler {

private $db;
private static $instance;


/*

This is the constructor of the PHP class, we create the object for databaseHandler here.

*/
function __construct() {
    $this->createConnection();
}


/*

We return an instance, if the object isn't created yet, we make it a new object and then return it.

*/
public static function getInstance() {
    if(!self::$instance) {
        self::$instance = new databaseHandler();
    }
    return self::$instance;
}


    /* returns the connection to the database, so we can use it to create statments */
public function getConnection() {
    return $this->db;
}

/*
Create a connection to the database, using constants to set the correct data for the connection.
*/
private function createConnection() {
    require 'config.php';
    try {
    $this->db = new PDO("mysql:host=" . HOST . ";dbname=" . DATABASE . ";charset=utf8", USERNAME, PASSWORD);

    $this->db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_WARNING);
    } catch(PDOException $e) {
        echo $e->getMessage();
    }

    }

}
?>

Class B:

class DatabaseConnection
{
 public $dbc, $stmt, $result;

  public function __construct()
  {
    global $dbc;
    $dbc = new PDO("mysql:host=<removed host from post>;dbname=REDACTED", "REDACTED", "REDACTED");
  $dbc->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
  }

public function closeConn ()
{
  global $dbc;
  $dbc = NULL;
}

public function selectAll ($tableName)
{
  global $dbc, $stmt, $result;

  $stmt = $dbc->prepare("SELECT * FROM $tableName");
  $stmt->execute();
  $result = $stmt->fetchAll();
  $this->closeConn();
  return $result;
}

public function getConnection() {
  global $dbc;
  return $dbc;
}

public function updateStock ($artnr, $amountToDecline)
{
  global $dbc, $stmt, $result;

  $stmt = $dbc->prepare("SELECT `stock` FROM `artikelen` WHERE `artnr` = $artnr");
  $stmt->execute();
  $result = $stmt->fetchAll();
  $updatedStock = $result[0]['stock'] - $amountToDecline;
  if ($updatedStock < 0) {
  return false;
  $this->closeConn();
  } else {
    $stmt2 = $dbc->prepare("UPDATE `artikelen` SET  `stock` = '$updatedStock' WHERE  `artikelen`.`artnr` = $artnr");
    $stmt2->execute();
    return true;
    $this->closeConn();
  }
}
}

We are asking this, as we are planning to use this on our first project with an actual customer.

\$\endgroup\$
5
  • \$\begingroup\$ Please include your real code rather than example code. Example code is not meaningfully reviewable. \$\endgroup\$
    – Phrancis
    Feb 8, 2017 at 9:28
  • \$\begingroup\$ Phrancis this is the real code, what do you mean? \$\endgroup\$
    – ThatsAudio
    Feb 8, 2017 at 10:04
  • \$\begingroup\$ Is the updateStock function the same for both implementations or is its implementation just missing in class A? \$\endgroup\$
    – Graipher
    Feb 8, 2017 at 10:29
  • \$\begingroup\$ @Graipher he wanted to add all the functions in the database connection class, i want to execute it in their own classes (etc..) \$\endgroup\$
    – ThatsAudio
    Feb 8, 2017 at 10:50
  • \$\begingroup\$ I think then that a comparative review is a bit hard in this case, because the two things to be compared do not offer the same functionality (and so the one with more functionality will have an advantage off the bat). \$\endgroup\$
    – Graipher
    Feb 8, 2017 at 11:04

1 Answer 1

2
\$\begingroup\$

I think both classes have problems. Class A looks like an improperly implemented singleton and Class B has so many issues (detailed below) that it should probably just be thrown away as it is not really close to being suitable for inclusion in any code base.


In class A, why store both an instance of the class and an instance of PDO? If you really wanted to stick with singleton I would simplify this down to something like:

class PdoSingleton
{
    public static $pdo;

    private function __construct() {}

    public static function getInstance() {
        if(self::$pdo instanceof PDO === false) {
            self::$pdo = new PDO(...);
        }
        return self::$pdo;
    }
}

I would however suggest you guys consider using dependency injection rather than (or at least in conjunction with) singleton. Taking a dependency injection approach allows you to remove code in classes that need a dependency (such as a PDO object) and enforce strict contracts in your code.

So rather than this sort of code:

class ClassThatNeedsPDO
{
    protected $pdo;

    public function __construct() {
        try {
            $this->pdo = PdoSingleton::getInstance();
        } catch (PDOException $e) {
            // do something
        }
        // do other stuff
    }
}

You take this approach:

class ClassThatNeedsPDO
{
    protected $pdo;

    public function __construct(PDO $pdo) {
        $this->pdo = $pdo;
        // do other stuff
    }
}

With you either passing PDO object defined in you bootstrap sequence upon instantiation, or possibly using singleton provider like this:

$obj = new ClassThatNeedsPdo(PdoSingleton::getInstance());

This makes it to where the class that needs that dependency can never even be instantiated if the appropriate dependency, which is type-hinted in parameter declaration, is not passed (an InvalidArgumentException would be thrown). Your code fails loudly, which is what you want.


In class A, why require your config? It should be considered very unusual to have a require inside a class like this. The require should be somewhere in your bootstrap sequence prior to where this singleton is first used. Then you just use the constants as you are currently doing since constants are global in scope.


In class A, while I appreciate that you are trying to work with PDO in exception mode (a good thing) and using try-catch here around the instantiation. Think about what impact this has on your code.

Here if PDO instantiation fails with exception, you basically just swallow it up with only some output to standard out to indicate there was a problem. Subsequent code could call this class, expecting an instance without knowing that the class was instantiated in a bad state. You should be failing loudly here and letting the caller to the class make the decision on how to handle the situation.

I don't think that there is really a case here to even use try-catch, as I would think it reasonable that a PDOException bubble up from a PDO singleton class. Maybe the only reason to use try-catch here is in case you wanted to log some information about the internal state of the class before re-throwing or wrapping the exception. Either way, an exception should be thrown externally when you can't instantiate in an expected state.


A class such as this should never output anything by the way. It should log errors and/or throw exceptions, but not output directly to the user. Leave this to code up the call stack that is better positioned to provide end-user messaging.


In class B, I don't understand global declarations throughout. Using global is generally a PHP coding anti-pattern that should be removed from the code. It seems as if the intent here is to access the class properties, which should be done using $this->*.


Class B looks like it wants to grow into a database wrapper, which to me is a bit of an anti-pattern unless that wrapper adds some specific value like:

  • managing connections (like using singleton or factory patterns)
  • abstracting user from specific database implementation details (PDO itself is already such an abstraction)
  • providing abstract ORM/model behaviors to be implemented in object-specific inheriting classes (it seems like you might be trying to achieve this in this class though I don't know why specific class/table logic like you have in updateStock() would be in a generalized database class)
  • providing natural language query-building functionality

My suggestion would be to abandon such an approach in favor of writing individual classes for your database entities such as artikelen, or better yet using a proper PHP ORM.


Never hard code database or other security credentials into your code. These should be derived from configuration that lies outside your code repository (ideally derived from environmental configuration).

You should change your DB password NOW as you just posted all your credentials for the whole world to see.


You have unreachable lines of code in updateStock(). When return is called, any lines of code after that call in that code path will never be reached.

You should never have unreachable lines in your code.


In class B, you should be using parametrized prepared statements vs. concatenating string values into your query strings. You are totally open to SQL injection as you are doing nothing to sanitize your parameters for use in the queries.


The public methods in class B that accept arguments have no guarding code to validate those arguments. You should, at a minimum make sure you have reasonable values (non-zero length strings, integer values, etc.) before getting to the point of making the (relatively) expensive query against the database.

Ideally you would throw InvalidArgumentException if parameters don't meet basic validation criteria. This lets calling code know it did something wrong.


There is no reason to ever perform two queries in updateStock(). The first select is just a wasteful query against the database and it seems your code just assumes the record will exist anyway. You can go directly to trying the update (after validating and sanitizing input of course), setting the new values based on the old value.

That query might look like the following (of course you should parametrize this):

UPDATE `artikelen`
SET  `stock` = IF(`stock` > $amountToDecline, `stock` - $amountToDecline, 0)
WHERE  `artikelen`.`artnr` = $artnr

You can look at the number of affected rows to determine if the record was found or not (should should get 0 if nothing was done and 1 if an update was performed), alleviating the need to perform an upfront select at all.


It seems clear that you two are not working using a common coding standard or styleguide. If you are going to collaborate, you should agree on this so that you don't end up with poorly written code that varies from script to script and class to class. I would recommend looking at the PSR standards, particularly PSR-1 and PSR-2 as a starting point.

http://www.php-fig.org/psr/

You should enforce whatever style you decide upon using a style checker before committing code to your repository.

I would also suggest that you both familiarize yourself with http://www.phptherightway.com/ as a reference you can use to help guide your development.

\$\endgroup\$
1
  • \$\begingroup\$ Thank you for your information as on how to better our programming, i really appreciate it! \$\endgroup\$
    – ThatsAudio
    Feb 9, 2017 at 7:49

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.