5
\$\begingroup\$

I've made a simple querybuilder for my job, but I'm kind of insecure about starting to use it because I'm afraid of errors happening when I start using it. I've done some tests with where, join, etc... So far I didn't find anything, but if someone could take a look and see if there's something missing, I would appreciate it a lot.

Is this class reliable enough?

P.S.: I've only made it work for SELECT queries because it's the only thing I need.

P.P.S.: I know I could use a external lib, but it is worth nothing trying to make one myself. (And libs are kind of overwhelming for what I need)

<?php
class QueryBuilder {
    private $query;
    private $table;
    public function __constructor() {
        $this->query = '';
        $this->table = null;
    }
    public function __call($method, $arguments) {
        $index = 1;
        preg_match('/where/i', $method, $isWhere);
        if(!empty($isWhere) && strlen($method) > 6) {
            $method = str_ireplace($isWhere[0], '', $method);
        }
        preg_match_all('/[A-Z]/', $method, $temp, PREG_OFFSET_CAPTURE);
        $temp = $temp[0];
        if(!empty($temp)) {
            foreach($temp as $offset) {
                if($offset > 1 || $offset[1] > 0) {
                    $this->query .= strtoupper(substr_replace($method, ' ', $offset[1], 0));
                    $this->query .= ' ';
                }
            }
        }
        else {
            $this->query .= strtoupper($method);
            $this->query .= ' ';
        }
        if(count($arguments) == 1) {
            if(!empty($arguments)) {
                foreach($arguments as $arg) {
                    foreach($arg as $key => $value) {
                        $this->query .= $value;
                        if($index < count($arg))
                            $this->query .= ', ';
                        else
                            $this->query .= ' ';
                    }
                }
            }
        }
        else {
            if(!empty($arguments)) {
                $this->query .= $arguments[0];
                $this->query .= ' ON ';
                unset($arguments[0]);
                foreach($arguments as $arg) {
                    foreach($arg as $key => $value) {
                        $this->query .= $value;
                        if($index < count($arg))
                            $this->query .= ', ';
                        else
                            $this->query .= ' ';
                    }
                }
            }
        }
        return $this;
    }
    public function table($tables = []) {
        $index = 1;
        if($tables === null || empty($tables)) {
            throw new Exception('Table value cannot be null.');
            return;
        }
        foreach($tables as $table) {
            $this->table .= $table;
            if($index < count($tables)) {
                $this->table .= ', ';
            }
            else {
                $this->table .= ' ';
            }
        }

        return $this;
    }
    public function select($cols = null) {
        $index = 1;
        $this->query .= 'SELECT ';
        if($cols === null || empty($cols) || $cols == '') {
            $this->query .= ' * ';
        }
        else {
            foreach($cols as $value) {
                $this->query .= $value;
                if($index < count($cols)) {
                    $this->query .= ', ';
                }
                $index++;
            }
        }
        $this->query .= ' FROM '.$this->table;
        return $this;
    }
    private function makeQuery() {
        return $this->query;
    }
    public function get() {
        return $this->makeQuery();
    }
}
$test = new QueryBuilder();
echo $test
        ->table()
        ->select(['id', 'name'])
        ->Where(['name= \'john\''])
        ->whereNotIn(['id > 1'])
        ->get();
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Did you consider possible SQL injection threats from input data? \$\endgroup\$ – πάντα ῥεῖ May 8 '18 at 18:43
  • \$\begingroup\$ I haven't because the IDE that we're using handles it by itself. I just want to generate SQL queries through this code. \$\endgroup\$ – Mateus Barbosa May 8 '18 at 18:47
  • \$\begingroup\$ I created something like that here: github.com/JustCarty/DbQueryBuilder It is work in progress and I stopped developing it when I discovered a way of implementing Laravel Query Builder within my app without the need for the rest of Laravel: medium.com/@lekker/… \$\endgroup\$ – JustCarty May 13 '18 at 9:52
  • \$\begingroup\$ "I haven't [considered SQL injection threats] because the IDE we're using handles it by itself" --- Your IDE isn't running on the server, so any string concatenation when building a SQL statement means that one text field on your web page becomes a giant, unauthenticated remote session directly to your database that accepts any kind of SQL. I really hope this isn't on the public internet. \$\endgroup\$ – Greg Burghardt Jun 23 '18 at 1:24
2
\$\begingroup\$

Bug with WHERE conditions

I tried running the sample code:

$test = new QueryBuilder();
echo $test
    ->table()
    ->select(['id', 'name'])
    ->Where(['name= \'john\''])
    ->whereNotIn(['id > 1'])
    ->get();

It threw an exception because the argument to the table method was null (which would yield an empty array per the default argument value), so I changed it to pass an array with a single string literal ['users'] to ->table(). Then when I ran it again, I saw the string literal returned below:

SELECT id, name FROM users  WHERE name= 'john'  NOTIN NOT IN id > 1 

Correct me if this is wrong but most SQL engines need to have the predicates combined with the AND and OR keywords, and those where conditions NOTIN NOT IN would definitely yield an error. It is unclear how the NOT IN should be combined with the id > 1... My best guess is that a sub-query would be needed for that to work... something like

SELECT id, name FROM users  WHERE name= 'john'  AND id NOT IN (SELECT id FROM users WHERE id > 1)

Given that issue, I would say to your question "Is this class reliable enough?": No it isn't reliable, but maybe if that issue is resolved then it would be.

Constructor is useless

The only affects of the constructor are to set the two properties (instance variables) to primitive values (i.e. an empty string literal and null). Those could be initialized when declared since those values can be evaluated at compile time. Thus the constructor can be removed once those initializations are added to the declarations:

class Builder {
    private $query = '';
    private $table = null;

One advantage here would be that if this class had a parent class, then any method that overrides the same method in the parent class would need to have the same signature or at least pass sufficient parameters when calling the parent method is needed.

Variables declared even if not used

While the next section describes how to eliminate variables like $index, I do notice that variable is often declared as a local varible assigned the value 1 at the start of methods (like __call(), table(), select()). However in some cases the method may return early - for example in table() an exception is thrown if the $tables argument is null or empty. While it is only an integer, it is wise to not assign values to variables until they are needed. Imagine a large object was assigned to a variable there after calling a function (or multiple functions) - if the method returned early, then the CPU cycles used to get the variable from the function would then be wasted.

Use implode() instead of conditionally appending seperators

I see a few places like the block from select() below, where array elements are appended to the query property and then commas are added if end of the list hasn't been reached:

foreach($cols as $value) {
    $this->query .= $value;
    if($index < count($cols)) {
        $this->query .= ', ';
    }
    $index++;
}

That can be simplified using implode() with the comma and space used as the $glue and the array $cols as the $pieces parameters.

$this->query .= implode(', ', $cols);

And this makes $index superfluous so it can be removed.

\$\endgroup\$
3
  • \$\begingroup\$ I need to learn some more js because when I read your answers it's like reading harry-potter - all magic ;-] \$\endgroup\$ – t3chb0t Jun 22 '18 at 19:31
  • \$\begingroup\$ While that may be constructive for you and they are similar, this is actually PHP; Thanks for the compliment. I mostly just apply what I have learned over time :). \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jun 22 '18 at 19:35
  • \$\begingroup\$ lol, how embarassing is that -_- I was reading your answer and wondering what js is that! I should have looked at the tags first :-P \$\endgroup\$ – t3chb0t Jun 22 '18 at 19:43

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.