Problem
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:
The 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.
Solution
OOP
- Why have a
get
method, if your not going to use it (eg inget_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 asquery
andget
fitting into it), but then it’s also the user dao? That’s a bit much for one class. If you separate it intoDbWrapper
(query
andget
) andUserDAO
(get_by_username
andfind_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).