AndrewPHP Posted February 10, 2016 Share Posted February 10, 2016 After writing procedural style PHP for many years I've decided to give OOP & PDO a go (and I'm hating it already). I have written a simple function which validate a flag value, and another function which is later used to get the description of that value. Am I on the right lines here and can any improvements be made? <?php // Connect to MySQL $dsn = 'mysql:dbname=dev_pof;host=127.0.0.1'; $username = 'dev'; $password = 'dev.dev'; $pdo = new PDO($dsn, $username, $password,[PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]); // Function to validate the flag value function CheckFlagValue($pdo, $FlagName, $FlagValue) { $sql = "SELECT 1 FROM flags WHERE flagname = ? AND flagvalue = ? LIMIT 1"; $stmt = $pdo->prepare($sql); $stmt->execute([$FlagName, $FlagValue]); return $stmt->fetchColumn(); } // Function to get the flag description function GetFlagDescription($pdo, $FlagName, $FlagValue) { $sql = "SELECT flagdescription FROM flags WHERE flagname = ? AND flagvalue = ? LIMIT 1"; $stmt = $pdo->prepare($sql); $stmt->execute([$FlagName, $FlagValue]); return $stmt->fetchColumn(); } // Example of using the CheckFlagValue function if (CheckFlagValue($pdo, 'intent', 2)) { echo "valid"; } else { echo "invalid"; } ?> Quote Link to comment Share on other sites More sharing options...
requinix Posted February 10, 2016 Share Posted February 10, 2016 So, this isn't really OOP... It's still procedural code like before, except you're using functions instead of having to copy/paste code around. An OOP version would be like class Flag { private $FlagName = ""; private $pdo = null; public function __construct($pdo, $FlagName) { $this->FlagName = $FlagName; $this->pdo = $pdo; } public function CheckValue($FlagValue) { $sql = "SELECT 1 FROM flags WHERE flagname = ? AND flagvalue = ? LIMIT 1"; $stmt = $this->pdo->prepare($sql); $stmt->execute([$this->FlagName, $FlagValue]); return $stmt->fetchColumn(); } public function GetDescription($FlagValue) { $sql = "SELECT flagdescription FROM flags WHERE flagname = ? AND flagvalue = ? LIMIT 1"; $stmt = $this->pdo->prepare($sql); $stmt->execute([$this->FlagName, $FlagValue]); return $stmt->fetchColumn(); } } $pdo = new PDO($dsn, $username, $password, [PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION]); $intent = new Flag($pdo, 'intent'); if ($intent->CheckValue(2)) { echo "valid"; } else { echo "invalid"; }1. It could be that "a flag" is not just a name but a name/value pair, in which case you'd set up the name and value with the Flag class and then not have to pass the value into the CheckValue and GetDescription methods.2. If you did that name/value pair thing, better than querying the database in each method would be to simply load up all data when the class gets constructed - then CheckValue and GetDescription could just return values already available. Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted February 10, 2016 Share Posted February 10, 2016 The PDO code can also be improved a bit: Always set PDO::ATTR_EMULATE_PREPARES to false. Otherwise all prepared statements are silently replaced with a kind of client-side auto-escaping, which is a lot less secure. You should explicitly define the desired character encoding in the DSN string (e. g. charset=utf8mb4). Otherwise MySQL will fall back to a default encoding and may garble non-ASCII characters. Also consider setting the default fetch mode. PDO supports named parameters in prepared statements. Use them! It's a lot more readable than a bunch of question marks. When you connect to 127.0.0.1, you force MySQL to use the TCP protocol. It's generally more efficient to use a UNIX socket by connecting to localhost. <?php const DB_HOST = 'localhost'; const DB_USER = '...'; const DB_PASSWORD = '...'; const DB_NAME = '...'; const DB_CHARSET = 'utf8mb4'; // just an example; choose whatever you need $dSN = 'mysql:host='.DB_HOST.';dbname='.DB_NAME.';charset='.DB_CHARSET; $databaseConnection = new PDO($dSN, DB_USER, DB_PASSWORD, [ PDO::ATTR_EMULATE_PREPARES => false, PDO::ATTR_ERRMODE => PDO::ERRMODE_EXCEPTION, PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC, ]); Quote Link to comment Share on other sites More sharing options...
AndrewPHP Posted February 10, 2016 Author Share Posted February 10, 2016 Thanks for that, it looks like I should have stuck to the procedural side of things as this looks incredibly more complex. I also need to create some other functions too but unsure how they would break down by class. CheckIfEnabled - This function checks in the DB if this script is allowed to be run. CheckIfTime - This function checks in the DB if this script is allowed to be run at this current minute. CheckMessagesSent - This function checks in the DB to see how many messages have been sent already today CheckMessageLimit - This function checks in the DB to see how many messages are allowed to be sent. Would I create a new class for each function? Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted February 10, 2016 Share Posted February 10, 2016 No, because then all you get is your old procedural code with a lot of useless OOP overhead. OOP is about objects, not functions. Objects are actors which have attributes and can perform tasks. In your case, an obvious actor would be the current script: It has a path (or some other unique identifier) and a database connection, and it can tell you whether you're allowed to run it. For example: <?php class RestrictedScript { private $path; private $database; public function __construct($path, PDO $database) { $this->path = $path; $this->database = $database; } public function isEnabled() { // query the database } public function isRunnable() { // query the database } } <?php $thisScript = new RestrictedScript(__FILE__, $database); if (!$thisScript->isEnabled()) { echo 'Script is not enabled.'; exit; } if (!$thisScript->isRunnable()) { echo 'Script may not be executed right now. Please try again later.'; exit; } You should, of course, choose a more meaningful name than “RestrictedScript”. Quote Link to comment Share on other sites More sharing options...
AndrewPHP Posted February 10, 2016 Author Share Posted February 10, 2016 Great stuff, I think I'm slowly taking it in (I hope), so how's this... <?php class ScriptControl { private $pdo; private $ScriptName; public function __construct($pdo, $ScriptName) { $this->pdo = $pdo; $this->ScriptName = $ScriptName; } public function IsEnabled(); { $sql = "SELECT 1 FROM config WHERE config_name = ? AND config_value = 1 LIMIT 1"; $stmt = $this->pdo->prepare($sql); $stmt->execute([$this->ScriptName]); return $stmt->fetchColumn(); } public function IsRunnable(); { date_default_timezone_set("Europe/London"); $time = date("Hi"); $sql = "SELECT 1 FROM config WHERE config_name = ? AND config_value = ? LIMIT 1"; $stmt = $this->pdo->prepare($sql); $stmt->execute([$this->ScriptName, $this->$time]); return $stmt->fetchColumn(); } } ?> <?php include_once('PDO.class.php'); include_once('ScriptControl.class.php'); $ThisScript = new ScriptControl('SendMessage'); if ( (!$ThisScript->isEnabled()) && (!isset($_POST['bypass'])) ) { echo "<p>This script is not enabled</p>\n"; die(); } if ( (!$ThisScript->isRunable()) && (!isset($_POST['bypass'])) ) { echo "<p>This script is not scheduled to run</p>\n"; die(); } ?> Quote Link to comment Share on other sites More sharing options...
Jacques1 Posted February 11, 2016 Share Posted February 11, 2016 The OOP part is OK (except that you're not passing the PDO instance to the ScriptControl constructor in your example code). The database layout looks odd. What's the type of config_value? Why does the column simultaneously contain integers(?) and time values? Also, you can get the current time in MySQL itself using CURTIME(). There's no need to do that in PHP (unless you somehow want different timezones in MySQL and PHP). Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.