3
\$\begingroup\$

Let me start with a little introduction. I am in the process of learning OOP in PHP. I have also researched Design Patterns but have not yet fully grasped the different types of concepts. I am at the stage where every few months I realise that I am not doing things the correct way and have to change my style. This is so frustrating. Therefore I would like to find out the correct way of doing things once and for all. I have tried to fully read up on Stackoverflow about the following topics:

ORM
Data Mapper
Singleton
Globals are evil
Everything related

However I am still not clear about a few things. I am posting my code here in a clear and concise way and hope that people can point out both the good practices and the bad ones. I will list all my questions at the end.

Please do not close as a duplicate, I have honestly searched through almost every question on the topic but I still want to know a few things which I have not been able to clarify. Sorry it is so long but I have tried to organize it so it should read well!

I will start by posting the essentials of my Database class.

Database.php

<?php 


class DatabaseMySQL{

    private static $dbh;

    public function __construct(){
        $this->open_connection();
    }

    public function open_connection(){
        if(!self::$dbh){
            return (self::$dbh = new PDO(DB_TYPE.':host='.DB_HOST.';dbname='.DB_NAME, DB_USER,DB_PASSWORD)) ? true : false;
        }
        return true;
    }

    public function query($sql, $params=array()){
        $this->last_query = $sql;
        $stmt = self::$dbh->prepare($sql);
        $result = $stmt->execute($params);  
        return $result ? $stmt : $stmt->errorInfo();
    }

    public function fetch_all($results, $class_name=''){
        return $results->fetchAll(PDO::FETCH_CLASS, $class_name);
    }

}

?>

This is my Database class file. This class allows me to create as many instances as I like of this class and it will reuse the already instantiated PDO object stored as a Static property of the class. It also fetches the data from the result set using PDO to get the data as objects of a specified class.

My next file I have is my class that all my other classes inherit from. I have called it MainModel. I have no idea if this follows convention or not.

MainModel.php

<?php



abstract class MainModel{

    protected static $table;

    public function __construct($array=array()){
        $this->assign_known_properties($array);
    }

    public function assign_known_properties($array){
        foreach($array as $key=>$value){
            $this->$key = $value;
        }
    }

    public static function find_by_id($id){
        $db = new DatabaseMySQL();
        self::intialise_table_name();
        $id = (int) $id;
        $sql = "SELECT * FROM ".static::$table." ";
        $sql .= "WHERE id = {$id} ";    
        $result = self::find_by_sql($sql);      
        return array_shift($result);
    }

    public static function find_all(){
        $db = new DatabaseMySQL();
        self::intialise_table_name();
        $sql = "SELECT * FROM ".self::$table." ";
        return self::find_by_sql($sql);
    }

    public static function fetch_as_objects($results){
        $db = new DatabaseMySQL();
        $called_class = get_called_class();
        $results = $db->fetch_all($results, $called_class);
        return $results;
    }

    public static function find_by_sql($sql){
        $db = new DatabaseMySQL();
        $results = $db->query($sql);
        return $results ? self::fetch_as_objects($results) : false; 
    }

    public static function intialise_table_name(){
        $called_class = get_called_class();
        static::$table = strtolower($called_class).'s';
    }

    public function get_table_fields(){
        self::intialise_table_name();
        $sql = "SHOW FIELDS FROM ".static::$table." ";
        return self::find_by_sql($sql);
    }

    public function set_table_details(){
        $fields = $this->get_table_fields();
        $total = count($fields);
        $array = array();
        foreach($fields as $object){
            $array [] = $object->Field;
        }
        $this->table_details = array('objects'=>$fields,'array'=>$array,'total'=>$total);
        $this->set_placeholders_for_new_record();
        $this->set_properties_as_array();
        $this->set_properties_as_array(true);
    }

    public function set_properties_as_array($assoc=false){
        $array = array();
        if (!$assoc){
            foreach($this->table_details['array'] as $field){
                if(isset($this->$field)){
                    $array [] = $this->$field;
                }else{
                    $array [] = NULL;
                }
            }
            $this->table_details['values'] = $array;
        }else{
            foreach($this->table_details['array'] as $field){
                if(isset($this->$field)){
                    $array[$field] = $this->$field;
                }else{
                    $array [$field] = NULL;
                }
            }
            $this->table_details['assoc_values'] = $array;
        }
    }

    public function set_placeholders_for_new_record(){
        $string = '';
        for($i=0; $i<$this->table_details['total']; $i++){
            $string .= '? ';
            if(($i+1) != $this->table_details['total'] ){
                $string .= ", ";
            }
        }
        $this->table_details['placeholders'] = $string;
    }

