6
\$\begingroup\$

I am really just looking to make sure I am not making any weird mistakes that could hurt me in the long run or if something seems odd in the way I imagine it to work. This code does work for the way I use it and may lack other more advanced things that I may use in the future but have not implemented yet. I left in my phpdoc code to maybe make some sense of weird things. Any feedback is appreciated.

Note: I am constantly reading up on things and try to cover everything in each of the stack sites but I might have overlooked some very basic resources due to be newer to the stack family.

/**
 * Class Main Mysql Database 
 */

class mydb {

    /**
     * Connect to DB
     */
    public function __construct() {
        // Connect to Database
        $this->mydb = new mysqli('host', 'database', 'user', 'pass');
        if ($this->mydb->connect_errno) { // Error on connection failure
            echo "Failed to connect to MySQL in Construct: (" . $this->mydb->connect_errno . ") " . $this->mydb->connect_error;
        }
    } /** End construct */

    /**
     * Choose items from the database (SELECT Statement with WHERE equals and like options)
     * Designed to be slim and rely more on PHP than mysql for result formating
     * @param string $select
     * @param string $from
     * @param string $config
     * @param array $options
     * @return array
     */
    public function choose ($select, $from, $config = NULL, $options = NULL) {
        if ($config === NULL) {
            $stmt = "SELECT {$select} FROM {$from}";
        } elseif ($config === 'EQUALS') {
            $stmt = "SELECT {$select} FROM {$from} WHERE {$options['where_comp']} = '{$options['where_value']}'";
        } elseif ($config === 'LIKE') {
            $stmt = "SELECT {$select} FROM {$from} WHERE {$options['where_comp']} LIKE '{$options['where_value']}'";
        } /** End if $config */
        if (!($result = $this->mydb->query($stmt))) {
            echo 'Query Error: ' . $result->error . '<br/>';
        } else {
            while ($row = $result->fetch_assoc()) {
                $payload[] = $row;
            }
            return $payload;
        } /** End if mydb->query */
    } /** End choose method */

    /** Put items into the database (INSERT INTO $table ($col) VALUES ($set)
     * This method was designed to do basic inserts without complicating it and filter when $clean is required
     * @param string $table
     * @param string $col
     * @param array $set
     * @param string $clean
     * */
    public function put ($table, $col, $set, $clean = NULL) {
        if ($clean) {
            $set = "'" . implode("','",filter_var_array($set,FILTER_SANITIZE_STRING)) . "'";
            $stmt = "INSERT INTO {$table} ({$col}) VALUES ({$set})";
        } else {
            $set = "'" . implode("','",$set) . "'";
            $stmt = "INSERT INTO {$table} ({$col}) VALUES ({$set})";
        } /** End form $stmt  */
        if (!($putResult = $this->mydb->query($stmt))) {
            echo 'Insert Error: ' . $this->mydb->error . '<br/>';
        } else {
            return $putResult;
        }
    } /** End put method */

    /** Change items in the database (UPDATE $table SET item=value WHERE condition)
     * This method was designed to do basic updates without complicating it and filter when $clean is required
     * @param string $table
     * @param array $set
     * @param array $where
     * @param string $clean
     * */
    public function change ($table, $set, $where, $clean = NULL) {
        if ($clean) {
            foreach ($set as $key => $value) {
                $value = filter_var($value, FILTER_SANITIZE_STRING);
                $setArray[] = "{$key}='{$value}'";
            }
            $set = implode(',',$setArray);
            $stmt = 'UPDATE ' . $table . ' SET ' . $set . ' WHERE ' . $where['target'] . ' = ' . $where['match'];
        } else {
            foreach ($set as $key => $value) {
                $setArray[] = "{$key}='{$value}'";
            }
            $set = implode(',',$setArray);
            $stmt = 'UPDATE ' . $table . ' SET ' . $set . ' WHERE ' . $where['target'] . ' = ' . $where['match'];
        } /** End form $stmt  */
        if (!($putResult = $this->mydb->query($stmt))) {
            echo 'Insert Error: ' . $this->mydb->error . '<br/>';
        } else {
            return $changeResult;
        } /** End Query Check */
    } /** End change method */

    public function __destruct() {
        if (is_object($this->mydb)) {
            $this->mydb->close();
        }
    } /** End destruct */

} /** End mydb Class */

Example of CODE use:

::Include above and create $mydb object::

$set = array('team'=>'First Shift Tech','access'=>'None');

