Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I wrote this class to make it reusable in any project. It uses PDO as the MySQL connection. Beside not using Setters and Getters, is this code secure and good practice for CRUD ? Thanks for further replies.

class ContactsRow
{
const PDO_CHARSET = PDO_CHARSET;
const PDO_DBNAME = PDO_DBNAME;
const PDO_DRIVER = PDO_DRIVER;
const PDO_HOSTNAME = PDO_HOSTNAME;
const PDO_PASSWORD = PDO_PASSWORD;
const PDO_USERNAME = PDO_USERNAME;

const CURRENT_TIMESTAMP = CURRENT_TIMESTAMP;
const PHP_AUTH_USER = PHP_AUTH_USER;

protected $id = null;
public $name = null;
public $email = null;
public $filename = null;
public $created_by = self::PHP_AUTH_USER;
public $modified_by = self::PHP_AUTH_USER;
public $date_created = null;
public $date_modified = null;
public $published = 1;
public $publish_up = null;
public $publish_down = null;
public $ordering = null;

/**
 * Getters for Protected Vars
**/
public function getId()
{
    return (int)$this->id;
}

/**
 * Save
 * @return boolean $query
 */
public function save()
{
    if(! (isset($_SESSION['errors']) AND !empty($_SESSION['errors']))){
        $db = new PDO(self::PDO_DRIVER . ':host=' . self::PDO_HOSTNAME . ';dbname=' . self::PDO_DBNAME, self::PDO_USERNAME, self::PDO_PASSWORD, array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES ' . self::POD_CHARSET));
        $values = array(
            ':name' => $this->id_parent,
            ':email' => $this->email,
            ':filename' => $this->filename,
            ':created_by' => self::PHP_AUTH_USER,
            ':modified_by' => $this->modified_by,
            ':date_created' => $this->date_created,
            ':date_modified' => self::CURRENT_TIMESTAMP,
            ':published' => $this->published,
            ':publish_up' => $this->publish_up,
            ':publish_down' => $this->publish_down,
            ':ordering' => $this->ordering,
            ':id' => $this->id
        );
        if((int)$this->id > 0){
            $sql = '
                UPDATE contacts
                SET
                    name = :name,
                    email = :email,
                    filename = :filename,
                    created_by = :created_by,
                    modified_by = :modified_by,
                    date_created = :date_created,
                    date_modified = :date_modified,
                    published = :published,
                    publish_up = :publish_up,
                    publish_down = :publish_down,
                    ordering = :ordering
                WHERE 1
                AND id = :id
                LIMIT 1
            ';
            $stmt = $db->prepare($sql);
            $query = $stmt->execute($values);
        }else{
            $sql = '
                INSERT INTO contacts
                SET
                    name = :name,
                    email = :email,
                    filename = :filename,
                    created_by = :created_by,
                    modified_by = :modified_by,
                    date_created = :date_created,
                    date_modified = :date_modified,
                    published = :published,
                    publish_up = :publish_up,
                    publish_down = :publish_down,
                    ordering = :ordering,
                    id = :id
            ';
            $stmt = $db->prepare($sql);
            $query = $stmt->execute($values);
            $this->id = $db->lastInsertId();
        }
        $db = null;
        return (bool)$query;
    }
    return false;
}

/**
 * Delete
 * @return boolean $query
 */
public function delete()
{
    if((int)$this->id > 0){
        $db = new PDO(self::PDO_DRIVER . ':host=' . self::PDO_HOSTNAME . ';dbname=' . self::PDO_DBNAME, self::PDO_USERNAME, self::PDO_PASSWORD, array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES ' . self::POD_CHARSET));
        $sql = '
            DELETE t, tt 
            FROM contacts AS t
            WHERE 1
            AND t.id = :id
        ';
        $stmt = $db->prepare($sql);
        $query = $stmt->execute(array(
            ':id' => $this->id
        ));
        $db = null;
        return (bool)$query;
    }
    return false;
}


/**
 * fetchRow
 * @return $this
 */
public function fetchRow($id = null)
{
    $db = new PDO(self::PDO_DRIVER . ':host=' . self::PDO_HOSTNAME . ';dbname=' . self::PDO_DBNAME, self::PDO_USERNAME, self::PDO_PASSWORD, array(PDO::MYSQL_ATTR_INIT_COMMAND => 'SET NAMES ' . self::POD_CHARSET));
    $sql = '
        SELECT * 
        FROM contacts
        WHERE 1
        AND id = :id 
        LIMIT 1
    ';
    $stmt = $db->prepare($sql);
    $stmt->execute(array(
        ':id' => (int)$id
    ));
    if($row = $stmt->fetch(PDO::FETCH_OBJ)){
        foreach($row as $i => $e){
            if($e != "") $this->{$i} = $e;
        }
    }
    $db = null;
    return $this;
}
}
share|improve this question
add comment

1 Answer

up vote 1 down vote accepted

The main security concern here I guess is protection from sql injection. Good news. You got that part right by using bound parameters.

Here is what you should improve and why:

Make the name of the class match what it is. In this case, it represents 1 Contact. So, name it Contact.

You said "besides no use of getters and setters". However, I'll point out that the reason you should use them is to provide a stable api to your calling code and to encapsulate future behavior. There are more reasons, but those are important ones.

Inject an instance of PDO instead of creating it all over the place. For one, all those connections will kill your performance. And, it will separate the concern of making the connection from this class - making it simpler.

public function __construct(PDO $pdo)
{
    $this->pdo = $pdo;
}

You will likely have other kinds of classes that represent different kinds of rows. Move the common behavior (find, save, delete) into a parent class.

Create proper doc blocks for your methods:

/**
 * @param int|string $id
 * @return null|Contract
 */
public function fetchRow($id)

Add proper error handling for PDO. You can configure PDO to throw exceptions. Or, if not, you should do checks like:

$stmt = $this->pdo->prepare(...);
if (! $stmt) {
    //handle this error situation
}

Remove direct access to $_SESSION. This kind of global access is very difficult to maintain and test in the long run.

Now, having said all this, your approach here (which is sort of an active record pattern) has been done many times over. Check out a library such as php active record and you will save yourself countless hours and hassle.

share|improve this answer
    
Hi! I can't vote but thanks for the reply. Very Very Appreciated. –  SequenceDigitale Jan 25 '13 at 17:17
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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