3
\$\begingroup\$

I have been writing PHP for a number of years but in a procedural style. I have always shied away from object oriented PHP because I just didn't understand it. However now I'm back for another go.

I have a large web application which is written in procedural style PHP and as a learning exercise I would like to re-write it entirely in object-oriented PHP.

So before I start coding away thinking I'm doing the right thing, I would like to help, tips, and advice on what I have written so far. I don't think I need to explain what I am doing here because it is pretty simple stuff and self-explanatory.

pdo.inc.php

<?php

$host = "localhost";
$db = "dev";
$user = "dev";
$pass = "dev";
$dsn = "mysql:host=$host;dbname=$db;";

try
{
    $pdo = new PDO($dsn, $user, $pass);
} catch (PDOException $ex) {
    echo 'Connection failed: ' . $ex->getMessage();
    die();
}

?>

customer.class.php

<?php

include_once('pdo.inc.php');

class Customers
{
    function GetCustomerName($pdo, $customer_id)
    {
        $query = "SELECT first_name FROM Customers WHERE customer_id = :customer_id LIMIT 1";
        $stmt = $pdo->prepare($query);
        $stmt->bindParam(':customer_id', $customer_id);
        $stmt->execute();
        return $stmt->fetchColumn();
    }
}

?>

test.php

<?php

include_once('customers.class.php');

$a = new Customers();

echo $a->GetCustomerName($pdo, 1);

?>

Having looked at a number of PHP OOP tutorials it seems people use try/catch differently. I have seen some people use the try/catch within the class's method, and some people call the method from within the try/catch. What's the recommended usage of try/catch?

\$\endgroup\$
  • \$\begingroup\$ Your question at the end of the post looks more like a fit for Programmers.SE. Afar from that: Welcome to Code Review! \$\endgroup\$ – Mast Jun 8 '16 at 18:34
  • \$\begingroup\$ I have rolled back the last edit. Please see What to do when someone answers. \$\endgroup\$ – Mast Jun 9 '16 at 10:16
1
\$\begingroup\$

One thing you are doing well:

  • Dependency injection - I like how you pass the PDO object to the method that needs to use it. Since that is not an abstract function however, I wonder if you should be passing the PDO object to the class upon instantiation in the constructor (that is of course if the class needs a valid PDO object for other methods as well).

