3
\$\begingroup\$

I'm learning OOP and totally new to this way of coding. I've always scripted PHP the procedurial way. Now I've written a working class, which creates a database connection and has the method to create a query which is impossible to SQL-inject by hexing the non-integer data. (I know how to use prepared statements, but i just don't want to use them because there is a slight performance penalty in my case)

Note: When hexing data the right way, its not possible to inject.
Have a look at Zaffy's answer.

My questions:

  1. What could I've done better?
  2. Is it 'wrong' to use this class because its not good OOP (besides its function), or does it not really matter, because it works as how i want it to work? (nobody else is going to maintain the script)

And I must say, I've read multiple tutorials, but it's really difficult to fully understand howto write OOP style if you've never done before!

databaseconnection.class.php

<?php
class DatabaseConnection {
    private $data, $sql, $conn, $host, $user, $pass, $dtbs;

    public function __construct($host, $user, $pass, $dtbs) {
        $this->host = $host;
        $this->user = $user;
        $this->pass = $pass;
        $this->dtbs = $dtbs;

        $this->conn = new mysqli($this->host, $this->user, $this->pass, $this->dtbs);
        if ($this->conn) {
            return $this->conn;
        } else {
            return false;
        }
    }

    public function query($sql, $data) {
        $this->data = $data;
        $this->sql = $sql;
        foreach ($data as $val) {
            if (strpos($this->sql, "'%i'") !== false || strpos($this->sql, "'%s'") !== false || strpos($this->sql, '"%i"') !== false || strpos($this->sql, '"%i"') !== false) {
                echo "SQL incorrect: There can't be any quotes around the parameters, because this function does that automaticly for you";
                exit();
            }
            $pos = strpos($this->sql, '%');
            $type = substr($this->sql, $pos, 2);
            if ($type == '%i') {
                if (is_int($val)) {
                    $this->sql = substr_replace($this->sql, $val, $pos, 2);
                } else {
                    echo "Parameter incorrect: Data is not an integer value";
                    exit();
                }
            } elseif ($type == '%s') {
                if (!is_int($val)) {
                    $this->sql = substr_replace($this->sql, "UNHEX('".bin2hex($val)."')", $pos, 2);
                } else {
                    echo "Parameter incorrect: Data is an integer value";
                    exit();
                }
            } else {
                echo "Parameter incorrect: Parameter %i for integer of %s for all other types of data";
                exit();
            }
        }

        $this->query = $this->conn->query($this->sql);
        return $this->query;

    }

    public function fetch($query) {
        return $query->fetch_assoc();
    }

    public function num_rows($query) {
        return $query->num_rows;
    }

}
?>

index.php

<?php
function __autoload($class){
  require('classes/' . strtolower($class) . '.class.php');
}
require('config.php');

$sql = new DatabaseConnection(DB_HOST, DB_USER, DB_PASS, DB_DTBS);
// The DB_xxxx constants are defined in config.php
?>
<!doctype html>
<html>
<head>
<meta charset="utf-8">
<title>OOP - Object Orientated Programming</title>
</head>

<body>
<?php
$data = array(1, "Brian o'Reilly");
$query = $sql->query("SELECT * FROM users WHERE id = %i AND name = %s", $data);
if ($sql->num_rows($query) > 0) {
    while ($r = $sql->fetch($query)) {
        echo '<p>User ' . $r['name'] . ' has ID ' . $r['id'] . '</p>';
    }
} else {
    echo "<p>No results</p>";   
}
?>
</body>
</html>
\$\endgroup\$
  • \$\begingroup\$ Please edit your title to tell us what your script is doing - as it stands this title is pretty much applicable to every single question on this site. \$\endgroup\$ – Mathieu Guindon Mar 22 '16 at 0:12
  • 1
    \$\begingroup\$ It's a nice question you've got here, but be prepared for answers that tell you to use prepared statements. \$\endgroup\$ – RubberDuck Mar 22 '16 at 7:23
  • 2
    \$\begingroup\$ I know how to use prepared statements, but i just don't want to use them because there is a slight performance penalty in my case. Did you measure it? Care to show your performance difference? Is it a bottleneck? \$\endgroup\$ – Bruno Costa Mar 22 '16 at 7:57
  • 1
    \$\begingroup\$ it's really difficult to fully understand howto write OOP style if you've never done before. I suggest you create a small app (or two) using an OOP framework like Symfony, while adhering to best practices there. It should give you a good introduction to (how they) structure OOP code. \$\endgroup\$ – JimL Mar 22 '16 at 9:30
  • 1
    \$\begingroup\$ If the only performance gain you can get is changing prepared statemens to that hex bullcrap, adding a server will be the better option. Micro optimisations like this will only leed to errors and slower code as the application grows. As others pointed out, to learn you should probably stick to a framework like Laravel or Symfony. \$\endgroup\$ – Pinoniq Apr 3 '16 at 23:07
3
\$\begingroup\$
  • never return from constructor
  • avoid having new statements in the constructor
  • on error, throw an exception instead of echo + exit
  • % is used in MySQL's LIKE syntax
  • you should refactor your DatabaseConnection::query() method - it's too high cyclomatic complexity
  • constants are global immutable state (it's not as bad as mutable globals, but still a bad idea)
  • you should be using spl_autoload_register instead of the archaic __autoload
  • you are mixing HTML with logic .. bad idea, try this approach instead
  • don't use ".class.php" in filenames, because then you will be screwed, when you start adding interfaces (which one would want to autoload too)
\$\endgroup\$
  • \$\begingroup\$ % is used in MySQL's LIKE syntax, You're absolutely right, forgot about that one. Ill just replace it with a question mark (as used in prepared statements also :P:P) \$\endgroup\$ – Ramon Bakker Mar 24 '16 at 14:48

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.