A few questions on Coding practises and generalised php coding

Hi guys, I’ve dabbled with php for couple of years and used it to do what I needed, Since PDO came out i’ve been teaching myself how to use it, but I’ve also been trying to figure out the right coding practises on building these.

Could I ask someone to look through this code, it does what it needs to do, but it could be better and I want to no how I can make it better.

Thanks in advance.

[php]

<?php try{ $host = "localhost"; $dbname = "dbname"; $user = "user"; $pass = "pass"; $errMsg = "error message"; $odb = new PDO("mysql:host=" . $host . ";dbname=" . $dbname, $user, $pass); $odb->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION); } catch (PDOException $e) { echo "I'm afraid I can't do that."; file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND); } if($_SERVER["REQUEST_METHOD"] == "POST"){ if (isset($_POST['firstname']) && !empty($_POST['lastname']) && filter_var($_POST['email'],FILTER_VALIDATE_EMAIL) && !empty($_POST['phone']) && !empty($_POST['address']){ $firstname = $_POST['firstname']; $lastname = $_POST['lastname']; $email = $_POST['email']; $phone = $_POST['phone']; $q = "INSERT INTO jobform(firstname, lastname, email, phone) VALUES (:firstname, :lastname, :email, :phone, :address);"; $query = $odb->prepare($q); $results = $query->execute(array( ":firstname" => $firstname, ":lastname" => $lastname, ":email" => $email, ":phone" => $phone, )); } else { #echo $errMsg; } } ?>

[/php]

Your PDO usage is fine (ish).

However, your code structure is a mess. The most obvious thing is that jobs SQL request. Surely you’ll have multiple entry points to insert a new job to your database. Imagine that your query string changes - do you really want to have to go through your code and replace every occurence + every single SELECT that either looks directly at it or JOINs it?

Secondly, you want to use a way to pass $odbc around, preferably one that does not rely on a global variable (so it cannot be overwritten by accident).

Thirdly, you’re falling victim to namespace pollution. All your variables are global - this is a sign of failure in scoping.

Here is a re-written version of your code, which might make more sense:

DB class
[php]<?php
class DB {
private static $instance;
protected $PDO;
private static $username = “user”;
private static $password = “pass”;
private static $db = “dbname”;
private function __construct() {
$this->PDO = new PDO(“mysql:host=localhost;dbname=”.static::$db,static::$username,static::$password);
// Blah
}
public static function config(stdClass $cfg) {
foreach ($cfg as $k => $v) {
if (isset(static::${$k})) static::${$k} = $v;
}
}
public static function getInstance() {
if (!static::$instance) static::$instance = new self();
return static::$instance;
}
public function run($q) {
$r = $this->PDO->prepare($q);
// This function allows you to also deal with performance tracking, logging etc.
return $r;
}
}
[/php]

Jobforms model
[php]<?php
class Jobforms {
public static function find($id) {
$DB = DB::getInstance();
$q = $DB->run(“SELECT * FROM jobform WHERE id=:id”);
$r = $q->execute(array(":id" => $id));
return $r;
}
public static function create(stdClass $params) {
$DB = DB::getInstance();
$q = $DB->run(“INSERT INTO jobform (firstname, lastname, email, phone) VALUES (:fn, :ln, :email, :phone)”);
foreach ($params as $k => $v) {
$q->bindValue($k,$v);
}
return $q->execute();
}
}[/php]

This allows you to condense your code down to:
[php]<?php
if ($_SERVER[‘REQUEST_METHOD’] !== “POST”) throw new Exception(“Wrong request type”);
if (!isset($_POST[‘firname’) || !isset($_POST[‘lastname’]) || !isset($_POST[‘email’]) || !isset($_POST[‘phone’])) throw new Exception(“Some fields were not found”);
if (!filter_var($_POST[‘email’], FILTER_VALIDATE_EMAIL)) throw new Exception(“Incorrect email”);

$v = new stdClass();
$v[":fn"] = $_POST[‘firstname’];
$v[":ln"] = $_POST[‘lastname’];
// More assignments

Jobforms::create($v);[/php]

Which is not only a lot neater, but also a lot more reusable. Note that it needs a try/catch block.

What I have just pointed you towards is called ORM, by the way. It stands for Object-Relations Mapping, and it is commonly referred to as using “models”. A model is an object instance that represents a set of related data entries of a certain type in your database. Some frameworks and libraries do this exceedingly well - look up Doctrine!

Yea i apologise for the mess, the data is passed on from a form to this file to achieve the neccisary data.

This is actually what i’ve been wanting to structure my code like. Thanks alot for this it’s really helpful, although i’m not used to classes, would i set it these classes and models up as seperate files?

Such as the DB Class would it end up being DB.php with jorbforms model being a second .php file?

I’m just a little confused on how it would fit together.

Where you put your files is up to you. Provided that they are loaded, the actual location does not matter much provided that you know where they are.

A good thing to do would be to pick up a simple framework and take it apart. Kohana has a seriously good ORM implementation - pick it apart for a good example of what to do? :slight_smile:

Thanks for getting back to me pretty quickly.

I’m taking a look at Kohana now, i’ll get back to you on if I finid it useful.

I don’t suppose I could get you to create a very basic connection class that outputs a record could I? I was trying to get your connection class to work by calling the functions although it didn’t seem to work!

It’s just once i can see the connection class working i can probably pick it all apart!

Re-using the DB class from earlier

[php]

<?php class DBModel { private $DBI; private $fields; protected $id; public function __construct() { $this->DBI = DB::getInstance(); $fieldsRef = new ReflectionObject($this); $fieldList = $fieldsRef->getProperties(ReflectionProperty::IS_PUBLIC && ~ReflectionProperty::IS_STATIC); foreach ($fieldList as $v) { $this->fields[] = $v->name; } } public static function get($id) { $q = $this->DBI->run("SELECT * FROM ".$this->table." WHERE id=:id"); $q->bindValue(":id",(int)$id); $q->execute(); if ($r = $q->fetch()) { $rtn = new self(); foreach ($r as $k => $v) { $rtn->$k = $v; } } return $rtn; } public function save() { // You can implement this one! } } class Jobform extends DBModel { public $table = "jobforms"; public $firstname; public $lastname; public $phone; public $email; }[/php] This neatly wraps everything into something called a model. The syntax is borrowed from both Kohana and Laravel3.
Sponsor our Newsletter | Privacy Policy | Terms of Service