current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 6 down vote favorite

I'm trying to get familiar with database handling. Can you point out my errors and what I should change?

I want to learn new methods, but just don't want to learn it the wrong way.

I do realize these things below, yet I wanted to make the code more clear:

  • I need to bind values for insert()
  • Use try/catch for error handling
  • Password hashing (did not use it to make example simpler)

<?php

class Query 
{
    private $_sql;
    private $_sth;
    private $_db;

    public function __construct() 
    {
        $this->_db = new PDO('mysql:host=localhost;dbname=mvc;', 'root', '');
        $this->_db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
        $this->_db->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
    }

    public function select($arg)
    {
        $this->_sql .= "SELECT {$arg}";
        return $this;
    }

    public function from($arg)
    {
        $this->_sql .= " FROM {$arg}";
        return $this;
    } 

    public function insert($arg)
    {
        $this->_sql .= "INSERT INTO {$arg}";
        return $this;        
    }

    public function columns($arg)
    {
        $this->_sql .= " ({$arg})";
        return $this;            
    }

    public function values($arg)
    {
        $this->_sql .= " VALUES ({$arg})";
        return $this;            
    }

    public function execute($data = null)
    {
        $this->_sth = $this->_db->prepare($this->_sql);
        $this->_sth->execute($data);
        $this->_sql = null;
        return $this;
    }

    public function fetch()
    {
        return $this->_sth->fetchAll();
    }

    public function getSql()
    {
        return $this->_sql;
    }
}


$query = new Query;

// inserts into database
$query->insert('users')
        ->columns('`username`,`password`')
        ->values('"test","tester"')
        ->execute();

// returns array of users
$query->select('username')
        ->from('users')
        ->execute()
        ->fetch()

?>
share|improve this question
3  
I highly suggest you learn how other DBALs function, such as Doctrine. This will help you structure yours, and figure out exactly how large of a project you're taking on! –  Alex L Jul 29 '14 at 18:07
    
Thank you, i will look into it! –  SinisterGlitch Jul 30 '14 at 15:01

2 Answers 2

up vote 3 down vote accepted

Dependency Injection

You might want to utilize DI to get low coupling between your class and it's dependencies. If you ever have to use a different PDO object you then don't have to modify your class. It also makes unit testing much easier if you do test-driven development.

Don't forget to disable emulated prepared statements as a security convention against malicious SQL injections

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

    $pdo->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
    $pdo->setAttribute(PDO::ATTR_EMULATE_PREPARES, false);
    $pdo->setAttribute(PDO::ATTR_DEFAULT_FETCH_MODE, PDO::FETCH_ASSOC);
}

And as pointed out by @bad_boy, to do it even better you could also remove the preparation of the PDO object outside of this class to make sure it abides the Single Responsibility Principle and you would be left with this:

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

Saving Statement Handlers

Saving state to use it at some point later is prone to errors. When your class gets bigger with methods that do the same (saving it), chances are great the wrong statement handler will be used at some point. E.g. you would have a bunch of queries going on, but when using Query::fetch() it would fetch from only the most recently saved statement handler. Unless you intent to have different Query objects for each database query, which would be inefficient.

Naming

Why is a() doing b()?

fetch() is (only) encapsulating the PDO::fetchAll() method. A method named fetch() usually implies that it fetches only one row. Yours is in fact fetching every row. I would rather name it fetchAll() as well. It's not good to name a method a() when it actually does b(). This could get ugly real fast if let's say you work with sensitive money balance data where you only meant to send a dollar instead of the entire balance.

Better Class Names

If you look at your class name Query, what does it tell you about itself? Yes, that it's some sort of query class, but it's not specific enough. You should name your variables, methods and class names in such a way they are that self-explanatory to some degree. Right now you would actually have to view the body of the class to comprehend what it's doing. I would maybe call it something like MySqlPDO which already tells me more about the class then Query.

Wrapping Up

Based on what you have, I would probably make a class SqlQueryBuilder that has no outside dependencies. Use that to build SQL statements via methods to hide the actual SQL syntax, then use the returned syntax for whatever (e.g. executing a database query with PDO).

More often then not it's better to use the native PDO class itself to do your database stuff, then to actually use a class that just wraps around the PDO object with no significant benefits.

share|improve this answer
    
This is actually not quite correct. The instance of $pdo should be already prepared and configured and then injected only. The point of SRP is separating responsibilities. Object preparing must be another responsibility –  bad_boy Aug 26 '14 at 15:54
up vote 5 down vote

Passwords as plain text!?

If I were using an application or website that stored my password as plain text where anyone with access to the DB seeing this would make me feel very scared:

// inserts into database
$query->insert('users')
        ->columns('`username`,`password`')
        ->values('"test","tester"')
        ->execute();

For the love of your users, please consider password hashing!

Password hashing is one of the most basic security considerations that must be made when designing any application that accepts passwords from users. Without hashing, any passwords that are stored in your application's database can be stolen if the database is compromised, and then immediately used to compromise not only your application, but also the accounts of your users on other services, if they do not use unique passwords.

Redundancy

This:

public function insert($arg)
{
    $this->_sql .= "INSERT INTO {$arg}";
    return $this;        
}

public function columns($arg)
{
    $this->_sql .= " ({$arg})";
    return $this;            
}

public function values($arg)
{
    $this->_sql .= " VALUES ({$arg})";
    return $this;            
}

And this:

// inserts into database
$query->insert('users')
        ->columns('`username`,`password`')
        ->values('"test","tester"')
        ->execute();

That seems redundant, unless you plan on inserting user names and passwords in different tables/columns. Why not shorten it like this:

public function insert($arg)
{
    $this->_sql .= "INSERT INTO users (`username`, `password`) VALUES ({$arg})";
    return $this;        
}
share|improve this answer
    
Thank you for giving me feedback about how i need to query. my thought was to make building blocks wich can be any query at any time. maybe im not thinking this clearly and more straight to the point queries are better. - about the hashing point. this example is local only for testing. I always use hash in practise! –  SinisterGlitch Aug 26 '14 at 7:56

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.