1
\$\begingroup\$

I am creating a CMS for a website, so I've added most of the functions that will fetch data from the database to a separate class. The code doesn't seem to have any bugs(Since I've tested it) but I don't like the way how it's looking, I feel like it can be improved but I don't know how. The other concern I have is whether using object orientation will be in this case better (speed etc) than simple functional way.

    <?php

class getResources{

    private $rowId;
    private $tableName;
    private $rowRes;
    private $limInit, $limEnd;
    private $direction;

    private function sqlFetcher(){

        $selectPosts = "select * from {$this->tableName} where post_id='{$this->rowId}'";

        $run_posts = mysql_query($selectPosts);
        if(!$run){
           die('Could not retreive data: "<em>'.mysql_error().'"</em>');
       }
        $row = mysql_fetch_array($run_posts);

        return $row;      
    }

    private function fetchAllData(){
       $query = "SELECT * FROM {$this->tableName} ORDER BY {$this->rowRes} {$this->direction} LIMIT {$this->limInit},{$this->limEnd}";
       $run = mysql_query($query);
       if(!$run){
           die('Could not retreive data: "<em>'.mysql_error().'"</em>');
       }

       return $run;
    }
    /**************sqlFetcher Getters****************/
    function getContent($rowId, $tableName){
        $this->rowId = $rowId;
        $this->tableName = $tableName;

        $post_content = $this->sqlFetcher();

        return $post_content['post_content'];
    }

    function getTitle($rowId, $tableName){
        $this->rowId = $rowId;
        $this->tableName = $tableName;

        $post_title = $this->sqlFetcher();

        return $post_title['post_title'];
    }

    function getAuthor($rowId, $tableName){
        $this->rowId = $rowId;
        $this->tableName = $tableName;

        $post_author = $this->sqlFetcher();

        return $post_author['post_author'];
    }

    function getKeywords($rowId, $tableName){
        $this->rowId = $rowId;
        $this->tableName = $tableName;

        $post_keywords = $this->sqlFetcher();

        return $post_keywords['post_keywords'];
    }

    function getPicture($rowId, $tableName){
        $this->rowId = $rowId;
        $this->tableName = $tableName;

        $post_picture = $this->sqlFetcher();

        return $post_picture['post_picture'];
    }


    function getDate($rowId, $tableName){
        $this->rowId = $rowId;
        $this->tableName = $tableName;

        $post_date = $this->sqlFetcher();

        return $post_date['post_date'];
    }

    /*****************fetchAllData Getter********************/

    function getFetchedData($tb, $rR, $lI, $lE, $direction="ASC"){
        $this->tableName = $tb;
        $this->rowRes = $rR;
        $this->limInit = $lI;
        $this->limEnd = $lE;
        $this->direction = $direction;

        $row1 = $this->fetchAllData();

        return $row1;
    }

}

?>
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Please stop using the deprecated mysql_* extension. It's no longer being maintained. Learn to use the replacement extension(s): PDO and mysqli, and learn to love prepared statements \$\endgroup\$ Commented Feb 18, 2015 at 11:53

2 Answers 2

1
\$\begingroup\$

Naming

  • Variable names should make it very clear what a variable does. One or two letter variables generally don't do that. tb, rR, lI, `lE are all very unclear names.
  • Do not shorten words in variables if possible. eg limInit, limEnd, rowRes.
  • Be consistent with your variable names. Either use camelCase or under_score.
  • your function names are sometimes unclear. What does sqlFetcher fetch that isn't fetched by fetchAllData? Does fetchAllData actually fetch all data? etc.
  • getResources isn't a very good class name (What is a getResources? ResourceGetter would be semantically better, but still not express what it does very well). Also, what resources? It looks like posts, because post_id is hardcoded (which is odd, since the table name isn't).

SQL

  • is there a reason for the different getSomething functions? Because it doesn't seem likely that only the title or only the author is needed very often. And a user of this class might think that these are simple getter (as the name implies), while in reality they perform costly database accesses. I would get rid of all of these and return the complete row, ideally wrapped in an object.
  • Be consistent with your SQL statements. Either SQL keywords are all upper case or all lower case (I prefer upper case).
  • don't select *, but name the fields you want explicitly.
  • In sqlFetcher you check $run, which doesn't actually exist.

