4
\$\begingroup\$

I'm uncertain about proper OOP design since my teachers are explaining OOP very vaguely. Do I need to use the constructor and the mutators like this or am I better off removing the constructor and passing the information when the methods are called?

This is currently my dynamic sql class(extends the db connection)

class dbMainExec
{

    private $_db;

    private $_table;
    private $_column;
    private $_where;
    private $_join;

    private $_errors = array();
    private $_stmt;
    private $_results;
    private $_countResults;

    public function __construct($table)
    {
        $this->_db = new dbConn();
        $this->setTable($table);
    }

    public function setTable($table)
    {
        $table = strtolower($table);
        $this->_table = $table;
    }

    public function getTable()
    {
        return $this->_table;
    }

    public function setColumn($column)
    {
        if (is_array($column)) {
            $this->_column = $column;
        } else {
            $this->_errors[] = "Columen is geen array!";

        }
    }

    public function getColumn()
    {
        return $this->_column;
    }

    public function setWhere($where)
    {

        if (is_array($where) || empty($where)) {
            $this->_where = $where;
        } else {
            $this->_errors[] = "where is geen array!";

        }
    }

    public function getWhere()
    {
        return $this->_where;
    }

    public function setJoin($join)
    {
        if (is_array($join) || empty($join)) {
            $this->_join = $join;
        } else {
            $this->_errors[] = "join is geen array!";

        }
    }

    public function getJoin()
    {
        return $this->_join;
    }


    public function setResults($results)
    {
        if (is_array($results)) {
            $this->_results = $results;
        } else {
            $this->_errors[] = "Fout bij resultaten";

        }
    }

    public function getResults()
    {
        return $this->_results;
    }

    public function setCountResults($countResults)
    {
        if (is_numeric($countResults)) {
            $this->_countResults = $countResults;
        } else {
            $this->_errors[] = "fout bij tellen resultaten";

        }
    }

    public function getCountResults()
    {
        return $this->_countResults;
    }

    public function setStatement($stmt)
    {
        $this->_stmt = $stmt;
    }

    public function getStatement()
    {
        return $this->_stmt;
    }

    public function execute($sql,$select = false)
    {

        $this->setStatement($this->_db->connect()->prepare($sql));



        if (!empty($this->getColumn()) && $select == FALSE ) {
            foreach ($this->getColumn() as $singleColumn => $columnValue) {

                $this->getStatement()->bindValue(':' . $singleColumn, $columnValue);
            }
        }


        if (!empty($this->getWhere())) {
            foreach ($this->getWhere() as $singleWhere => $whereValue) {

                $this->getStatement()->bindValue(':' . $singleWhere, $whereValue);
            }
        }

        if (!empty($this->getJoin())) {
            foreach ($this->getJoin() as $singleJoin => $joinValue) {
                $this->getStatement()->bindValue(':' . $singleJoin, $joinValue);
            }
        }

        $this->getStatement()->execute();


        $this->setColumn(array());
        $this->setWhere(array());
        $this->setJoin(array());

    }

    public function selectRow($column = null, $where = null, $join = null)
    {

        $this->setColumn($column);
        $this->setWhere($where);
        $this->setJoin($join);


        if ($this->_errors) {
            var_dump($this->_errors);
            die();
        }

        $sql = "SELECT " . implode($this->getColumn(), ',') . " FROM " . $this->getTable() . " ";

        if (!empty($this->getJoin())) {
            foreach ($this->getJoin() as $extraTable => $extraValues) {
                $sql .= "JOIN $extraTable";
                foreach ($extraValues as $singleValue => $singleTarget) {
                    $sql .= " ON {$singleValue} = {$singleTarget} ";
                }
            }
        }

        if (!empty($this->getWhere())) {
            $sql .= "WHERE ";
            $arrayKeys = array_keys($this->getWhere());
            foreach ($this->getWhere() as $option => $value) {
                if ($option != end($arrayKeys)) {
                    $sql .= $option . " = :{$option} AND ";
                } else {
                    $sql .= $option . " = :{$option}";
                }
            }
        }

        $this->execute($sql,$select = TRUE);


        $this->setResults($this->getStatement()->fetch(PDO::FETCH_ASSOC));
        $this->setCountResults($this->getStatement()->rowCount());

    }

