Pulling people out of a database

Question

I have this class that pulls people out of a database. I want it to be a little prettier and function better.

<?php
class comps extends db_config {

    public $output = array();
    private $arr = array();
    public $area;
    public $me;
    public $options = array();

    public function __construct() {
        $this->pdo = new PDO('mysql:host='.$this->_host.';dbname='.$this->_db.';charset='.$this->_charset, $this->_user, $this->_pass);
    }

    public function __toString() {
        return var_dump($this->result());
    }

    public function result() {
        return $this->output;
    }

    public function setoption($option, $value) {
        array_push($this->options, array($option=>$value));
    }

    public function run() {

        $where = array();
        $limit = '';
        $showMe = '';

        if (isset($this->options['admin'])) {
            $where[] = "admin = '".$this->options['admin']."'";
        }

        if (isset($this->options['area'])) {
            $where[] = "area = '".$this->area."'";
        }

        if (isset($this->options['showMe']) && $this->options['showMe'] = true) {
            $showMe = "AND missionary <> '".$this->me."'";
        }

        if (isset($this->options['limit'])) {
            $limit = " LIMIT ".$this->options['admin'];
        }

        if (count($where) > 0) {

            $where = implode(' AND ', $where);

        } else {
            $where = '';
        }

        $area = isset($this->area) ? $this->area : '';

        if ($area != '') {
            $sth = $this->pdo->prepare("SELECT missionary FROM missionary_areas WHERE area_uid = :area AND missionary_released = '0' ".$showMe);
            $sth->bindParam(':area', $area);
            $sth->execute();
            $sth->setFetchMode(PDO::FETCH_OBJ);

            while ($mis = $sth->fetch()) {
                $sth2 = $this->pdo->prepare("SELECT * FROM missionarios WHERE ".$where." mid = :mid LIMIT 1");
                $sth2->bindParam(':mid', $mis->missionary);
                $sth2->execute();

                $this->arr[] = $sth2->fetch(PDO::FETCH_ASSOC);
            }

            foreach($this->arr as $a) {
                array_push($this->output, $a);
            }
        } else {
            $sth = $this->pdo->prepare("SELECT * FROM missionarios ".$where.$limit);
            $sth->execute();
            $res = $sth->fetchAll(PDO::FETCH_ASSOC);

            foreach($res as $a) {
                array_push($this->output, $a);
            }
        }

    }
}

Answer 1

Here's the code smells:

1. The new keyword in a class definition
2. A result() method and $output attribute for data created by single method
3. A very long, ambiguous method: run
4. Two mostly-different SQL statements separated by an if-else

And here's what I'd do to eliminate them:

1. Move the PDO object to a constructor argument (see below) so you're not violating the Law of Demeter (see #1 and 2 here)
2. Remove result() altogether and simply return the result from the method in which it's built.
3. Break this into smaller, more meaningful methods based on responsibility (see #4 below)
4. Create getMissionaryByArea() and getAllMissionaries() methods with optional parameters

Here's what my code might look like:

<?php

/**
 * This class name is ambiguous.
 * I'm not sure what it means, so I cannot suggest a better name.
 */
class comps { 

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

    public function getMissionariesByArea($area, $admin_id=false, $omit_missionary_id=false, $limit=100) {
        $sql = <<<'ENDSQL'
        SELECT m.*
        FROM missionarios m
        JOIN missionary_areas ma ON ma.missionary = m.mid
        WHERE ma.area_uid = :area 
        AND ma.missionary_released = '0'
        LIMIT :limit
ENDSQL;
        $params = [
            'area' => $area,
            'limit' => $limit
        ];

        if ($omit_missionary_id) { 
            $sql .= ' AND ma.missionary <> :omit_missionary_id';
            $params['omit_missionary_id'] = $omit_missionary_id;
        }

        if ($admin_id) {
            $sql .= ' AND m.admin = :admin_id';
            $params['admin_id'] = $admin_id;
        }

        $sth = $this->pdo->prepare($sql);
        $sth->execute($params);
        return $sth->fetchAll();
    }

    public function getAllMissionaries($admin_id=false, $limit=100) {
        $sql = 'SELECT * FROM missionarios';
        $params = ['limit' => $limit];

        if ($admin_id) {
            $sql .= ' AND admin = :admin_id';
            $params['admin_id'] = $admin_id;
        }

        $sth = $this->pdo->prepare($sql);
        $sth->execute($params);
        return $sth->fetchAll();
    }
}

I can revise if I've missed something in my refactoring.

Answer 2

I completely agree with the answer from John Syrinek, but there is one more thing I would like to point out.

The issue

I think your implementation of __toString() is somewhat buggy. The function var_dump does not return anything and since the magic method __toString must return a string, I consider this an error.

The consider a scenario where you cast this class into a string and concatenate the returned value onto another string. You would not be appending anything, but the output would be populated with HTML from the var_dump() call.

A possible solution

You could fetch an array of the different names of each person and use the implode() native PHP method to make a pretty string you could return.