3
\$\begingroup\$

I'm making a matchmaking system using PHP and MySQL which allows users to search through a list of musicians so that they can find people with the right skills and interests who are near them. It works but it's extremely messy, and I'd like to learn how I can clean it up and eliminate as many bad habits as possible. I've been trying to learn OOP but it feels more like I'm just writing procedural code and putting it into methods.

The search form:

Search bar

The database:

Database

When the form is submitted, the find_users method is called from my database class.

class DB
{
protected $conn;

public function __construct($config)
{
    try {
        $conn = new PDO('mysql:host='. $config['host'] . ';
            dbname=' . $config['db'], 
            $config['username'],
            $config['password']);

        $conn->setAttribute(\PDO::ATTR_ERRMODE, \PDO::ERRMODE_EXCEPTION);

        $this->conn = $conn;
    } catch (Exception $e) {
        return false;
    }
}



public function query($query, $bindings = [])
{
    try {
        $stmt = $this->conn->prepare($query);
        $stmt->execute($bindings);
        return ($stmt->rowCount() > 0) 
        ? $stmt :
        false;
    } catch (Exception $e) {
        var_dump($e);
    }
}



public function get($tableName, $limit = 24)
{
    try {
        $result = $this->conn->query("SELECT * FROM $tableName LIMIT $limit");
        return ( $result->rowCount() > 0)
        ? $result->fetchAll(PDO::FETCH_OBJ)
        : false;
    } catch (Exception $e) {
        die($e);
    }
}



public function get_user_by_username($username) 
{
    $result = $this->query("SELECT * FROM users where username = :username LIMIT 1", array(
        "username" => $username
        ));

    return $result->fetch(PDO::FETCH_OBJ);
}



public function find_users($username = '', $skill = '', $tag = '', $lat = 1, $lng = 1, $distance)
{

    $sql = "SELECT DISTINCT
    users.username, ( 3959 * acos( cos( radians($lat) ) * cos( radians( latitude ) ) 
            * cos( radians( longitude ) - radians($lng) ) + sin( radians($lat) ) * sin(radians(latitude)) ) ) AS distance 
    FROM
    users
    INNER JOIN
    user_skills ON users.user_id = user_skills.user_id 
    INNER JOIN
    skills ON user_skills.skill_id = skills.skill_id
    INNER JOIN
    user_tags ON users.user_id = user_tags.user_id
    INNER JOIN 
    tags ON user_tags.tag_id = tags.tag_id
    WHERE users.availability = 'Yes'";

    $bindings = array();

    if ($username) {
        $sql .= " AND users.username LIKE :username";
        $bindings['username'] = "%" . $username . "%";
    }

    if ($skill) {
        $sql .= " AND skills.skill_name = :skill";
        $bindings['skill'] = $skill;
    }

    if ($tag) {
        $sql .= " AND tags.tag_name = :tag";
        $bindings['tag'] = $tag;
    }

    $sql .= " HAVING distance < $distance ";
    $sql .= " ORDER BY distance";

    $result = $this->query($sql, $bindings);

    return ( $result )
    ? $result->fetchAll(PDO::FETCH_OBJ)
    : false;
}
}

Like I mentioned earlier, it's seriously messy but I'm not sure how to tidy it up. Accepting 6 parameters feels really wrong but I need them so that I can sort users using the specified criteria.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ I think this code is quite good. There are some minor syntax issues, but nothing bad. Of course the find_users() function is where things get interesting and possible improvements can be made. In your database I noticed that 'skills' and 'tags' are basically the same. Some normalization might be needed here: en.wikipedia.org/wiki/Database_normalization \$\endgroup\$ – KIKO Software Mar 6 '15 at 12:36
  • 1
    \$\begingroup\$ Very nice question! Welcome to Code Review! \$\endgroup\$ – RubberDuck Mar 6 '15 at 13:27
1
\$\begingroup\$

OOP

  • Why have a get method, if your not going to use it (eg in get_user_by_username)?
  • It's not clear what the responsibility of your class is. DB suggests that it's some kind of generic wrapper around PDO (with methods such as query and get fitting into it), but then it's also the user dao? That's a bit much for one class. If you separate it into DbWrapper (query and get) and UserDAO (get_by_username and find_user) you should be on the right track.
  • It's not ok to just dump an exception in a class (the calling code cannot do anything to recover, you have ugly output to the enduser), and it's also not ok to just die inside a class (again, no possibility to recover for the calling class). If you can't recover in the class, just throw the exception upwards.

Misc

  • be consistent with your SQL: all keywords should be all uppercase (see eg where).
  • Why do you return false for no results? I think an empty array works just as well (and it's what is returned by PDO, so it means less code).
\$\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.