2
\$\begingroup\$

After moving from mysql_ to PDO I created a PDO handling function. Basically this function takes parameters, checks them and returns end result. The thing is that I created this ~1 year ago and haven't really optimized it and after looking at it again I wanted to see if I can improve it somehow, but right now I can't really think of anything. So I came here.

Note: Code is long, but after writing hundreds of MySQL request it saves some space.

function UltimatePDO($Type, $DB, $SeWhere = "", $SePointer = "", $Values = "", $Param = "", $UpPointer = "", $Extra = "")
{
    global $dbh;
    global $abc;

    if(empty($DB) || empty($Type))
    {
        PDOError("Not all parameters");

        return false;
    }

    if($Type == "INSERT")
    {
        if(!empty($Values) && !empty($Param))
        {
            $Lenght = count($Values);
            $Lenght1 = count($Param);

            if($Lenght == $Lenght1)
            {   
                $a = 1;
                $Values[0] = Replace($Values[0]);
                $Param[0] = Replace($Param[0]);
                $Rez = array(":$Values[0]" => "$Param[0]");
                $Rez1 = array();
                $Rez1 = array_merge($Rez1, $Rez);
                $New = ":$Values[0]";
                $In = implode(",", $Values);

                while($a < $Lenght)
                {
                    $Values[$a] = Replace($Values[$a]);
                    $Param[$a] = Replace($Param[$a]);
                    $New .= ",:$Values[$a]";
                    $Rez = array(":$Values[$a]" => "$Param[$a]");
                    $Rez1 = array_merge($Rez1, $Rez);

                    $a++;
                }

                try
                {
                    $stmt = $dbh->prepare("INSERT INTO $DB ($In) VALUES ($New)");   
                    $stmt->execute($Rez1);
                    $Affected = $stmt->rowCount();

                    if($Affected)
                    {
                        return true;
                    }
                    else
                    {
                        PDOError("Can't insert into DB");

                        return false;
                    }
                }
                catch (PDOException $e) 
                {
                    PDOError($e->getMessage());

                    return false;
                }
            }
            else
            {
                PDOError("Not all parameters");

                return false;
            }
        }
        else
        {
            PDOError("Not all parameters");

            return false;
        }
    }

    if($Type == "SELECT" || $Type == "COUNT")
    {
        if(!empty($SeWhere) && !empty($SePointer))
        {
            try
            {
                if($Type == "SELECT")
                {
                    $stmt = $dbh->prepare("SELECT * FROM $DB WHERE $SeWhere = :user $Extra");
                    $stmt->execute(array(":user" => $SePointer));

                    return $stmt->fetch(PDO::FETCH_ASSOC);
                }

                if($Type == "COUNT")
                {
                    $stmt = $dbh->prepare("SELECT COUNT(*) FROM $DB WHERE $SeWhere = :user");
                    $stmt->execute(array(":user" => $SePointer));
                    $count = $stmt->fetch(PDO::FETCH_NUM);

                    return $count[0];
                }
            }
            catch (PDOException $e) 
            {
                PDOError($e->getMessage());

                return false;
            }
        }
        else
        {
            PDOError("Not all parameters");

            return false;
        }
    }

    if($Type == "SELECT NO PARAM" || $Type == "COUNT NO PARAM")
    {
        try
        {   
            if($Type == "SELECT NO PARAM")
            {
                $stmt = $dbh->prepare("SELECT * FROM $DB $Extra");
                $stmt->execute();

                return $stmt->fetch(PDO::FETCH_ASSOC);
            }

            if($Type == "COUNT NO PARAM")
            {
                $stmt = $dbh->prepare("SELECT COUNT(*) FROM $DB $Extra");
                $stmt->execute();
                $Re = $stmt->fetch(PDO::FETCH_NUM);

                return $Re[0];
            }


        }
        catch (PDOException $e) 
        {
            PDOError($e->getMessage());

            return false;
        }
    }

    if($Type == "SELECT TABLE" || $Type == "SELECT TABLE NO PARAM")
    {
        if($Type == "SELECT TABLE")
        {
            if(!empty($SeWhere) && !empty($SePointer))
            {
                try
                {
                    $stmt = $dbh->prepare("SELECT * FROM $DB WHERE $SeWhere = :user $Extra");
                    $stmt->execute(array(":user" => $SePointer));

                    return $stmt->fetchAll(PDO::FETCH_ASSOC);
                }
                catch (PDOException $e) 
                {
                    PDOError($e->getMessage());

                    return false;
                }
            }
            else
            {
                PDOError("Not all parameters");

                return false;
            }
        }

        if($Type == "SELECT TABLE NO PARAM")
        {
            try
            {
                $stmt = $dbh->prepare("SELECT * FROM $DB $Extra");
                $stmt->execute();

                return $stmt->fetchAll(PDO::FETCH_ASSOC);
            }
            catch (PDOException $e) 
            {
                PDOError($e->getMessage());

                return false;
            }
        }
    }

    if($Type == "UPDATE")
    {

        if(!empty($Values) && !empty($Param) && !empty($UpPointer))
        {
            $Lenght = count($Values);
            $Lenght1 = count($Param);

            if($Lenght == $Lenght1)
            {
                $Set = array();
                $Rez = array();
                $Values[0] = Replace($Values[0]);
                $Param[0] = Replace($Param[0]);
                $First = array("$Values[0] = :$Values[0]");
                $First1 = array(":$Values[0]" => "$Param[0]");
                $Rez = array_merge($Rez, $First);
                $Set = array_merge($Set, $First1);
                $MakeID = array(":PDOUniqueId" => "$UpPointer[1]");

                $a = 1;

                while($a < $Lenght)
                {
                    $Values[$a] = Replace($Values[$a]);
                    $Param[$a] = Replace($Param[$a]);
                    $First = array("$Values[$a] = :$Values[$a]");
                    $Rez = array_merge($Rez, $First);
                    $First1 = array(":$Values[$a]" => "$Param[$a]");
                    $Set = array_merge($Set, $First1);

                    $a++;
                }

                $Final = implode(",", $Rez);
                $Bound = array_merge($Set, $MakeID);
                $Id = "$UpPointer[0]";

                try
                {
                    $stmt = $dbh->prepare("UPDATE $DB SET $Final WHERE $Id = :PDOUniqueId");
                    $stmt->execute($Bound);

                    $Affected = $stmt->rowCount();

                    if($Affected)
                    {
                        return true;
                    }
                    else
                    {
                        PDOError("Can't update DB");

                        return false;
                    }
                }
                catch (PDOException $e) 
                {
                    PDOError($e->getMessage());

                    return false;
                }
            }
            else
            {
                PDOError("Not all parameters");

                return false;
            }

        }
        else
        {
            PDOError("Not all parameters");

            return false;
        }

    }

    if($Type == "DELETE")
    {
        if(!empty($SeWhere) && !empty($SePointer))
        {
            try
            {
                $stmt = $dbh->prepare("DELETE FROM $DB WHERE $SeWhere = :user");
                $stmt->execute(array(":user" => $SePointer));

                $Affected = $stmt->rowCount();

                if($Affected)
                {
                    return true;
                }
                else
                {
                    PDOError("Can't delete from DB");

                }
            }
            catch (PDOException $e) 
            {
                PDOError($e->getMessage());

                return false;
            }

        }
        else
        {
            PDOError("Not all parameters");

            return false;
        }

    }

    PDOError("Unknown type of request");

    return false;
}

