PHP practing a better code

Question

I've been wondering about this for a few weeks. My code practice is to create several classes (insert, update, delete, etc) and then create functions inside of that.

The problem is that for every function I do something like:

public function clients(){
    $db = new mysqli($this->host, $this->user, $this->pass, $this->db);
    if ($db->connect_errno) {
        printf("Connecting error" . $db->connect_error);
        return false;
        exit();
    }
    $db->set_charset("utf8");

    $visibile = true;

    $query = $db->prepare("SELECT * FROM clients WHERE visible = ?");
    $query->bind_param("i", $visible);  
    $query->execute();
    $query->bind_result($id, $name, $address, $photo);
    $query->store_result();
    $rows = array();

    while($query->fetch()){

        $rows[] = array("id" => $id, "name" => $name, "address" =>  $address, "photo" => $photo);
    }

    $query->close();
    $db->close();

    return $rows;
}

I've been thinking about reduce this code..at least the connection in the beginning of every function.

// currently call
include_once('select.php');
$select = new select();

$rows = $select->clients();

Something better would be

$select = new select();
$rows = $select->connect()->clients();

Or even better

$rows = $connect->select()->clients();

I know that this is possible, I just don't know how. Or there's even a better approach than this one?

Having classes whose names are verbs is a definite sign you're structuring things oddly. What data and methods are you encapsulating in "Select" vs. "Update"? — Wooble, Feb 21 '14 at 15:19
Seconding @Wooble. Your class structure is certainly odd, but getting you started on DI principles would be a good first step. — deceze, Feb 21 '14 at 15:21
Why is that odd? Is more simpler to read `$select->clients()` and `$insert->clients($name)` than `$variable->insert_clients()`. — user3243925, Feb 21 '14 at 15:59
IMO that's a common trap to fall into. If you structure your classes in a way that *reads good* when calling them, you'll often end up with weirdness. The logical structure should typically be to group by the topic first, not by the action. E.g. `$users->getAll()`, `$users->save($newUser)` etc. One class is responsible for anything user related, one class is responsible for anything posts related etc. You often have many more actions you can do with your things than the other way around. Will you have `$sendReminderEmailIfExpired->clients()` as well...? — deceze, Feb 21 '14 at 16:24
I see your point and it's a valid one. To answer your question: no. I've another class called: `Notifications`. So, `$notification->confirmation_email()` is well perceptible. — user3243925, Feb 21 '14 at 16:40

score 1 · Accepted Answer · answered Feb 21 '14 at 15:17

1

Dependency injection!

$db     = new mysqli(...);
$select = new Select($db);
$select->clients();

class Select {

    protected $db;

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

    public function clients() {
        $this->db->...
        ...
    }

}

The advantages are obvious, hopefully. You can share a single $db instance among not just all your methods, but all your classes as well. It also doesn't hardcode the database specifics inside the class anymore.

answered Feb 21 '14 at 15:17

deceze

510,633
85
743
889

1

I'll stop writing my answer, because this is pretty much what I would say. For convenience, however, I would recommend OP make a `Database` class that candles creating the `mysqli` connection. So you would say `$db = new Database(); $select = new Select($db);` – Sam Feb 21 '14 at 15:19
1

@Sam Depends whether you also want to *abstract* your database connector, but yes, certainly not a bad idea. Maybe a factory would do too. – deceze Feb 21 '14 at 15:20

score 0 · Answer 2 · answered Feb 21 '14 at 15:18

0

You should make the connection first above the class, inject it in your class. Then in the constructor you bind the connection to a var. Finally you can use the connection in any function in your class.

class Select {
    public function __construct(mysqli $db) {
        $this->db = $db;
    }
    public function clients() {
        $query = $this->db->prepare();
    }
}

$db = new mysqli(); // Make connection

$class = new Select($db); // Pass the database variable into the class

answered Feb 21 '14 at 15:18

vonUbisch

1,384
17
32

Making connection for every page? I don't think that's the best solution. See the answer I marked as correct. – user3243925 Feb 21 '14 at 16:00
Ofcourse not, that's not what I'm suggesting, it should be set higher from where your class is started. It's exactly the same answer as the accepted one. – vonUbisch Feb 21 '14 at 17:24
You are right, I've read again the answer and it's the same. Althought I don't want that. I want to be able to, in the `__construct()`, initialize the mysqli. – user3243925 Feb 21 '14 at 18:35

score 0 · Answer 3 · edited May 23 '17 at 10:26

I'll go with a Repository Pattern (with Dependency Injection). You'll find more information there.

The goal being to have a class usable by any other business logic class which offers an easy way to persist and retrieve objects.

You'll end up having :

class Database
{
    public function select(...)
    ...
}

And your repository (where the Database is used)

class ClientsRepository // probably : implements RepositoryInterface
{
    protected $db;

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

    public function findAll()
    {
        // use $this->db to select *
    }
}

PHP practing a better code

3 Answers3