$where = Array('target'=>'author','match'=>'John Doe');

$mydb->change('agent',$set,$where); Can I pass array's in this line instead? Seems I get an inspection error in phpstorm if I try to do some type of lazy deal.

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

First of a note: you say yourself that everything is working. And that is what often counts. I hope I'm not to harsh, but it's my way of telling things ;)

Overall:

  • you should (try and) avoid an 'if' statement that change the action of a method. And document the $config (what does it mean, what does it do)
  • you should use method names that are self-explicable (choose is actually select, change is actually update, put is actually insert.)
  • Again, use method names that say what they do. 'change': change what? a record? the table declaration? the column definition?
  • Never echo inside a method. If there is an error throw one. Of use a global Error handling class. e.g. Log::setError(..); (just imagining trying to figure out why there is a 'header allready sent' error because of that error).

Now,I don't really know what you wrote. Is it a wrapper class for mysqli? That failed, you can now do less and code doesn't get easier to read (imo).

Is it an adapter that adapts mysqli to a certain interface? Where is the interface?

Does it help you abstract the database-layer and easily change the DB? again, no. mysqli is tightly coupled with this mysql class. What if we want to start using PDO? Or even step away from mySQL? All that code is useless then.

Now, some specific remarks:


function choose();

Designed to be slim and rely more on PHP than mysql for result formating

In what way does it rely more on php for result formatting? You are simply calling fetch_assoc(); where is the formatting happening?


function change(); & function put();

This method was designed to do basic updates without complicating it and filter when $clean is required

How doesnt it complicate things? Why not use the mysqli prepared statements and let mysqli handle the filtering of of inputted variables? And your code isn't really basic. There is a lof of 'Repeat-yourself code'.


So, let's fix what is 'broken'.

I think that what you actually wanted to create was an adapter. The task of an adapter is translating the desired interface onto the correct method of the actual db class (mysqli in your case).

To start of, we will have to design the desired interface. Let's for the sake of my answer (I'm lazy) create a simple Select, Update, Delete and Insert interface using an 'identifier' string as primary key.

<?php
interface Database {
    public function __construct($client);
    public function select($table, $identifier);
    public function insert($table, $identifier, $data);
    public function update($table, $identifier, $data);
    public function delete($table, $identifier);
}

Ok, brilliant. We now let other programmers (and ourself) know that if you are passing me a Database object, it has to implement these methods. Sadly for us, mysqli doesn't implement these methods directly. So the adapter pattern comes in handy here. We write an adapter for mysqli to fit our Database interface:

<?php
class MysqliToDatabaseAdapter implements Database {
    private final $client;

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

    public function select($table, $identifier) {
        $stmt = $this->client->prepare('SELECT * FROM ' . $table . ' WHERE identifier = ?');
        $stmt->bind_param('s', $identifier);
        $stmt->execute();
        return $stmt->fetch();
    }

    public function insert($table, $identifier, $data) {
        $stmt = $this->client->prepare(
            'INSERT INTO ' . $table .
            ' (`identifier`, `' . implode('`,`'array_keys($data)) . '`)' .
            ' VALUES (? ' . str_repeat(',?', count($data)) . ')'
        );
        $data = array_values($data);
        array_unshift($identifier, $data);
        $stmt->bind_param(
            str_repeat('s', count($data)),
            $data
        );
        return $stmt->execute();
    }

    public function update($table, $identifier, $data) {
        //...
    }
    public function delete($table, $identifier, $data) {
        //...
    }
}

We would then use this class as follows:

