CODEWITHSUNDEEP
phpsqloop
Patrik Björklund
Patrik Björklund

Reputation: 1237

Is this php code written correctly according to OO principles?

Iv been trying to get my head around object orientation and I think iv started to get some of the concepts, but im not sure. Making a google search that answers if my train of thought is correct proved to be quite hard so I decided to ask here really quick, please tell me if the question is against any rules.

Im I thinking correctly in regards to messagepassing? What are the obviously bad things? How should I think while going forward with my learning? Like getpagecontent($page, $connection); etc

Atm im reading [Oreilly - Learning php and mysql][1] and [Programming in an Object-Oriented Environment][2] And some books on UML

Here is the code.

dbfunctions.php

<?php
class dbconnect {

    function dbconnect() {
    $this->dbhost = 'xx';
    $this->dbuser = 'xx';
    $this->dbpass = 'xx';
    $this->dbdatabase = 'xx';
    }

    function createdbconnection() {
        require_once('DB.php'); // pear

        $this->connection = DB::connect("mysql://$this->dbuser:$this->dbpass@$this->dbhost/$this->dbdatabase");

        if (DB::isError($this->connection)) {
        die("Could not connect (connection)<br>" . DB::errorMessage($this->connection));
        }
    }

    function closedbconnection(){
        $this->connection->disconnect();
    }
}

class dbinteractions {

   function dbinteractions($connection) {
            $this->connection = $connection;
        }

   function searchdb($qstring) {
    if (get_magic_quotes_gpc()) {
        $qstring = stripslashes($qstring);
    }

    $qstring = mysql_real_escape_string($qstring);

    $query = "SELECT content FROM site_content WHERE content LIKE '%$qstring%'";
    $result = $this->connection->query($query);

    if(DB::isError($result)) {
        die("Could not connect (query)<br>" . DB::errorMessage($result));
    }

    while($result_row = $result->fetchRow()) {
        echo("<h2>Resultat:</h2>");
        foreach($result_row as $out)
            echo($out . "<p>");
    }
}

    function getpagecontent($page) {
        $query = "SELECT * FROM site_content WHERE (page_id = \"" . $page . "\")";
        $result = $this->connection->query($query);;

        while($result_row = $result->fetchRow()) {
            echo "<h1>" . $result_row[0] . "</h1>"; //Echo page_id
            echo $result_row[1]; //Echo content
            echo "<h2>" . $result_row[2] . "</h2>"; //Echo timestamp
        }
    }

}
?>

search.php

<?php
function displaysearchform()
{
    echo("<p></p>");
    if($_GET["search"] == '') { //Display search box if no search ?>

        <form method="GET" action="<?php echo(htmlentities($_SERVER['PHP_SELF'])); ?>">
            <label>
                Search: <input type="text" name="search" />
            </label>
            <input type="submit" value="Go!">
        </form>

    <?php
        return false;
    }
    else
    {
        return true;
    }
}

?>

index.php

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<link rel="stylesheet" href="style.css" type="text/css">
<html>
<head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
    <title></title>
</head>
<body>
    <div id="container">
        <div id="sidebar">
            <a href="?id=1">1</a><br>
            <a href="?id=2">2</a><br>
        </div>
        <div id="main">

            <?php
                include("dbfunctions.php");
                include("search.php");

                $dbconnect = new dbconnect();

                $dbconnect->createdbconnection();

                $dbinteractions = new dbinteractions($dbconnect->connection);

                if(!$_GET["id"] && $_GET["search"] == "") { //Check if direct site hit and no search query
                    $dbinteractions->getpagecontent("1");
                    }

                else  {
                    $page = $_GET["id"];
                    $dbinteractions->getpagecontent($page); //Get and display page content
                }

                if (displaysearchform() == true){ //If search was made don't display searchform
                    $dbinteractions->searchdb($_GET["search"]);
                }

                $dbconnect->closedbconnection(); //Close connection to db
            ?>
        </div>
    </div>
</body>
</html>

Upvotes: 1

Views: 417

Answers (7)

sqram
sqram

Reputation: 7201

i like to keep it simple and straight forward. Here's a class that I use, that was used in the book Beginning PHP and MySQL: From Novice to Professional by Jason Gilmore

<?php

class SQL {


    function __construct() {

        $this->host     = "xxx";

        $this->user     = "xxx";

        $this->password  = "xxx";

        $this->database  = "xxx";



        $this->result;

        $this->querycount;

        $this->connect();

        $this->select();        

    }



    function connect() {

        $this->linkid = @ mysql_connect($this->host, $this->user, $this->password);

        if(!$this->linkid) {

            echo "--[ could not connect to sql database ]--";   exit();

        }

    }



    function select() {

        if(!@ mysql_select_db($this->database, $this->linkid)) {

            echo "--[ could not select database ]--";   exit();

        }

    }