The function handles basic request such as update, insert, delete, select, count. In code it looks like this:

All values comes in array() form, which seemed like easiest solution:

UltimatePDO("UPDATE", "DB", "", "", array("Column"), array("Value"), array("hash", $user));

As for function Replace(), it just replaces unique symbols.

\$\endgroup\$
8
\$\begingroup\$

Poor cohesion

It's called logical cohesion when a routine that does multiple things of different logic. Your function will either insert records, or select records, or update records. These are very different operations, and their implementations should be in separate functions.

If you refactored this way, it would be much better:

if ($Type == "INSERT") performInsert(...)
elseif ($Type == "SELECT") performSelect(...)
elseif ($Type == "UPDATE") performUpdate(...)
// ...

In this form, by using an if-else chain to decide the next action and delegating to appropriate specialized functions, this function will have functional cohesion, which is the best kind of cohesion, characterized by a routine having a single purpose, in this case orchestrating database operations.

Mutually exclusive if

Notice that in the previous point I chained the conditions with elseif. In your code there are many if conditions that cannot happen together, so they should have been elseif.

Poor naming

I think, the common convention is to only use PascalCase for class names in PHP. Function names and variable names are usually either snake_case or camelCase. See also this related post.


It seems that $Param is expected to be an array, just like $Values. To indicate this fact better, I suggest making it plural, to $Params.