Your code looks wide open to SQL injection. It is unclear what data is user supplied, but for security reasons I would treat all variable data as user supplied (it doesn't hurt). Use prepared statements with mysqli or PDO everywhere, always.

OOP

Currently, it seems a bit unclear what your class should actually do. Should it be a model that temporarily stores data, as the getter imply? Or should it perform database interactions?

You could easily separate these concerns with a different structure:

class Post {

    private $author;
    private $title;
    // [...]

    public function __construct($author, $title) {
            $this->author = $author;
            $this->title = $title;
            // [...]
    }

    public function getAuthor() {
        return $this->author;
    }

    public function getTitle() {
        return $this->title;
    }
}

class PostDAO {

    function getById($id, $mysqli) {
        $selectById = $mysqli->prepare("SELECT author, title FROM <table_name> WHERE post_id = ?");
        $selectById->bind_param("i", $id);
        $selectById->execute();
        $selectById->bind_result($author, $title);
        $selectById->fetch();
        $post = new Post($author, $title);
        $selectById->close();
        return $post;
    }

    function getAll($mysqli, $amount, $offset = 0) {
        $selectAll = $mysqli->prepare("SELECT author, title FROM <table_name> LIMIT ?,?");
        $selectAll->bind_param("ii", $offset, $amount);
        $selectAll->execute();
        $selectAll->bind_result($author, $title);
        $posts = array();
        while ($selectAll->fetch()) {
            $posts[] = new Post($author, $title);
        }        
        $selectAll->close();
        return $posts;
    }
}

You can use it like this:

$mysqli = new mysqli(credentials);
$post = PostDAO::getById($mysqli, 1);
echo $post->getAuthor();

$posts = PostDAO::getAll($mysqli, 5);
echo $posts[0]->getAuthor();

Now it's very clear that calling the getters does not cost very much (time-wise), and your model can be reused. For example, you might already know all the data without needing a database connection (because they are stored in the URL or session, or because you are performing tests and thus set them yourselves). If you only use the Post class in your other classes (such as the views), it can easily be separated from the database.

\$\endgroup\$
1
\$\begingroup\$

The simple things:

A quick canter through the doubts/questions you're having.

The code doesn't seem to have any bugs

That's not entirely true. The fact that your code uses the mysql extension will produce E_DEPRECATED notices if you attempt to run this code on PHP 5.5 and up. The extension is deprecated, as the red warning box on the doc pages will tell you.
When developing/testing new code, always turn display_errors on, and set the error_reporting to E_STRICT|E_ALL. Notices are a sure-fire way to slow your code down (seeing as the notices are being produced anyway, and only hushed up at the last moment).

mysql also doesn't support prepared statements, so you might end up with injection vulnerabilities. This means you'd have to spend a lot more time thoroughly testing your code, and wasting a lot more hours on security testing that you normally would.

whether using object orientation will be better (speed etc) than the functional way

First off, allow me to be pedantic and say that PHP does really do functional programming. PHP functions are not first class objects: they can't be returned by functions, or be passed around as arguments, or assigned to variables. Sure, PHP has anonymous functions, which are actually objects (get_class(function(){}); will tell you that anonymous functions are instances of the Closure class. Functional languages are Lisp, Scheme, Haskell, JavaScript and so on. PHP originally was a procedural language, that now supports OOP, too.
There are some constructs that allow you to adopt a more functional style of programming, but that's more syntactic sugar than it is actual functional programming.

Having said that, your question is an easy one to answer: OO will almost never be faster. OOP wasn't introduced to increase performance, or at least not runtime performance. Its main purpose was to increase code re-usability, cut down on development (mainly maintenance) time, create cleaner, more intuitive and easy to learn API's and all that. In exchange, a little overhead is a price worth paying (in most cases).
If all you're really after, you'd still be best served using regular procedural code. To increase performance even more, you'd probably want to keep the number of files to a minimum (to reduce disk I/O), and you might want to replace all loops like this:

foreach ($array as $k => $v) {
    //do stuff
}

With the faster:

foreach ($array as &$v) {
    //do stuff
}

It's faster (or at least, last I checked it was), but there are some major drawbacks you'll have to take into consideration: more error-prone code, hellish to debug, harder to read, understand and therefore harder to maintain,... Basically, it's just not worth it.

Bottom line is: don't worry about speed too much. If you're using PHP 5.5 and up, or use APC to cache the OP-Codes, you won't be able to tell the difference. Micro optimization is the root of all evil and all that.
OO will make a huge difference when your codebase starts to grow in size. You'll be happy with things like namespaces and classes that bundle functionality nicely without your having to wonder which files you need to require, and whether or not you've require'd it already

The code itself

Now moving on to the actual code

Standards matter

It's something I can bang on about for quite some time, but it boils down to this: Code should be easy to read for anyone familiar with the language. If we all write code the way we like it, the coding style can become a hindrance. Just think of it like this: you can read (obvioulsy), but some people's handwriting can be quite hard do decipher. The same principles apply to code, and to get around the handwriting problem, the PHP community, for the largest part, has adopted/accepted the PHP-FIG standards. You should to.

Looking at your code, this means that:

    <?php
class getResources{

Should become:

<?php //no leading whitespace
//classes start with an UpperCase, get is for methods
class Resources
{//<== goes on the next line

Other things you need to fix include:

function getContent($rowId, $tableName){
    //method body
}
//...
?>

Fixes needed:

  • Missing access modifier (public by default, but should be added explicitly)
  • methods' opening { goes on a new line
  • Files containing nothing but PHP code should not have a closing ?> tag

What's the point?

Now seeing as you're going to have to rewrite this class anyway, to use one of the replacement extensions for mysql, you might want to look at this review. You're essentially writing a wrapper object. Given that the new extensions (in particular PDO) has a very clean API right out of the box, I'd strongly suggest you didn't bother to try and wrap it up in a class of your own. At least not in the way you're doing now. You'd have to create an actual abstraction layer, which has been done before (Doctrine's DBAL, for example).

Again, this is something I could drone on about for ages, like I did in the review I linked to, but the gist of it really is this: Attempting to wrap the new DB extensions into a single class is going to end up in tears. That's a promise. What you need to realize about this matter is that code that interacts with databases actually consists of several distinct resources:

  • A DB connection (a PDO instance, or a mysqli instance)
  • A handle to a prepared statemend (a PDOStatement instance, or a mysqli_stmt instance)
  • Query results (Again: PDOStatement instance or a mysqli_result instance).

Trying to wrap all of this into a single class is a gross violation of the Single Responsibility Principle (SRP) - The S in S.O.L.I.D.. This principle states that a class can have only one reason to change. Which is a fancy way of saying that a Class can do only one job. Connecting to the DB, and representing the state of that connection is an important job. What results are obtained through that connection, and what happens to them afterwards is another job, hence it requires another class.

So what now?

To be honest, there's a lot of alternative routes you can explore. For now, though, I think the simplest one is to simply use data models, and a data provider, or mapper of sorts. Something like this might work quite well for you now:

class Dataprovider
{
    /**
     * @var PDO
     */
    protected $db = null;

    /**
     * Pass a PDO connection to the constructor, this connection will be used to query the DB
     *
     * @param PDO $db
     */
    public function __construct(PDO $db)
    {
        $this->db = $db;
    }

    /**
     * Pass a model instance to this method, and a bind array to get the data
     * if the bind is null, all records will be fetched
     *
     * @param Model $model
     * @param array $bind = null
     * @return array
     */
    public function fetchModels(Model $model, array $bind = null)
    {
        $fields = $model->getFields();
        $query = sprintf(
            'SELECT %s FROM %s.%s',
            $model->getFields(),
            $model->getDbName(),
            $model->getTable()
        );
        if ($bind !== null) {
            $where = [];
            foreach ($bind as $k => $v) {
            //perhaps implement bind_param here, to specify types, for now KISS:
                $where[] = sprintf('%s = :%1$s', $k);
            }
            $query .= ' WHERE '.implode(' AND ', $where);
        }
        $stmt = $this->db->prepare($query);
        $stmt->execute($bind);
        //get the model class, so we can create the instances using the results
        $class = get_class($model);
        $models = [];
        while ($row = $stmt->fetch(PDO::FETCH_ASSOC)) {
            $models = new $class($row);
        }
        return $models;
    }
}

Of course, this'll require you to (re)write the models, too:

abstract class Model
{
    protected $table = null;
    protected $dbName = 'default';
    protected $fields = [];

    public function __construct(array $data = [])
    {
         foreach ($data as $key => $value) {
            if (in_array($key, $this->fields)) {
                $setter = 'set'.ucfirst($key);//use setters to validate data
                $this->{$setter}($value);
            }
         }
    }

    public function getDbName()
    {
        return $this->dbName;
    }

    public function getTable()
    {
        return $this->table;
    }

    public function getFields($asArray = false)
    {
        return $asArray === true ? $this->fields : implode(', ', $this->fields);
    }
}

//an example:
class User extends Model
{
    const STATUS_ACTIVE = 1;
    const STATUS_UNCONFIRMED = 0;
    const STATUS_DELETED = -1;

    protected $table = 'user';
    protected $fields = [
        'id',
        'status',
        'email',
    ];
    private $id,
            $status,
            $email;

    public function setStatus($status)
    {
        //validate: if ($status !== self::STATUS_ACTIVE && ...){}
        $this->status = $status;
        return $this;
    }

    public function getStatus()
    {
        return $this->status;
    }

    public function setId($id)
    {
        if (!is_numeric($id)) {
            throw new InvalidArgumentException('A user id is numeric');
        }
        $this->id = (int) $id;
        return $this;
    }

    public function getId()
    {
        return $this->id;
    }

    public function setEmail($email)
    {
        if (!filter_var($email, FILTER_VALIDATE_EMAIL)) {
            throw new InvalidArgumentException('Invalid email!');
        }
        $this->email = $email;
        return $this;
    }

    public function getEmail()
    {
        return $this->email;
    }
}

With these classes, you could quite easily query the DB like so:

$activeUsers = $dataProvider->fetchModels(
    new User(),
    [':status' => User::STATUS_ACTIVE]
);
foreach ($activeUsers as $user) {
    echo 'User email ' . $user->getEmail() . ' is active and, thanks to the setter, Valid', PHP_EOL;
}

Note that this code just an example. It's far from complete/finished. These classes were also written in one go, here, on this site, so I haven't actually bothered to test/lint-check any of the code. It's possible the syntax isn't quite right here and there

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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