5
\$\begingroup\$

The following is a small class I extended MySQLi with. I'm going to use this in my upcoming projects, but my main reason to do this class is learning, so I would like to submit this for your review, tell me if there are any bugs or improvements to be made.

NOTES:

  1. I know about prepared statements, but I am not going to only run the same queries with different parameters, I'm expecting very different queries in each page, So I don't want to use prepared statements. Don't suggest prepared statements.

  2. I am aware of PDO, but I will use MySQL only, so I'm better with MySQLi. So please don't suggest PDO.

class Dbquery extends MySQLi
{
    public $host,$user,$password,$database,$connection;
    public function __construct($host,$user,$password,$database)
    {
        $this -> host = $host;
        $this -> user = $user;
        $this -> password = $password;
        $this -> database = $database;
        $this -> connect_me();
    }
    private function connect_me()
    {
        $this -> connection = $this-> connect($this->host,$this->user,$this->password,$this->database);
        if( $this -> connect_error )
            die($this->connect_error);
    }
    private function extracts($data) // function used to append data and column names for insert query
    {
        $column = array("","");
        foreach ($data as $index => $details)
        {
            $column[0].= $index.",";
            $column[1].= "'".$this ->real_escape_string($details)."',";
        }
        $column[0] = rtrim($column[0],",");
        $column[1] = rtrim($column[1],",");
        return $column;
    }
    private function append($data) // function used to append data and column names for update query
    {
        $string = "";
        foreach($data as $index => $details)
        {
            $string.= $index."='".$this -> real_escape_string($details)."',";
        }
        $string = rtrim($string,",");
        return $string;
    }
    public function insert($table,$data)
    {
        $extracted = $this -> extracts($data);
        $this -> query("INSERT INTO ".$table." (".$extracted[0].") VALUES(".$extracted[1].")");
        if($this -> error)
            return $this -> error;
        else
            return $this -> insert_id;
    }
    public function update($table,$data,$where)
    {
        $extracted = $this -> append($data);
        $this -> query("UPDATE ".$table." SET ".$extracted." WHERE ".$where);
        if($this -> error)
            return $this -> error;
    }
    public function select($table,$column = "",$where = 1,$orderby = "",$limit = "")
    {
        if($column == "")
            $column='*';
        if($orderby == "")
            $orderby = 'NULL';
        if($limit != "")
        {
            $limit = "LIMIT ".$limit;
        }
        $result = $this -> query("SELECT ".$column." FROM ".$table." WHERE ".$where." ORDER BY ".$orderby."".$limit."");
        if($this -> error)
        {
            return $this -> error;
        }
        else
        {
            $index = 0;
            while($row = $result -> fetch_assoc())
            {
                $data[$index] = $row;
            }
            return $data;
        }
    }
    public function delete($table,$where)
    {
        $this -> query("DELETE FROM ".$table." WHERE ".$where."");
        if($this -> error)
        {
            return $this -> error;
        }
        else
        {
            return true;
        }
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ First thing I notice is a lack of sanitization. You seem to be passing the arguments into the SQL string straight up. Can you supply some examples of your usage in a theoretical project? These methods seem to wrap around simple methods already provided by mysqli, and dare I say, pdo. \$\endgroup\$ – Phix Apr 11 '14 at 7:46
  • \$\begingroup\$ the real_escape_string is not enough? I'm searching for a way to send the values of select functions into real escape string. soon I'll take PDO. \$\endgroup\$ – Vignesh Apr 11 '14 at 7:54
  • \$\begingroup\$ @Paul can I run complex queries like join queries inner join using prepared statements, and can I run different query each time using prepared statements? is that efficient? \$\endgroup\$ – Vignesh Apr 15 '14 at 4:34
14
\$\begingroup\$

Note: I don't think this is what you want to hear. But I have to write "the truth". This is my personal opinions about your code.

Give me parameterized SQL or give me death!

tell me if there are any bugs or improvements to be made.

Don't suggest prepared statements.

I'm sorry, but those two requests really don't fit together in the same Code Review question.

I understand what you're doing here, and it's nice that you're doing it for learning purposes. It's nice to be able to extend the mysqli class in PHP.

However, I wouldn't recommend using the methods you have in this class. As you say:

I'm expecting very different queries in each page

Consider then: What functionality does this class provide you with? How much code does it save for you? Will it make your code more readable?

As I see it, there are primarily four lines that summarize what your class is all about:

$this -> query("INSERT INTO ".$table." (".$extracted[0].") VALUES(".$extracted[1].")");
$this -> query("UPDATE ".$table." SET ".$extracted." WHERE ".$where);
$result = $this -> query("SELECT ".$column." FROM ".$table." WHERE ".$where." ORDER BY ".$orderby."".$limit."");
$this -> query("DELETE FROM ".$table." WHERE ".$where."");

That is, you're essentially making it a little "easier" to write the SQL queries.

In your comment you ask if you can run different queries each time using prepared statements, and if you can use joins and stuff. The answer to these questions is: YES! Is it efficient? YES!. We're often talking milliseconds of difference between prepared statements and non-prepared statements (except when you perform the same prepared statement multiple times with different parameters, in which a prepared statement performs significantly faster!). If you are worried about the speed of prepared statements, don't be. They're fast. If they're slow, then there's something wrong with your database structure or your query, not with the fact that you're using prepared statements.

Here is what I personally think:

  • Other developers reading your code will be confused as they are not familiar with your Dbquery class
  • By not using your class you will be more experienced with writing SQL statements, it will probably go faster writing the SQL statements than you think.
  • Your class does not provide that much functionality. In fact, it can act as a restriction as writing something like select("mytable LEFT JOIN table2 ON (mytable.a = table2.b)", "table2.c, mytable.a", "mytable.a > 4", "table2.c", 5) just feels completely strange, wrong, and hard to remember what the parameters are, and you'll likely end up trying to reconstruct the resulting SQL query in your mind anyway (or is that just me?)

The point I'm trying to make here is: If you expect "very different queries in each page", then write those queries in each page. As long as they're not entirely dynamic queries, use prepared statements! Prepared statements are the way to go in the "real" PHP world.

I am aware of PDO, but I will use MySQL only, so I'm better with MySQLi. So please don't suggest PDO.

Sure, I have nothing against MySQLi myself. That's also what I tend to use when working in PHP. I think the choice of PDO vs. mySQLi is mostly opinion-based. The main advantage of PDO is that it can work with other databases except mySQL, but as you will use mySQL only, it's perfectly fine in my opinion to use mySQLi. At least you're not using the old mySQL functions!

\$\endgroup\$
  • 5
    \$\begingroup\$ +1 for the prepared statements part. Not only does it make it look better & easier to code with, it also makes it much more secure. You really don't want to be vulnerable to SQL injection. \$\endgroup\$ – hichris123 May 22 '14 at 1:08
  • 5
    \$\begingroup\$ @Simon Forsberg : It may be a little late to thank you, but your answer really lead me to the right direction. Since I was a beginner, I know laugh at this question on the words like don't suggest pdo, don't suggest prepared statements. Thanks man, you gave me the hurting truth at the right time. \$\endgroup\$ – Vignesh Mar 23 '16 at 12:32
  • \$\begingroup\$ @Vignesh It's never too late for a comment like that, thanks. Consider asking a CR question for how your code looks today, it could be impressive to see how much you have developed as a programmer. \$\endgroup\$ – Simon Forsberg Mar 23 '16 at 13:24
  • \$\begingroup\$ yeah sure, I'll do it. \$\endgroup\$ – Vignesh Mar 24 '16 at 10:15
  • \$\begingroup\$ To elaborate on db extension choice. There are benefits that MySQLi has over PDO like asynchronous queries, and others PDO has over MySQLi (aside from supporting multiple DBs), like stmt::fetchAll(PDO::FETCH_GROUP | PDO::FETCH_COLUMN), using named parameters, not forced to specify parameter data types, ability to set parameter data using stmt::bindValue(':param', 'value') or stmt::execute([':param' => 'value']). In general for "ease of use", PDO is preferred. In complex apps MySQLi is preferred. Use what can accomplish your goals. \$\endgroup\$ – fyrye Oct 30 '18 at 14:58
5
\$\begingroup\$

I'm not a PHP programmer so I will only address what I see from your use of MySQLi:

public function update($table,$data,$where)
    {
        $extracted = $this -> append($data);
        $this -> query("UPDATE ".$table." SET ".$extracted." WHERE ".$where);
        if($this -> error)
            return $this -> error;
    }

Your UPDATE clause seems to be missing a few arguments to be proper SQL syntax which is:

UPDATE table_name SET column_name = value1 WHERE column_name = value2

I would say the same about this section:

}
        $result = $this -> query("SELECT ".$column." FROM ".$table." WHERE ".$where." ORDER BY ".$orderby."".$limit."");
        if($this -> error)
        {

Correct syntax is:

SELECT values FROM table_name WHERE column_name = some_value ORDER BY column_name [ASC] | [DESC]

That's all I got as far as MySQLi is concerned.

\$\endgroup\$
  • 3
    \$\begingroup\$ Of course this would be much better and safer if you passed your values to MySQL via a stored procedure, but as you have indicated in your OP you don't want prepared statements, which doesn't make sense to me. \$\endgroup\$ – Phrancis May 22 '14 at 0:04
  • \$\begingroup\$ I decided to use PDO and looking at it now thanks for your help \$\endgroup\$ – Vignesh May 26 '14 at 12:44

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.