    public function selectMultiple($column = null, $where = null, $join = null)
    {


        $this->setColumn($column);
        $this->setWhere($where);
        $this->setJoin($join);

        if ($this->_errors) {
            var_dump($this->_errors);
            die();
        }

        $sql = "SELECT " . implode($this->getColumn(), ',') . " FROM " . $this->getTable() . " ";
 
        if (!empty($this->getJoin())) {
            foreach ($this->getJoin() as $extraTable => $extraValues) {
                $sql .= "JOIN $extraTable";
                foreach ($extraValues as $singleValue => $singleTarget) {
                    $sql .= " ON {$singleValue} = {$singleTarget} ";
                }
            }
        }


        if (!empty($this->getWhere())) {
            $sql .= "WHERE ";
            $arrayKeys = array_keys($this->getWhere());
            foreach ($this->getWhere() as $option => $value) {
                if ($option != end($arrayKeys)) {
                    $sql .= $option . " = :{$option} AND ";
                } else {
                    $sql .= $option . " = :{$option}";
                }
            }
        }


        $this->execute($sql, $select = TRUE);

        $this->setResults($this->getStatement()->fetchAll(PDO::FETCH_ASSOC));
        $this->setCountResults($this->getStatement()->rowCount());

    }

    public function insert($column = null)
    {
        $this->setColumn($column);

        if ($this->_errors) {
            var_dump($this->_errors);
            die();
        }

        $sql = "INSERT INTO {$this->getTable()} ( " . implode(', ', array_keys($this->getColumn())) . ") VALUES (";
        $arrayKeys = array_keys($this->getColumn());
        foreach ($this->getColumn() as $singleColum => $singleValue) {
            $sql .= ":" . $singleColum;
            if ($singleColum != end($arrayKeys)) {
                $sql .= ", ";
            }

        }
        $sql .= ')';
        $this->execute($sql);
    }

    public function update($column = null, $where = null)
    {

        $this->setColumn($column);
        $this->setWhere($where);


        if ($this->_errors) {
            var_dump($this->_errors);
            die();
        }

        $sql = "UPDATE {$this->getTable()} SET ";


        $arrayKeys = array_keys($this->getColumn());
        foreach ($this->getColumn() as $singlecolum => $singleValue) {
            $sql .= $singlecolum . " = :" . $singlecolum;
            if ($singlecolum != end($arrayKeys)) {
                $sql .= ", ";
            }
        }


        if (!empty($this->getWhere())) {
            $sql .= " WHERE ";
            $arrayKeys = array_keys($this->getWhere());
            foreach ($this->getWhere() as $option => $value) {
                if ($option != end($arrayKeys)) {
                    $sql .= $option . " = :{$option} AND ";
                } else {
                    $sql .= $option . " = :{$option}";
                }

            }
        }

        $this->execute($sql);
    }

    public function delete($where = null)
    {
     
        $this->setWhere($where);


        if ($this->_errors) {
            var_dump($this->_errors);
            die();
        }

        $sql = "DELETE FROM {$this->getTable()}";
        if (!empty($this->getWhere())) {
            $sql .= " WHERE ";
            $arrayKeys = array_keys($this->getWhere());
            foreach ($this->getWhere() as $option => $value) {
                if ($option != end($arrayKeys)) {
                    $sql .= $option . " = :{$option} AND ";
                } else {
                    $sql .= $option . " = :{$option}";
                }

            }
        }

        $this->execute($sql);
    }

}
\$\endgroup\$
2
  • \$\begingroup\$ This is a good question, and the code deserves to be reviewed, but we generally don't answer questions that require an opinion on design. Can you change the introductory paragraph and can you remove the commented out code? \$\endgroup\$ Commented Nov 23, 2017 at 12:48
  • \$\begingroup\$ Welcome to Code Review! This question is incomplete. To help reviewers give you better answers, please edit to add sufficient context to your question. The more you tell us about what your code does, the easier it will be for reviewers to help you. Questions should include a description of what the code does. \$\endgroup\$ Commented Nov 23, 2017 at 14:01

1 Answer 1

