Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I need some advice off the back of a question I posted on StackOverflow. I'm a procedural PHP programmer and I'm looking to make the move into object oriented PHP. I have started a small project that basically acts as a small CMS that allows me to separate my website content from the actual structure of the page. The code I am using to achieve this is:

<?php

class Connection extends Mysqli{

    public function __construct($mysqli_host,$mysqli_user,$mysqli_pass, $mysqli_db) {
        parent::__construct($mysqli_host,$mysqli_user,$mysqli_pass,$mysqli_db);
        $this->throwConnectionExceptionOnConnectionError();     
    }

    private function throwConnectionExceptionOnConnectionError(){

        if(!$this->connect_error){
            echo "Database connection established<br/>";
        }else{
            //$message = sprintf('(%s) %s', $this->connect_errno, $this->connect_error);
            echo "Error connecting to the database."; 
            throw new DatabaseException($message);
        }
    }
}

class DatabaseException extends Exception
{
}

class Page{

    private $con; 

    public function __construct(Connection $con) {
        $this->con = $con;
        if(isset($_GET['id'])){
            $id = $_GET['id'];
        }else{      
            $id = 1;
        }       
        $this->get_headers($id);
        $this->get_content($id);
        $this->get_footer($id);
    }

    private function get_headers($pageId){ 
        $retrieveHead = $this->con->prepare("SELECT headers FROM pages WHERE page_id=?");
        $retrieveHead->bind_param('i',$pageId);
        $retrieveHead->execute();
        $retrieveHead->bind_result($header);
        $retrieveHead->fetch();
        $retrieveHead->close();
        echo $header;   
    }

    private function get_footer($pageId){ 
        $retrieveFooter = $this->con->prepare("SELECT footer FROM pages WHERE page_id=?");
        $retrieveFooter->bind_param('i',$pageId);
        $retrieveFooter->execute();
        $retrieveFooter->bind_result($footer);
        $retrieveFooter->fetch();
        $retrieveFooter->close();
        echo $footer;   
    }

    private function get_content($pageId){
        $retreiveContent = $this->con->prepare("SELECT template_id, section_title, i1, i2 FROM content WHERE page_id=? ORDER BY sequence DESC");
        $retreiveContent->bind_param('i',$pageId);
        $retreiveContent->execute();
        $retreiveContent->bind_result($template_id, $section_title, $i1, $i2);
        $retreiveContent->store_result();
        while ($retreiveContent->fetch()) {
        //Variables will be populated for this row.
        //Update the tags in the template.
        $template = $this->get_template($template_id);
        $template = str_replace('[i1]',$i1,$template);
        $template = str_replace('[i2]',$i2,$template);
        //$theTemplate is populated with content. Probably want to echo here
        echo $template;
        }
        $retreiveContent->free_result();    
        $retreiveContent->close();
    }

    private function get_template($template_id){
        $retreiveTemplate = $this->con->prepare("SELECT code FROM templates WHERE template_id=?");
        $retreiveTemplate->bind_param('i',$template_id);
        $retreiveTemplate->execute();
        $retreiveTemplate->bind_result($template);
        $retreiveTemplate->fetch();
        $retreiveTemplate->close();
        return $template;
    }

}
?>

I create a page object in my index.php file which is used to output the page by running the functions below in the order listed in the Page constructor. The comments I received on StackOverflow were along the lines of:

This code violates numerous OOP principles. As a result of that, I hope that no newbies attempt to use this as a means of learning OOP, as they will be learning an invalid programming paradigm if they use what you have here as an example. Read up on the single responsibility principle and the other components of SOLID, and perhaps get a book on design patterns.

I've read up on these subjects and I remember a lot of the principals from Java in my first year of Uni (about 6 years ago now) but as far as I can see in my code I am separating concerns as much as possible. It doesn't make sense to me to have the database connection as part of the page as it isn't really a property of the page whereas the headers and footers and content are. However, the page requires a database connection to function so I therefore have to pass a Connection object into the Page class to acheive the connectivity.

I've asked the authors of the comments numerous times to explain their reasoning behind such comments but all the keep doing is making statements saying this is bad code without providing examples as to why or what I might do to change it therefore I thought I'd ask this question here as I'll never learn if I'm not given a helping hand along the way.

share|improve this question

migrated from programmers.stackexchange.com Feb 7 '13 at 9:45

This question came from our site for professional programmers interested in conceptual questions about software development.

    
possible duplicate of Object Orientated PHP, what's wrong with this setup? –  Josay Feb 7 '13 at 9:47
add comment

1 Answer 1

up vote 5 down vote accepted

What I see at first glance.

Page and Connection are extending Mysqli

Why is a Page a special Mysqli? (Page extends Mysqli)

Why is a Connection a special Mysqli? (Connection extends Mysqli)

A page can use a Mysqli to save stuff, but it is not a Mysqli. Also the word Connection suggests it's a base-class of Mysqli (since a Connection can be a connection to a OracleDB, a TCP-Server etc.) but in your case Mysqli is the base for Connection.

The whole code implies you have no idea of OOP at all. Which is Okay, you can read some stuff about it, there is many outter there :)

Probably the most OOP programmers just see the code and don't know where to start improving it, because it just eludes them completly.

share|improve this answer
    
Sorry, that's actually old code from before I was passing the Connection object. Page shouldn't actually extend mysqli as you rightly suggest but once again I'm told I have no idea of OOP when most of this setup has been achieved using books and tutorials on the subject. Could you please elaborate.. –  jezzipin Feb 7 '13 at 10:09
    
Could you elaborate please. My implementation takes into account separation of concerns (e.g. the database object [Connection] is in charge of the connection to the database. The Page object [Page] is responsible for everything to do with the pages. I use inheritance correctly to ensure I am not duplicating code (My Connection class is a version of MySQLi with specified error handling). –  jezzipin Feb 7 '13 at 10:22
2  
Your Connection class adds nothing the Mysqli class did not already have. So why extend it? To add an extended exception, which also don't adds something new? –  K.. Feb 7 '13 at 10:33
    
Okay so in your view I shouldn't have a Connection class. That's fine. This was suggested elsewhere to implement it in this way but instead of passing the Connection class in here I would pass in the base Mysqli class using the built in exception handling. Any other pointers (I really appreciate this) there is an absence of real world examples anywhere that explain these principles with useable code. All examples refer to animals or people but this isn't useful in understanding classes would come together to form a login system for example.. –  jezzipin Feb 7 '13 at 10:43
1  
Thanks @..K I'll take a look at this. I did OOP in Java years ago at Uni but as I am an expert in HTML and CSS I moved away from Java and learned procedural PHP instead. In my job I don't really code in PHP but I want to put a portfolio together and most comments online suggest that PHP should now be coded using OOP hence why I'm trying to learn it. However, I am still a fan of the procedural approach and don't understand why people call it inefficient. If functions are well commented and documented the a procedural application can be very easy to maintain so I may just stick to procedural.. –  jezzipin Feb 7 '13 at 11:08
show 4 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.