2
\$\begingroup\$

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

    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ This is a bug: ... && $this->options['showMe'] = true, I believe you meant to use ==. \$\endgroup\$ – user220944 May 23 '13 at 20:34
4
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ I'm getting Parse error: syntax error, unexpected '<<' (T_SL) \$\endgroup\$ – watzon May 24 '13 at 18:29
  • \$\begingroup\$ <<<EOL; is incorrect. besides using something more meaningful than "EOL", you dont put a semicolon there. @sun-tzu try removing the semicolon. \$\endgroup\$ – Amelia May 24 '13 at 22:33
  • 1
    \$\begingroup\$ Thanks Hiroto! I've corrected this and also changed it from heredoc to nowdoc to be positively sure SQL injection won't happen. \$\endgroup\$ – Johntron May 24 '13 at 23:07
1
\$\begingroup\$

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.

\$\endgroup\$

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.