Reset to default
2
\$\begingroup\$

Potential for SQL injection

Before getting to the main question I’d like to mention the code is vulnerable to SQL injection- possibly via methods like update(). It is explained in this article An SQL injection against which prepared statements won't help (written by @YourCommonSense).

"placeholders are used and data is safely bound, therefore our query is safe". Yes, the data is safe. But in fact, what does this code do is taking user input and adding it directly to the query. Yes, it's a $key variable. Which goes right into your query untreated.

Which means you've got an injection. 1

While it is up to the calling code to provide fields to update, where conditions etc. this code cannot prevent calling code from passing input straight from the client side. As the article explains later on that one can attempt to make a whitelist of fields that can be updated but for this central code that doesn’t seem feasible. However using backtick delimiters will help but then delimiters inside the name need to be escaped

As it was said before, escaping delimiters would help. Therefore, your first level of defense should be escaping delimiters (backticks) by doubling them. Assembling your query this way,

$setStr .= "`".str_replace("`", "``", $key)."` = :".$key.",";

you will get the injection eliminated. 2

Main question

Do I need to use the constructor and the mutators like this or am I better off removing the constructor and passing the information when I call the methods?

Bear in mind that there are SQL statements that don't require a table - e.g.

SELECT 1

see this sqlFiddle. If statements like this should be supported then it would be wise to make the $table parameter either optional or else eliminated.

Other review points

tracking errors

Let's look at the setColumn() method:

public function setColumn($column)
{
    if (is_array($column)) {
        $this->_column = $column;
    } else {
        $this->_errors[] = "Columen is geen array!";

    }
}

if $column is not an array, then a string is added into the _errors property, which is checked during another method like selectRow(), selectMultiple(), insert(), update(), delete(). Instead of depending on the calling code to call one of those other methods before the error is realized, an Exception can be thrown immediately.

public function setColumn($column)
{
    if (!is_array($column)) {
        throw new Exception("Columen is geen array!");
    }
    $this->_column = $column;
}

This has multiple advantages:

  • it is easier to track the flaw from the calling code
  • the calling code could potentially recover from the exception
  • the else keyword can be avoided
  • indentation levels are reduced
  • there is no need for the die() statement

method selectRow()

public function selectRow($column = null, $where = null, $join = null)
    {

    $this->setColumn($column);

The default value for argument $column is null. Since this method calls setColumn and that method checks if its argument $column is not an array, then an error would be tracked. Perhaps instead of using a default value of null, it would be better to either set the default value to an empty array or not use a default value.

repeated code

A commonly accepted principle is the Don't Repeat Yourself. principle. There are places where blocks of code are repeated - e.g. the following block appears to be repeated within selectRow(), selectMultiple(), update(), delete().

if (!empty($this->getWhere())) {
    $sql .= "WHERE ";
    $arrayKeys = array_keys($this->getWhere());
    foreach ($this->getWhere() as $option => $value) {
        if ($option != end($arrayKeys)) {
            $sql .= $option . " = :{$option} AND ";
        } else {
            $sql .= $option . " = :{$option}";
        }
    }
}

It would be simpler to abstract that logic into a method. Also, the conditional to add AND except for the end can be eliminated by adding the conditions to an array and combining them with the implode() function. Also the variable $option can be moved into the string literal

if (!empty($this->getWhere())) {
    $conditions = [];
    foreach ($this->getWhere() as $option => $value) {
        $conditions[]= . "$option = :{$option}"; 
    }
    $sql .= "WHERE " . implode(" AND ", $conditions);
}

which allows for simplifications using array_map():

if (!empty($this->getWhere())) {
    $conditions = array_map(function($option) {
        return "$option = :{$option}"; 
    }, array_keys($this->getWhere()));
    $sql .= "WHERE " . implode(" AND ", $conditions);
}

And hopefully the server is running PHP 7.4 or later since currently there is LTS for version 7.4+, arrow functions can be used to simplify the mapping:

if (!empty($this->getWhere())) {
    $conditions = array_map(fn($option) => “$option = :{$option}", array_keys($this->getWhere()));
    $sql .= "WHERE " . implode(" AND ", $conditions);
}
\$\endgroup\$
0

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.