current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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;
    }
share|improve this question
5  
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. –  Marc-Andre May 26 at 13:55
    
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. –  Nquit May 27 at 14:27

1 Answer 1

up vote 5 down vote accepted

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));
}
share|improve this answer
    
Thank you for the rewiew, i'll look into it, and change it.. Really alot of helpfull stuff –  Nquit May 27 at 13:09

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.