Some things I would suggest

  • TheGetCustomerName() method only considers the happy path with regards to the query. What happens if prepare() fails? What happens if execute() fails? What happens if query operates correctly but no query match is found (i.e. id doesn't exist in table)? You should be handling these edge cases and either logging errors and/or handling exceptions if using PDO in exception mode (which it looks like you are). You might consider throwing or rethrowing an Exception from this method itself if something fails. You should consider returning null or false from this method if something fails and/or the SQL lookup is not successful.

  • You should consider enforcing types in you method signature.

Perhaps something like:

function GetCustomerName(PDO $pdo, $customer_id) {

or in PHP7 (which supports type hinting for scalars) and assuming $customer_id need to be an integer value:

function GetCustomerName(PDO $pdo, int $customer_id) {

This will allow you to enforce that a valid PDO instance is passed to the method, otherwise an InvalidArgumentException will be thrown. This prevents you from having to do something like if ($pdo instanceof PDO) in your method.

  • Related to the above item. Right now your method assumes all passed parameters are valid. You should always assume you are getting passed bad data and make sure you have appropriate parameters passed to the method before doing any other work in the method. I would enforce PDO in signature as noted above, and if not using PHP 7 at least add a line like the following.

Example:

if (empty($customer_id) || !is_int($customer_id) || $customer_id < 1) {
    throw new InvalidArgumentException(
        'Send me a valid positive integer for $customer_id' .
        ' Value sent: ' . var_export($customer_id, true)
    );
}
  • You do not need an include for PDO code in your customer class. You are taking on the PDO object as a dependency (which is a good example of dependency injection). The customer class code therefore doesn't need to reference the code where the PDO object is created. It just needs to know it has a valid PDO object passed to it, not reference the code where such objects are created. You should reference your include_once('pdo.inc.php'); line in test.php where the dependent PDO object is actually instantiated.

  • You should consider following typical PHP syntax schemes. Typically in PHP you would NEVER see a function or method name that starts with a capital letter. This syntax is typically reserved for classes and interfaces. I would consider renaming to getCustomerName().

  • I would consider separating your configuration from the place where you instantiate the PDO objects. If you ever have the need to instantiate multiple database connections, you would want to separate these. Oftentimes it is best to use constants to define these sorts of configuration values such that you don't tend to overwrite them and can see directly in code that these should be constant values.

  • You should consider changing the class name to Customer instead of Customers if a single class instance is expected to only deal with a single customer vs. a collection of customers. This may seem nitpicky, but it is typically best practice for developers to use meaningful names for items in code. If Customer more correctly describes what the object represents than Customers, the singular version should be used.

You also ask about try-catch usage. Generally you should use a try-catch block around any lines of code that you think might throw an exception and you want to be able to handle that exception in the code such that code execution will not terminate due to the uncaught exception. For example in you method. You should, at a minimum, have a try-catch block around the prepare(), bindParam()and execute() calls if you are using PDO in exception mode (which I would recommend). You could have one block around all this or multiple blocks depending on how granularity you want to handle the failure.

In test.php for example you would want to place a try-catch around the call to $a->GetCustomerName($pdo, 1); As this call could throw an exception (particularly if you added some of the parameter handling logic I mentioned earlier).

So for me, a rewrite of your code might look like:

pdoConfig.inc.php

<?php
define('DB_CONFIG_HOST', 'localhost');
define('DB_CONFIG_DB', 'dev');
define('DB_CONFIG_USER', 'dev');
define('DB_CONFIG_PW', 'dev');
$dsn = 'mysql:host=' . DB_CONFIG_HOST . ';dbname=' . DB_CONFIG_DB . ';';
define('DB_CONFIG_DSN', $dsn);l
?>

customer.class.php

<?php    
class Customer
{
    function getCustomerName(PDO $pdo, $customer_id)
    {
        // validate parameters
        // in PHP 7 you would just add int type hinting in method signature       
        // and could change the conditional below to only look
        // positive integer values assuming the id is > 0
        if (empty($customer_id) || !is_int($customer_id) || $customer_id < 1) {
            throw new InvalidArgumentException(
                'Send me a valid positive integer for $customer_id' .
                ' Value sent: ' . var_export($customer_id, true)
            );
        }

        // query DB for customer name
        $query = "SELECT first_name
                  FROM Customers
                  WHERE customer_id = :customer_id
                  LIMIT 1";
        try {                
            $stmt = $pdo->prepare($query);
            $stmt->bindParam(':customer_id', $customer_id);
            $stmt->execute();
            if($stmt->rowCount === 0) {
                // no match found
                return false;
            } else {
                return $stmt->fetchColumn();
            }
        } catch (PDOException $e) {
            error_log(
                'Something went wrong with query. Message: ' .
                $e->getMessage();
            );
            // you could also rethrow exception here instead of returning null
            // if you want to abort method completion and have caller handle exception.
            // or of course not use try-catch block here at all
            // if you don't want any extra exception handling in this method.
            return null;
        }
    }
}

?>

test.php

<?php


include_once('pdoConfig.inc.php');
include_once('customers.class.php');

// instantiate PDO object
try
{
    $pdo = new PDO(DB_CONFIG_DSN, DB_CONFIG_USER, DB_CONFIG_PW);
} catch (PDOException $e) {
    error_log('Connection failed with: ' . $e->getMessage());
    die();
}

// instantiate customer object
// you might want to include in try-catch if instantiation is expected
// to potentially throw exceptions
$a = new Customer();

try {
    echo $a->getCustomerName($pdo, 1);
} catch (Exception $e) {
    error_log('Customer name lookup failed with: ' . $e->getMessage());
}   
?>
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ thanks for taking the time to provide some very helpful feedback. \$\endgroup\$ – AndyH Jun 8 '16 at 20:08
  • \$\begingroup\$ Using the example above, if I wanted to echo the customer's name to the page, would I directly use this try/catch where I wanted the customer's name to be displayed, or, would I try to assign $name = $a->getCustomerName($pdo, 1), then just display $name where the customer's name should go? Obviously I will have a lot more options than just the customers name and I don't want to end up with 1000's of lines of code which are try/catch. \$\endgroup\$ – AndyH Jun 8 '16 at 20:30
  • \$\begingroup\$ @AndyH If you know you are going to be reusing the customer's name throughout an application, you would, at a minimum, want to place that value into an variable so that you are not querying the database everytime you need it. Better yet, would be to continue to flesh out your Customer class such that you can for example pass customer id to constructor and that populate object with all pertinent info from database (customer name, email address, etc.) so that you would then pass the customer object around in your application to get information about the customer. \$\endgroup\$ – Mike Brant Jun 8 '16 at 21:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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