<?php
$mysqli = new mysqli('host', 'database', 'user', 'pass');
$myDbHandler = new MysqliToDatabaseAdapter($mysqli);
//somewhere in the code
$myDbHandler->insert('my_table','my_identifier',array('my_column'=>'my_column_data'));
//and then we select it
$data = $myDbHandler->select('my_table','my_identifier';

Now this code is easy to maintina, the methods do what they are called (select, insert,...) and we can now easily change the databaseHandler without having to change anything in our application.

Offcourse at this point the Database interface is of not much use. There is no way to 'filter' data, bulk update, ... But these could all be implemented. A good rule here is if you have to write 'if' in the documentation, then split up the method into two different methods. e.f.: update() and bulkUpdate() or select() and selectByColumn();

Always keep your code small. Don't stuff one interface with massive amount of functions. Create little small interfaces that focus on one part. e.g. DatabaseCrud (for simple create, read update and delete) DatabaseBulk (for bulk manipulations) DatabaseSomethingCompletlyDifferent (some other functions)

Then depending on what you ned you can easily pass different DatabaseAdapters to different objects. Most code will only need simple CRUD functions. So you pass in the simple DatabaseCrudAdapter we made. For some special exotic method we can then create a bigger Adapter that implements extra interfaces that uses the same mysqli object. Some code for demonstrations:

<?php
//create the mysqli object
$mysqli = new mysqli('host', 'database', 'user', 'pass');
//create our adapters
$simpleCrud = new MysqliToSimpleDatabaseCrud($mysqli); // implements SimpleDatabaseCrud();
$simpleBulkCrud = new MysqliToSimpleBulkDatabase($mysqli); // extends MysqliToSimpleDatabaseCrud implements DatabaseBulk
//create some objects
$foo = new IOnlyNeedSimpltCrudFunctions($simpleCrud);
$bar = new INeedSomeSpecialBulkFunctions($simpleBulkCrud);

I hope this maks any sense :p

So, now it's up to you. You will have to write out a decent set of interfaces used inside your program. Then create adapters for existing code to fit those interfaces.

More on the adapter pattern: http://en.wikipedia.org/wiki/Adapter_pattern Some other usefull links:

\$\endgroup\$
  • \$\begingroup\$ This code was designed for a simple task of not passing the info for the db in each page that needed the database. I normally use exact method names but figured I would use different names for the heck of it. I felt like the interface only had benefit in the event that you had a bunch of different developers adding to the design but I see why its good to get into that practice. I did write a prepared version and a PDO version but I hate PDO and think it complicates things too much. The points you are making do help me a lot as Thanks for your time! \$\endgroup\$ – sin0cide Aug 13 '13 at 20:57
  • \$\begingroup\$ Also instead of waiting till I fully understand the requirements of the code I decided to make the basic "crud" setup to just give me a base to work with. Talking about Designed to be slim and rely more on PHP than mysql for result formating that meant I pull a large DB result and use arrays and functions to return results that don't need joins. I understand the importance of not duplicating the results in the db but don't really see a need for joins yet! If I change the db type I would write a new class for it since I feel PDO adds too much and slows everything down. \$\endgroup\$ – sin0cide Aug 13 '13 at 21:01
  • \$\begingroup\$ @sin0cide People think that using certain patterns instead of hacking away until it works is only good for big development enviroments. But they tend to forget themselves. If I now loko back at code I wrote 2 years ago before. I have no idea what is going on. I then am very happy to see that I used Patterns and good names. BEcause let's face it. We hardly read documentation :p I'm glad my answer helped :) \$\endgroup\$ – Pinoniq Aug 14 '13 at 9:06
5
\$\begingroup\$

I basically subscribe to most of Pinoniq's remarks, but I'd like to touch on one thing in particular, that I just don't get.
It's good to see you're using mysqli_*, and I, for one, do understand why one would write a wrapper for this extension. It's API can be messy, and look confusing, and just allows for the users to sort-of switch from OO to procedural style, as they see fit. That's just wrong.

However, you are significantly reducing the features I can use: Where is the prepare method? How can I use transactions? How do I configure the connections? can't I link data models to rows of a given table?
No, so what would stop me from creating my own instance of mysqli and do what I want? There's no insentive for me to use your code, if anything, there are a few reasons why I wouldn't want to use your code.

You attempt to sanitize the values you insert into a table, as you also sanitze the where clauses. You're not using prepared statements, though, why?
What if I wanted to query the DB like so:

SELECT clients.first_name, clients.last_name, contacts.email FROM db.clients
    LEFT JOIN db.contacts 
        ON contacts.client_id = clients.id
WHERE clients.language = 1
  AND clients.active = TRUE;

You're treating everything as strings, whereas I'm actualy using a boolean and integer value in the where clause. But that's not the real issue. The only way I can possibly get my query to execute via your class is to pass my fields and the JOIN as the $from parameter, since it's just being concatenated in there, without any check on it whatsoever.
Now I know it's highly unlikely that user input will ever be passed through via these arguments, and it does at least allow users to perform a JOIN. Still, you have to agree this is a rather hacky workaround for a problem that needn't exist in the first place, and it does mean that, in theory, this is possible:

$mydb->choose($_POST['input'], $_POST['input2']);

With the post values being * FROM users /* and */ respectively, which means, you're executing this query:

SELECT * FROM users /* FROM */

Not what you want, I think...

The whole point of an abstraction layer should be to shield the users from having to write a ton of queries. Not all great devs are capable of writing great queries.

Basically, I'd advise you to not reinvent the wheel, but if you want to, as a technical exercise, look at the bigger picture: DB's consist of tables/collections. Each row of those tables holds a finite number of fields, and each field has a given type/dimension. The rows, therefore, can easily be poured into data objects. As can the tables, because, if you know the fields, you'll want to know how they fit in the table (key, primary, index, collation, storage engine...).

Take a look here, especially at the links below (ActiveRecord and ORM).
Read through a couple of pages, let it sit for a while, and think about how you'd go about implementing a simple query api.

And please, do use type-hints:
For example here:

public function choose ($select, $from, $config = NULL, $options = NULL)
{
    //some stuff
    $options['some_key'];
}

If I passed a string there, or a numerically indexed array, and saw there was an error, and the message pointed to a line in your code, I'd tell you to debug your class. If a user doesn't pass the correct type of argument, he should be notified when he attempts to call the method. Change the signature to:

public function choose ($select, $from, array $config = NULL, array $options = NULL)

Even so, you're simply assuming certain keys will be set. Either use an object, that has the property you need, and defaults to a meaningful value, or compare the provided array to a private array that contains the default values:

class FoolMe
{//minimal checks
    private $defaults = array(
        'name'    => array('type'  => 'string', 'value' => 'default'),
        'age'     => array('type'  => 'integer', 'value'  => 0)
    );
    public function checkArray(array $arr)
    {
        foreach($this->defaults as $k => $settings)
        {
             if (isset($arr[$k]))
             {
                 $arr[$k] = gettype($arr[$k]) === $settings['type'] ? $arr[$k] : $settings['value'];
             }
             else
             {
                 $arr[$k] = $settings['value'];
             }
        }
        return $arr;
    }
}

class SelectOptions
{
    protected $where_comp = null;
    public function __get($name)
    {//bad, but better than array
        if (property_exists($this, $name))
        {
             return $this->{$name};
        }
    }
    //best:
    public function getWhereComp()
    {
         if ($this->where_comp === null)
         {
             return '';
         }
         return (string) $this->where_comp;
    }
    public function setWhereComp($str)
    {
        $this->where_comp = (string) $str;//ensure types using setters
    }
}

Ah well, check a couple of my answers, both here and on SO. They often contain some elaborate example of a data-model and why you shouldn't use magic __get and __set methods and so on...

\$\endgroup\$
  • \$\begingroup\$ +1. I should indedd have gon a little deeper in the point you point out here \$\endgroup\$ – Pinoniq Aug 13 '13 at 13:53
  • \$\begingroup\$ @Pinoniq: To be honest, I'm surprized you didn't. Especially since your main focus in the comments to the linked question were about prepared statements. There's a lot of things in your answer I subscribe to, but I can't bring myself to upvote: in my latest edit, I pointed at the (highly unlikely, but still) security holes introduced by stringing a table name into the query. That's why I suggested making a table and row objects (like most FW's and ORM's do) \$\endgroup\$ – Elias Van Ootegem Aug 13 '13 at 13:56
  • 1
    \$\begingroup\$ True about the security concerns, in fact my code is crap and should never be used in production enviroments. It's more about explaining the adapter pattern and showing it's strengths. Then depending on what type of code you are writing you could go for Design by contract, defensive programming,... \$\endgroup\$ – Pinoniq Aug 13 '13 at 14:09
  • 1
    \$\begingroup\$ @Pinoniq: Well, the adapter pattern is nicely explained, +1 for that (I hadn't read the complete answer). Of course, there's a lot to explain, and the longer the answer, the less likely it is ppl will actually read it, there's always going to be a trade-off. Anyway your answer + some of my additions should be enough to get the OP started \$\endgroup\$ – Elias Van Ootegem Aug 13 '13 at 14:14
  • 1
    \$\begingroup\$ You could add a method that exposes the mysqli object (like a magic __call method), but don't return it. Prepared statements and the mysqli API don't mix well, I agree. They're a good example of how the API just looks like a right mess, PDO offers a lot easier, and cleaner API for prepared statements. And of course you don't need to sanitize prepared statements, that's another reason why to use prepared statements... \$\endgroup\$ – Elias Van Ootegem Aug 14 '13 at 7:28

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.