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());
}
?>