    function escape($data) {

        return mysql_real_escape_string(trim(htmlspecialchars($data)), $this->linkid);

    }



    function query($query) {

        if( $this->result = @ mysql_query($query, $this->linkid) ) {

            $this->querycount++;

            return true;

        } else {

            echo "<b>Error:</b>" . mysql_error($this->linkid);              

            return false;

        }   

    }



    function affectedRows() {

        $count =  @ mysql_affected_rows($this->linkid);

        return $count;

    }



    function numRows() {

        $count = @ mysql_num_rows($this->result);

        return $count;

    }



    function fetchObject() {

        $row = @ mysql_fetch_object($this->result);

        return $row;

    }



    function fetchRow() {

        $row = @ mysql_fetch_row($this->result);

        return $row;

    }



    function fetchArray() {

        $row = @ mysql_fetch_array($this->result);

        return $row;

    }



    function fetchAssoc() {

        $row = @ mysql_fetch_assoc($this->result);

        return $row;

    }



    function numQueries() {

        return $this->querycount;

    }

}



?>

I'm not saying it's the best way. But it's very simple and straight forward; and should also be changed to use mysqli instead, but i've been lazy =p

Upvotes: 0

arda
arda

Reputation:

I have a small advice about object oriented php programming; firstly read the documentation in the php.net and finally you should read the book "Object-Oriented PHP" by No Starch Press -> http://nostarch.com/oophp.htm

Also , use this statement at the top of your class/function files with you will include(search and dbfunctions.php)

if (!defined('page')) exit('No direct script access allowed');

and define the page which uses this class - like as the sample in below:

define("page","search"); include("dbfunctions.php"); include("search.php");

Upvotes: 0

Jani Hartikainen
Jani Hartikainen

Reputation: 43253

I can't say there's really much OOP here, but it sounds and looks more like you are trying to learn a good approach with parameters instead of using globals - which you seem to have accomplished.

If you want to improve more from this code, here are some pointers

  • Try using a consistent coding style. For example, the PEAR coding standard is commonly used in PHP
  • Change your database code into a class, which encapsulates the connection object. For example, you could have a Database class with methods connect, search and close
  • Use some templating solution with your HTML code. You can use PHP itself as a templating language, as demonstrated here. The main idea is that you separate "application logic" from "view logic"
  • You may also want to modify your functions which use echo to simply return the result, and use another function (or just code in your template) to output them. Usually, you would want a function either provide/fetch data or output data, not both.

Lastly, I'd suggest looking at some of the popular PHP frameworks, as they usually follow good OOP coding styles. CakePHP should be easy to start with, but because it uses PHP4, it doesn't always do OOP very well. You could get comfortable with the principles with Cake, and then move on to something like Symfony or Zend Framework. My own coding style got much better after I started using Zend Fw.

Upvotes: 1

rogeriopvl
rogeriopvl

Reputation: 54104

You only have one thing right with OO. The $connection object.

Everything else is procedural. You should start by creating a class, instead of all those loose functions in "dbfunctions.php".

Also note that you should avoid mixing logic code with HTML. It gets hard to maintain.

Here's probably the best book you can read on the subject .

It's not easy to get into object oriented paradigm. But when you get it, it's like riding a bicycle. You never forget.

Upvotes: 1

Alfred
Alfred

Reputation: 61793

I think when you really like to write OO you should use objects which interact with each other to accomplish the task at hand. I don't see any object instantiated from classes so I don't think this is proper Object oriented code. This is more procedural code if you ask me.

Upvotes: 1

gargantuan
gargantuan

Reputation: 8944

You're certainly using objects correctly, and you're accesing them correctly. You're DB object is a singleton by the look of it, that's good. And you're using '->' to access other object methods and properties.

But it looks like you're re implementing functionality that should be part of the DB object. For example, with my own DB class that I have i'd do something like this 

$db = DBObject::getInstance(); // The DB object is a singleton

$sql = "Select * from.... etc etc";

$result = $db->query($sql);

Which I think should accomplish everything you've tried to do. The DB object should know how to connect to the database by itself, issue its own error messages and close the connection should it need to (which it probably doesn't). And stuff like handling quotes, slashes and making sure your Query is safe could all be handles inside the DB object as well.

But also, the procedure you're trying to perform could easily be a class all of its own (eg: PageRenderer). But here you've created a lot of functions which use other objects.

But it's hard to give you a straight answer. Yes, you're doing it right, but you seem to be doing too much.

Upvotes: 1

Sophie Alpert
Sophie Alpert

Reputation: 143194

I'm not sure what you mean by object-oriented, but your code looks nothing like OO code in the traditional sense of classes, objects, and methods. When most people say "OO", they mean that their code is built off of different types of objects that you can call methods on.

To me, your code looks like typical procedural code.

Upvotes: 1

Related Questions