4
\$\begingroup\$

By any means, if any can tell me if this code can be more optimized than it already is, please say so. It's a select statement to my PDO class. When I glare over my code, I'm certain there is a lot of easy improvements.

    public function select($table, $where = array(), $Innerjoin = array(), $group = array(), $selects = array()) {

        $vowels = array("a", "e", "i", "o", "u", "y", "A", "E", "I", "O", "U","Y");

        $select = "";
        if(empty($selects)){
            $select .= 'SELECT *';
        } else {
            foreach($selects as $key => $value){
                $select == "" ? $select = "SELECT " : $select .= " , ";

                $select .= str_replace($vowels, "" , $key) . "." . $value;
            }
        }

        $sql = $select . " FROM `" . $table . "` AS "  . str_replace($vowels, "" , $table);

        if(!empty($Innerjoin)){
            $join = "";
            foreach ($Innerjoin as $joinKey => $joinValue) {
                foreach ($joinValue as $key => $value) {
                    $join .= " INNER JOIN `" . $joinKey . "` AS " . str_replace($vowels, "" , $joinKey) . " ON ";
                    if(is_array($value)){
                        foreach ($value as $ids => $id) {
                            $join .= str_replace($vowels, "" , $ids)  . "." . $key . " = " . str_replace($vowels, "" , $joinKey) . "." . $id;
                        }
                    } else {
                        $join .= str_replace($vowels, "" , $table) . "." . $key . " = " . str_replace($vowels, "" , $joinKey) . "." . $key;
                    }
                }
            }
            $sql .= $join;
        }


        if(!empty($where)){
            $qWhere = "";
            foreach($where as $key => $value){
                foreach($value as $operator => $attribute){
                    $qWhere == "" ? $qWhere = " WHERE " : $qWhere .= " AND ";

                    $qWhere .= "" . str_replace($vowels, "" , $table) . "." . $key . " " . $operator . " '" . $attribute . "'";
                }
            }
            $sql .= $qWhere;
        }

        if(!empty($group)){
            $on = "";
            foreach($group as $key => $value){
                $on == "" ? $on = " GROUP BY (" : $on .= " , ";

                $on .= str_replace($vowels, "" , $key) . "." . $value;
            }
            $on .= ")";
            $sql .= $on;
        }
        return $this->action($sql);
    }


    public function delete($table, $where) {
        $sql = "DELETE FROM {$table} ";
        if(!empty($where)){
            $qWhere = "";
            foreach($where as $key => $value){
                foreach($value as $operator => $attribute){
                    $qWhere == "" ? $qWhere = " WHERE " : $qWhere .= " AND ";

                    $qWhere .= "" . str_replace($vowels, "" , $table) . "." . $key . " " . $operator . " '" . $attribute . "'";
                }
            }
            $sql .= $qWhere;
        }
        return $this->action($sql);
    }

The way I call this is

    public function Show() {
        $output = '';

        $where = array(
            'user_id' => array(
                '!=' => '0'
            )
        );

        $join = array(
            'users_profile' => array(
                'user_id' => 'user_id'
            ),
            'users_information' => array(
                'user_id' => 'user_id'
            ),
            'rightsgroup' => array(
                'rightsgroup_id' => 'rightsgroup_id'
            ),
            'users_accounting' => array(
                'user_id' => 'user_id'
            ),
            'accounting_groups' => array(
                'salary_id' => array(
                    'users_accounting' => 'id'
                )
            )

        );

        $DataUsers = $this->_db->select('users', $where, $join, "");
        if($DataUsers->count()) {
            $resultsUsers = $DataUsers->results();
            foreach ($resultsUsers as $userS) {
                $output .= '<tr>';
                $output .= '<td>' . $userS->firstname .'</td>';
                $output .= '<td>' . $userS->lastname .'</td>';
                $output .= '<td>' . $userS->username . '</td>';
                $output .= '<td>' . $userS->district . '</td>';
                $output .= '<td>' . $userS->groupname . '</td>';
                $output .= '<td>' . $userS->accountingname . '</td>';
                $output .= '<td><a href="?page=medarbejder&view='. $userS->user_id .'">View</td>';
                $output .= '</tr>';
            }
        }
        return $output;
    }
\$\endgroup\$
  • 5
    \$\begingroup\$ You should give a little more context about your code. What's the real goal of this code, it's a simple select statement but in what context. In the first reading of the code, it's look like you're concatenating strings for a sql statement, this is sensible to SQL injection. \$\endgroup\$ – Marc-Andre May 26 '14 at 13:55
  • \$\begingroup\$ Thank you for the comment Marc-Andre: This is a part of my PDO class at it's current stage. It's a select statement to type out information's about the employees, etc. About the code, then it's a site i began to make recently for a few friends of mine. and i have yet to improved the code to be secure, but my main task at this current moment have been to optimize it, so it didn't run so slow, and to make it possible to do joins, multiple wheres and such. Which it had not yesterday. \$\endgroup\$ – Nquit May 27 '14 at 14:27
5
\$\begingroup\$

As you use $vowels in several different methods, let's start by making that a static member of your class:

class PDO
{
    private static $vowels = array("a", "e", "i", "o", "u", "y", "A", "E", "I", "O", "U", "Y");
}

You also use this twice in a foreach loop:

str_replace(self::$vowels, "", $key) . "." . $value;

And manually implode some queries with it. To make it generic, have a method which removes vowels in this way, such as:

private static function removeVowels(array $array)
{
    $removed = array();
    foreach($array as $key => $value)
        $removed[] = str_replace(self::$vowels, '', $key) . '.' . $value;
    return $removed;
}

With this, you can remove the first if/else and foreach loop entirely, by removing the ternary and using implode(), like so:

$select = 'SELECT ' . (empty($selects) ? '*' : implode(', ', self::removeVowels($selects)));

or even removing the select variable altogether and just collapsing it into your $sql:

$sql = 'SELECT ' . (empty($selects) ? '*' : implode(', ', self::removeVowels($selects))) .
       ' FROM `' . $table . '` AS ' . str_replace(self::$vowels, '', $table);

And using the same style, you can reduce your Group By block to:

if(!empty($group))
{
    $sql .= " GROUP BY (" . implode(', ', self::removeVowels($group)) . ')';
}

You also use this twice:

str_replace(self::$vowels, "", $table) . "." . $key . " " . $operator . " '" . $attribute . "'"

Which can be separated out in the same way:

private static function removeVowelsWhere(array $array, $table)
{
    $removed = array();
    foreach($array as $key => $value)
    {
        foreach($value as $operator => $attribute)
        {
            $removed[] = str_replace(self::$vowels, "", $table) . "." . $key . " " . $operator . " '" . $attribute . "'";
        }
    }
    return $removed;
}

To simplify your where blocks to:

if(!empty($where))
{
    $sql .= " WHERE " . implode(' AND ', self::removeVowelsWhere($where, $table));
}
\$\endgroup\$
  • \$\begingroup\$ Thank you for the rewiew, i'll look into it, and change it.. Really alot of helpfull stuff \$\endgroup\$ – Nquit May 27 '14 at 13:09

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.