Redunancy Issues in PHP Class

I've been studying PHP for a while now and decided to dive into OOP. Most of my code was a mess and I've begun to refactor much of the website to OOP; however, I'm having an issue with redundancy in my class functions. Below is a my Tracking.class.php file, which is responsible for returning arrays based on the function and the instance. This is my first time using OOP so I'm unsure if I'm being overly redundant in my child classes, and not entirely sure if there is a way I could clean up my code even more. Any help would be great!

```
class Tracking {

protected $type;
protected $user_id;
protected $response_array;
protected $result;
protected $typeToTable = array('weight' => 'wp_weight', 'calories' => 'wp_calories', 'move' => 'wp_move', 'sleep' => 'wp_sleep');
protected $arrayValue = array('weight' => 'value', 'calories' => 'totalcal', 'move' => 'length', 'sleep' => 'value');

function __construct($user_id, $type){

$this->type = $type;
$this->user_id = $user_id;

}

public static function getInstance($user_id, $type){

switch($type) {
case "weight" : $obj = new weightTracking($user_id, $type); break;
case "calories" : $obj = new caloriesTracking($user_id, $type); break;
case "move" : $obj = new moveTracking($user_id, $type); break;
case "sleep" : $obj = new sleepTracking($user_id, $type); break;
case "mood" : $obj = new feelTracking($user_id, $type); break;
}

return $obj;

}

function stats( Database $pdo ) {

$table_value = $this->arrayValue[$this->type];
$table = $this->typeToTable[$this->type];

$query = "SELECT AVG($table_value) as avg, MAX($table_value) as max, MIN($table_value) as min from $table WHERE user_id = :user_id";

$pdo->query($query);

$pdo->bind(':user_id', $this->user_id);

$pdo->execute();

$row = $pdo->single();

if ( empty( $row

I will apologize for the lack of formatting and using your code segments as straight example - but as I re-read your code I see my old self a whole lot and I don't consider myself any level near some of the coders here.

However one important aspect to improvement is obviously trial and error and a lot of refactoring. So you asked about OOP going from procedural php within a single require I would assume your going into one right way which is using classes - however there is a lot of pitfalls in your code.

The first would be that to effectively do OOP you should use SOLID design pattern.

First there are plenty of ORM out there that does your PDO wrapper. One thing starters would like to do is to wrap a low level class (the PDO object) into a higher wrapper functional object - its not wrong to do so - but its been done over. In reality what you should do is not offer the user (ie you in this case) a simpler form of accessing data that you need while using the database. The database of your APP can change...what will you then? You will need to write a wrapper class again because your class is tightly coupled with PDO.

Lets take a look at the database class

protected $host = DB_HOST;
protected $user = DB_USER;
protected $pass = DB_PASS;
protected $dbname = DB_NAME;

protected $dbh;
protected $error;
protected $stmt;

Too many variables - the majority of the time the need to hold onto the DB's credential inside the object is not necessary - it should be part of your configuration of the APP and invoked as such then passed inside to the DB object to initialize the connection - once that is done you don't need this info.

If you change information then a new object should be created. Why? because image you do need to connect to two different DB (one for user, one for tracking as an example) - if you store your credential your stuck to this. Also as I can see from your constructor - your not even passing your credentials to it - its fixed from a global stand point.

The majority of your class is just a smaller typing of what is already presented ie: query, then bind, then execute. Its a rehash of the system's PDO.

Another flaw is your Single() function - it has the execute() command - what happens if your query returns more than one result and you want to iterate it one at a time - you can't re-execute the query to grab the second row. fetch()'s purpose is to go fetch and iterate through the rows one by one. You can argue that you can use resultSet() to grab all then foreach but if you are returning 10K rows you will be in a heap of trouble.

Next - lets touch a bit of the tracking class.

One flaw is the getInstance. Singleton are bad for maintainability because you can't make more than one object of it and its hard to test it.

Ideally - since all your functions are relying on the database - you should have passed it (aka injected) is part of your constructor and then call your functions which uses the DB. Its not like you will instantly use a different DB within the function because you are tightly coupled from it.

Basically: your Track class is your repository, and your PDO is redundant because its just shorthanding the coding.

thats my first pass through of your code.

StackExchange Code Review Q#46670, answer score: 6