    public function create(){
        $db = new DatabaseMySQL();
        $this->set_table_details();
        $sql = "INSERT INTO ".static::$table." ";
        $sql .= " VALUES({$this->table_details['placeholders']}) ";
        $result = $db->query($sql, $this->table_details['values']);

        // If array is returned then there was an error.
        return is_array($result) ? $result : $db->insert_id();
    }

    public function update(){
        $db = new DatabaseMySQL();
        $this->set_table_details();
        $sql = "UPDATE ".static::$table." ";
        $sql .= " SET ";
            $count = 1;
            foreach($this->table_details['array'] as $field){
                $sql .= "{$field} = :{$field} ";
                if($count < $this->table_details['total']){
                    $sql .= ", ";
                }
                $count++;
            }

        $sql .= " WHERE id = {$this->id} ";
        $sql .= " LIMIT 1 ";
        $result = $db->query($sql, $this->table_details['assoc_values']);
        return $result;
    }

    public function save(){
        return isset($this->id) ? $this->update() : $this->create();
    }
}


?>

To summarise this file. I use static methods like find_by_id($int) that generate objects of the called class dynamically. I am using Late Static Bindings to gain access to the name of the called class and I use the $stmt->fetchAll(PDO::FETCH_CLASS, $class_name) to instantiate these objects with the data from the database automatically converted into objects.

In each static method I instantiate an instance of the DatabaseMySQL class. In each static method I set the correct static $table name to be used in the SQL queries dynmically by getting the name of the class and appending an s to it. So if my class was User, that would make the table name users.

In my constructer I have placed an optional array which can be used to insert some variables as properties of an object as it is created. This way everything is done dynamically which leaves me to the final stage of my project. My inheritance classes.

User.php

class User extends MainModel{

}

Question.php

class Question extends MainModel{

}

What I do now is simple. I can say:

$user = new User(array('username'=>'John Doe'));
echo $user->username; // prints *John Doe*
$user->save(); // saves (or updates) the user into the database.

I can retrieve a user with a static call to User.

$user = User::find_by_id(1);
echo $user->username; // prints users name

So now for my questions:

  • 1) What name would you call this design pattern (if it even is one at all).Data Mapper? Dependency Injection? Domain Model (whatever that is)? Data Access Layer?

  • 2) As it stands now is this implementation considered well structured?

  • 3) If it is good, is there a naming convention I should be aware of that is missing in my code?

  • 4) If it is considered good, would you mind pointing out what particularly you liked so I will know which part to definitely keep?

  • 5) If you dont think it is good would you care to give a detailed explanation why this is so?

  • 6) Should my create, update, delete which are all methods of my objects be in the same class as my find_by_id, find_all which are only called statically and actually return objects. If they should be in two different classes, how do I go about that?

  • 7) Why is everyone else using $load->('UserClass') functions and fancy words like mappers and I havent yet needed them once?

\$\endgroup\$
11
\$\begingroup\$

Congratulations, that means you are learning. You'll be at that stage for some time. In fact, I don't think you'll ever truly leave it, or if you do, then you should be concerned. The time between "changes" might increase, but you should never stop learning and should always find some way you can improve. Its only natural. The first few months I was learning OOP I felt the same. Then I started hanging out here and applying what I was learning, or already had learned, to some of the problems people were having. In the process I affirmed the lessons I was attempting to learn and actually ended up learning quite a bit more than if I were still trying to puzzle it all out myself. I guess what they say about teaching others being a great tool for learning is true and is something I strive for in every answer now. For instance, what I am taking from this is that I need to look more closely into these other patterns you mentioned.

If you have searched and shown effort, then you do not need to worry about your question being closed. If something obviously wasn't covered well enough for you, then it is obviously not a duplicate. Just be sure to show and cite appropriately, as you have done, otherwise you'll be told to google it and probably get downvoted.

Using both private and static is awkward and wrong. A private property or method cannot be accessed outside of the class in which it is declared. A static property or method is used to specifically grant outside access by providing a property or method that is not associated with any, or rather with all, instances of the class. So you begin to see why this is awkward. This is wrong because static properties cannot be initialized using a return value from a function, an object, or some other variable. It must be initialized with static data, such as a string literal or integer or array. You should be receiving silent errors about this. I would consider turning up your error reporting so that you can see them. What you really want here is a private property, which, by the way, should be initialized in a private method, or in the constructor, to ensure that it is only ever initialized once. This will remove the need for checking if it has already been set. It is good to separate your code by functions, this follows the Single Responsibility Principle fairly well, but some things should just be left in the constructor for convenience.

Your database class is backwards. You don't create a new instance of a class to do something different with the same data. You create an instance of the database class to have an instance of the database, then you call the proper methods to do those different things and save their returns as different variables.

