Jump to content

Is it considered bad practice to pass SQL script?


NotionCommotion

Recommended Posts

For example, would you frown upon this script regarding how SQL is passed?


class Foo
{
    public function method1($value)
    {
        $sql='SELECT xyz FROM mytable WHERE id=?';
        $this->otherMethod($sql,$value);
    }
    public function method2($value)
    {
        $sql='SELECT xyz FROM mytable WHERE fk=? LIMIT 1';
        $this->otherMethod($sql,$value);
    }
    protected function otherMethod($sql,$value)
    {
        $stmt = $this->db->prepare($sql);
        $stmt->execute([$value]);
        if($rs=$stmt->fetch(\PDO::FETCH_OBJ)) {
            // Do other taks
        }
        else {
            // respond with error
        }
    }
}
Link to comment
Share on other sites

I don't see a specific problem with passing the string value in that example. But, as it is written, there are two public methods that take a value parameter and both call the protected method to prepare the query and apply the value when executing.

 

Personally, I don't like that approach because the protected function will only work if there is one, and only one, value to be applied to the query. At the very least, I would have the public methods pass the value (or values) as an array. That way, the otherMethod() function will work with queries with one or multiple values.

Link to comment
Share on other sites

It's impossible to tell from your foobar example class. If this class is specifically designed as a PDO wrapper, the code can make a lot of sense. Otherwise it's probably wrong.

I thought you'd appreciate it :)  The only purpose is to eliminate duplicated script common to the methods that call it, and the class is not specifically designed to be a PDO wrapper.  It seemed a bit wrong to me, but I didn't know why.  Why do you think it is?

 

I don't see a specific problem with passing the string value in that example. But, as it is written, there are two public methods that take a value parameter and both call the protected method to prepare the query and apply the value when executing.

 

Personally, I don't like that approach because the protected function will only work if there is one, and only one, value to be applied to the query. At the very least, I would have the public methods pass the value (or values) as an array. That way, the otherMethod() function will work with queries with one or multiple values.

 

Maybe not very OOPish, but otherMethod() is specifically designed for one task and will only (at least today) accept one argument.  I do see your reasoning, however, of using an array, and I will likely do so.

 

Thank you both

Link to comment
Share on other sites

I thought you'd appreciate it :)

 

I'm not joking. When you provide bad example code, you're likely to get bad advice. Garbage in, garbage out.

 

A PDO helper method belongs into a PDO helper class where it can be used everywhere. Putting it into some other class means you'll quickly end up with dozens of specialized helper methods all over the place. So you'll eventually create more duplicate code, not less.

class Database
{
    protected $connection;

    public function __construct($connection)
    {
        $this->connection = $connection;
    }

    public function execute($query, $parameters = [])
    {
        if ($parameters == [])
        {
            return $this->connection->query($query);
        }
        else
        {
            $stmt = $this->connection->prepare($query);
            $stmt->execute($parameters);

            return $stmt;
        }
    }
}
Link to comment
Share on other sites

Thanks Jacques,

 

I've typically don't use a PDO wrapper, but probably should do so more often.  Do you have many other methods in your wrapper which you use often?  EDIT: Do you recommend creating the wrapper in a parent's constructor and assigning it to $this, and then using it as $this->PDOWrapper->execute('SELECT bla bla',[1,2,3]);?

 

The part I was talking about not wanting to duplicate is the below part.  Probably shouldn't delegate to a wrapper, right?  I suppose I should do the query in the parent (with a wrapper to make it more concise) and pass the results to the child method and if the argument is false, deal with the error.

        if($rs=$stmt->fetch(\PDO::FETCH_OBJ)) {
            // Do other tasks
        }
        else {
            // respond with error
        }

I'm not joking.

 

From your expression, I didn't think so.

Edited by NotionCommotion
Link to comment
Share on other sites

I've typically don't use a PDO wrapper, but probably should do so more often.  Do you have many other methods in your wrapper which you use often?  EDIT: Do you recommend creating the wrapper in a parent's constructor and assigning it to $this, and then using it as $this->PDOWrapper->execute('SELECT bla bla',[1,2,3]);?

 

Yes, but you shouldn't create the wrapper in the constructor, you should receive it as an argument.

 

There I plenty of useful methods I could think of. In your case, a fetchOne() method (a combined prepare/execute/fetch) will effectively eliminate all database-related duplicate code. You can then concentrate on the // do other task.

Link to comment
Share on other sites

What he's referring to is known as Dependency Injection. Basically it boils down to having some component of your application be responsible for constructing all your various objects and giving each of those objects whatever they need to function properly as arguments to the constructor (or via setter methods or whatever).As you've noticed before, the Slim framework you've been using handles this by using Pimple.

 

So what you do is define your Foo class as something like this:

class Foo {
   private $db;
   public function __construct(\PDO $db){
      $this->db = $db;
   }
} 
Then configure Pimple to construct that service when it is requested.

$container['foo'] = function($container){
   return new Foo($container['db']);
};
Notice we pass the DB service to the instance of Foo that is created. This means you also have to define your DB service on the container, such as

$container['db'] = function(){
   return new PDO('mysql:host=localhost;dbname=test', 'user', 'pass');
};
Now when you try and access your foo service with $container['foo'] pimple will first get an instance of the DB service then construct your Foo object and give it an instance of the DB service as the first constructor parameter.
Link to comment
Share on other sites

This thread is more than a year old. Please don't revive it unless you have something important to add.

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.