There are several problems with these names:

$Lenght = count($Values);
$Lenght1 = count($Param);
  • The correct spelling is "length"
  • Numbering in variable names is considered bad practice
  • The "first" length being $Lenght, and the "second" length being $Lenght1 is just odd

I suggest renaming like this instead:

$valueCount = count($Values);
$paramCount = count($Param);

You should find better alternatives for the other numbered variables too, like $Rez1.

\$\endgroup\$
  • \$\begingroup\$ Thanks I'll try to change my code. And could you explain a bit more why numbering in variables names is bad practice? \$\endgroup\$ – iCoRe Jun 14 '15 at 8:27
  • 1
    \$\begingroup\$ Because the numbers don't really mean anything, and usually you can replace them with more meaningful names, like in this example. It's rare to see in practice numbered variables that are well justified. I can't recall or make up a single example where numbering can be forgiven. As much as possible, try to find better, descriptive names. \$\endgroup\$ – RoboSanta Jun 14 '15 at 12:24
  • \$\begingroup\$ I see, now that I think about it numbering probably will get confusing in a long run. \$\endgroup\$ – iCoRe Jun 14 '15 at 12:28
3
\$\begingroup\$

Usability

Your function will be extremely hard to use for anyone not familiar with the actual code (and in a month or two, that will probably include you as well). They only see this:

UltimatePDO($Type, $DB, $SeWhere = "", $SePointer = "", $Values = "", $Param = "", $UpPointer = "", $Extra = ""

There are quite a lot of variables, which makes it hard to remember their order and their meaning. The variable names also do not make it obvious what they represent, so there really is no chance of a user using it correctly without reading all the code.

You can try to help a user of your function with a PHPDoc style comment, but still, it will be hard to use.

To try to fix this, you should at least split the function into multiple functions. There's really not enough similarity between eg insert and select to put them both in the same function (this is why there are so many arguments, most of which would have to be set to null, because they are not needed for the desired query).

But generally, a wrapper like yours is not really recommended. PDO already has a neat interface which people are used to. Your function doesn't make it a lot easier to use, and people would first need to learn it. It also only provides a very limited subset of the PDO functionality. What if I want to insert multiple things? The query will be prepared each time, which is bad for performance. What if I want to join something? Or only select the columns I need?

If you still want a wrapper, you should look at the interface of existing code. A commonly used interface might look like this: getPDO()->select('column')->from('db')->where('column = :value')->setParam(':value', 'foo'). This is a lot easier to use than your code, because I don't have to remember the order of a bunch of arguments. An IDE will even auto-complete the function names.

Security

It is very unclear for a user which values can contain user input, and which cannot (these are actually most, eg $Extra, $DB, $SeWhere, $UpPointer, etc), which will cause security issues later on.

At the very least this should be well documented in PHPDoc comments.

Personally, I would probably not write a function which can easily be vulnerable if "wrongly" used. To solve this, you could add mandatory whitelist parameters for those values that cannot be prepared (and get rid of $Extra, which is way too broad and basically cannot be used securely).

global

global is often a code smell. It makes code harder to use, harder to test, etc. Just pass needed parameters as arguments.

\$\endgroup\$
  • \$\begingroup\$ Thanks for your suggestions. Firstly I use this completely for myself and I tend to remind how things which I wrote works, so not really a problem about complexity :D On the other hand, I like how that interference might look, its simple, so if you know any good existing interferences, could you link any of them? \$\endgroup\$ – iCoRe Jun 14 '15 at 12:16
  • \$\begingroup\$ @iCoRe the keyword you want to google for is query builder. Most frameworks have them, eg laravel or the doctrine ORM. \$\endgroup\$ – tim Jun 14 '15 at 12:27
  • \$\begingroup\$ I'll look into that \$\endgroup\$ – iCoRe Jun 14 '15 at 12:31

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.