Variables, properties, methods, functions, constants; Pretty much anything that you can apply a name to should have a descriptive name so that it can easily be identified. Less descriptive names that are commonly used and so are easily identifiable are fine, such as $dbh. But those that you create yourself should be better named to ensure future understanding. So $array is a very vague name and you should consider renaming it.

You really want to be careful with variable-variables and variable-functions. There are instances where these are fine, but typically they are avoided because they are difficult to spot and debug. The only place I can think of where I use such is in MVC frameworks between the controller and view.

First, before I go too much farther, let me steer you away from the static keyword. Just forget you ever heard about it for now. I believe this is confusing you a little. I'll be honest, I haven't needed static for anything I've done yet. I know about it, know what it does, but I've never needed it. Its probably not anything you will ever need either, at least not in this application. Revisit the topic once you've gotten the rest straight.

Now... You said that this MainModel is inherited from, but from the looks of it, you are actually doing it backwards again. Your model class is, in every method, reinstantiating the database class. This is redundant and wasteful. In fact, the model class, in a roundabout and awkward fashion, is actually inheriting from the database class. You have set up the model as an abstract class so that it can be extended. In fact, abstract classes MUST be extended. They cannot be instantiated without throwing errors, so it should not even have a constructor. Once extended, it can then work from the assumption that whatever class is extending it will have set up the information it needs to run, and the class extending it can assume that the methods it is inheriting will work with its data. In fact, the two classes should be able to share data seamlessly, as if they were one. So, what you really want to do here is have your database class properly inherit from the model class, and have your model class properly use its information. For instance:

class DatabaseMySQL extends MainModel {
    //etc...
        $db->find_by_id( $id );//for demonstration
    //etc...
}

abstract class MainModel {
    //etc...
    public function find_by_id( $id ) {
        //$db = new DatabaseMySQL();//no longer necessary, implied by $this
        $id = (int) $id;

        $this->intialise_table_name();
        $sql = "SELECT * FROM {$this->table} "
                . "WHERE id = $id "
        ;
        $result = $this->find_by_sql( $sql );      
        return array_shift( $result );
    }
    //etc...
}

Hopefully the above will help you with some of your questions, now I'll attempt to answer the rest directly.

1) What name would you call this design pattern (if it even is one at all).Data Mapper? Dependency Injection? Domain Model (whatever that is)? Data Access Layer? I don't know, with "model" it appears that you are attempting to start an MVC, but obviously not. Maybe Domain Model? I honestly wouldn't be concerned with design patterns right now, though. Best thing is just to learn the theories and how to apply them. Once you have a firm grasp on this, then you can worry about design patterns and how they are applied. I learned OOP with MVC, so that's the one I'm most comfortable with. I've heard of these others, but have not looked into them as of yet.

As it stands now is this implementation considered well structured? See review.

If it is good, is there a naming convention I should be aware of that is missing in my code? Naming conventions are purely stylistic choice. For instance, I would say camelCase is better because I think its easier to read, but someone else, maybe you, would say under_score is better. Its only when you start mingling your code with another's that you have to worry about changing your style. The important thing to remember is to stay consistent.

If it is considered good, would you mind pointing out what particularly you liked so I will know which part to definitely keep? Abstract classes are good. They sort of go hand in hand with interfaces. I didn't start learning about them until much later. They are a powerful tool and will help you out immensely. I only wish I had heard about them sooner... Then again, I did, they just confused me :)

If you dont think it is good would you care to give a detailed explanation why this is so? See review.

Should my create, update, delete which are all methods of my objects be in the same class as my find_by_id, find_all which are only called statically and actually return objects. If they should be in two different classes, how do I go about that? I explained part of this. If we follow an MVC pattern, then your model would hold all of the methods it needs to fetch and manipulate data. So all of these methods look fine in the one class, but the implementation you use, as I pointed out in the review, could use some work.

Why is everyone else using $load->('UserClass') functions and fancy words like mappers and I havent yet needed them once? Got me, I'm still learning too. But if I were to hazard a guess, it is because we are essentially recreating what those other more advanced users are implementing. I'm not sure what that first example is supposed to be, whatever it is, it doesn't look like valid syntax, but the mapper is just a "more efficient" and "easier" way of doing it. Notice the air-quotes. Again, get comfortable with the concept before delving into the more advanced stuff.

\$\endgroup\$
  • \$\begingroup\$ What an unbelievable answer, thanks for taking the time to go through all of that. Your most important point to me at the moment (I gotta go through this a couple of times) is to forget about the static keyword. I see the way you have managed without it, i think its amazing. Still reading the post again, will come back when I understand the rest! +1 \$\endgroup\$ – In God I Trust Aug 28 '12 at 18:15
  • 1
    \$\begingroup\$ Consider accepting mseancole's question by clicking on the tick below the score. \$\endgroup\$ – Quentin Pradet Sep 17 '12 at 8:37

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.