Is it considered bad practice to pass SQL script?

[NotionCommotion]

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
        }
    }
}

[Jacques1]

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.

[Psycho]

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.

[NotionCommotion]

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

[Jacques1]

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;
        }
    }
}

[NotionCommotion]

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 September 2, 2016 by NotionCommotion

[Jacques1]

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.

[NotionCommotion]

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

 

 

Not sure I understand.  Mind giving an example?  Thank you

[kicken]

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.

[NotionCommotion]

Thanks Kicken,

 

Still a little confused. I am going to start a new post specific to Dependency Injection.

[gizmola]

This series of articles about DI was written by Fabien Potencier, who is the originator of the Symfony frameworks(s). I highly recommend reading through it prior to making a thread on the subject here.