3
\$\begingroup\$

After a whole night of work I finally have my first useful class - for reading database.

It works like a charm, but if you got a moment, please take a glance and give me feedback. Harsh criticism welcome.

Please note that security is not addressed yet, as well as usual SQL clauses, but I'd like to know am I on the right way to writing better software.

Here it is:

class DatabaseRead {
protected $dbh;                 // Database handle connection
public $columns     = array();  // Columns to be read
public $table;                  // Table to be read from
public $resultSet   = array();  // Returned results

public function __construct( $dbh, $columns, $table ) {
    $this->dbh          = $dbh;
    $this->columns      = $columns;
    $this->column       = '';
    $this->table        = $table;
    $this->resultSet    = array();

    $read_database = $this->dbh->query('SELECT ' . implode(',' , $this->columns) . ' FROM ' . $this->table);
    while ($row = $read_database->fetch()) {            
        foreach($this->columns as $this->column) {
            $this->resultSet[$this->column] = $row[$this->column];
        }
    }
    return $this->resultSet;        
}

}

Have I wrote an actual class that makes sense, or just a glorified function?

Thanks.

\$\endgroup\$

migrated from stackoverflow.com Apr 10 '12 at 3:26

This question came from our site for professional and enthusiast programmers.

  • 6
    \$\begingroup\$ You can't return a value from a constructor. \$\endgroup\$ – Corbin Apr 10 '12 at 3:32
  • \$\begingroup\$ Add to @Corbin note: Because the constructor is there only for initialization of class variables, nothing more. \$\endgroup\$ – StormByte Apr 10 '12 at 9:22
1
\$\begingroup\$

Unfortunately this is not the path to follow, Rather than focusing on DataBase level read and write classes etc. consider higher concepts, the best you can get out of this approach is the right OO way of doing a wrong thing.

What you want to research is design patterns and more specifically Patterns of Enterprise Application Architecture, Active Record, Business Objects, Repository pattern Design etc. Also lookup ORM. That would give you a better idea of what type of classes that you should focus on.

Getting the deisgn and implementation of this DataTable read class will take further away from the right approach.

\$\endgroup\$
4
\$\begingroup\$

Why don't you split the "constructor" part and the "get the values" part?

Code :

class Database {

    protected $dbh;                 // Database handle connection

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

    public function getResults($columns,$table)
    {
        $resultSet = array();

        $read_database = $this->dbh->query('SELECT ' . implode(',' , $columns) . ' FROM ' . $table);
        while ($row = $read_database->fetch()) {            
            foreach($columns as $column) {
                $resultSet[$column] = $row[$column];
            }
        }
        return $resultSet;
    }
}

Usage :

$db = new Database($dbh);
$db->getResults($columns, $table);
\$\endgroup\$
4
\$\begingroup\$

Some time ago, I had the same problem than you, wanting to create an useful and secure database handling class.

In your case, there are some flaws in its design:

  1. Constructors are only for initializing class instance, not for doing anything else.
  2. Your class is too specific, only allowing to perform one single action, which is not too useful because in that case you'd need X different classes for your X different needs (queries) which is not the goal for a class.
  3. Also, as you pointed, there is nothing about security there, which can be very hard to implement if you start to use a model like that in your project (I mean, several changes later in several files, etc...).
  4. Your class seems not scalable nor reusable, which will make you work more rather than work less by allowing you to reuse code (which is one of the goals of OO).

So what I'd recommend you to do is the following (now you are on time to do so without side effects in your project, and a class that could be reusable):

  1. Create an abstract class for handling a generic database, with configurable parameters, such as host, username, dbname that will be initialized in constructor (in a class you inherit from this one)
  2. Create class functions to execute SQL:
    1. Run SQL queries
    2. Prepare prepared statements
    3. Execute prepared statements
    4. Connect/disconnect

Once done that, you have a reusable class for everytime you want to access a database in every project you may have in a future.

After that, just create a new class inheriting from this one (one per database you use (what means in practice one per project you have). This new class should be database specific (having the parameters to connect to database, preparing statements you will need and so on) rather than general, but reusing all its base code.

I have made something similar in the past I posted in my blog (will provide link rather than include it here because it is too long), you are welcome to use it as you like: http://stormbyte.blogspot.com.es/2011/03/mysqls-prepared-statements-made-really.html

\$\endgroup\$
  • \$\begingroup\$ I would also add some other functionality that will be important down the road: getting the rowcount (# of affected rows from an UPDATE/INSERT statement) as well as grabbing the last insert ID. \$\endgroup\$ – Tom Nov 9 '12 at 15:24
  • \$\begingroup\$ That is already implemented in the class I propose in the link provided :) \$\endgroup\$ – StormByte Nov 13 '12 at 13:48
2
\$\begingroup\$

If you wan't to write more bug-free, secure and extensible applications with highly reusable code, try to focus more on OOP.

So for example such class as DatabaseRead should always return object (Entity) not array. Why? Bacause you can then do much better validation of its data. So for example it returns instance of class Article. Article has no public properties, only getters and setters. This will grant you, that (if well written) $article->getTitle() will allways return string.

You can then reuse the class Article in administration, so Admin will always create the Article correctly.

You can then use type-hinting through your application as well, which will provide you with certainty, that the parameter which was passed to method is really an Article, not some uknown array (which could be in inconsistent state).

function foo(Article $article) {
   // we are now sure that we work with valid data of article
   $title = $article->getTitle();
}

As a showcase, have a look at this example: https://github.com/Dundee/testing-showcase/blob/master/app/models/ArticleRepositorySqlite.php

This class (ArticleRepository) has one responsibulity (see Single Responsibility Principle): the persistence of an Article. Class Article doesn't know about persitence at all. Bacause of this we can have multiple repository instances, which stores Article on different medium (memory, database, filesystem).

The concrete class ArticleRepositorySqlite uses some DB framework, but this is not always neccessary. The class could take PDO connection in constructor and execute ordinary SQL queries as well.

Hope it's understandable :)

\$\endgroup\$
  • \$\begingroup\$ Yes, I sort of get the basic idea, but am yet to understand practical implementation. Thanks for the link, will definitely dive more into this. +1 \$\endgroup\$ – CodeVirtuoso Apr 10 '12 at 17:49
  • \$\begingroup\$ PS.@Daniel Milde: Other than that, do you think I'm on a good way, does it make sense creating class like the one above? \$\endgroup\$ – CodeVirtuoso Apr 10 '12 at 18:03
  • \$\begingroup\$ I think there is quite a lot of such database layers, so there is probably not needed to write our own. Try look on Doctrine 2, NotOrm, Dibi etc. \$\endgroup\$ – Daniel Milde May 30 '12 at 12:54

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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