tags:

views:

256

answers:

7

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>
+1  A: 

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.

Ben Alpert
Sorry for that Ben
Patrik Björklund
+1  A: 

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.

gargantaun
Like the updated code now?
Patrik Björklund
hmmmm, no not really. Making a class called dbconnect is wrong for two reasons. Firstly all classes should start with a capital letter ;-) Secondly, I suspect you're still duplicating functionality that would be inherent is any half decent DB object. Which suggest you've still not grasped the way OOP is meant to work. Did you write the DB class. If so, think about what extra functionality belongs in that class. If you didn't write it, look through it's methods to figure out what functionality you are duplicating. If you see functionality missing, add it yourself to the DB class.
gargantaun
I'll also add, there's no escaping the fact that you're going to have to read a lot to get your head around OOP. Making the switch from procedural to OOP is like digging for gold with your bare hands. It hurts like hell, but it's worth it.
gargantaun
in fact... I just took a more detailed look at your code and it seems you've really missed the point. Grouping your functions into a class is definitely the wrong way to go. Those "methods" should be part of the DB class itself. If you need to tweak them for particular tasks, then you either need to think about making your methods more abstract, or subclassing DB for more fine-grained control. So the answer to your original question is definitley... no.
gargantaun
+1  A: 

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.

Alfred
+1  A: 

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.

rogeriopvl
The mixing is one of the things I thought about how to do, do you have any suggestion/example you can think of?
Patrik Björklund
You can start by making all functions return only strings without any html. I can give a very simple example: your function returns a simple string, then in your index you call the function and add the html tags, this way you only get a function call in your html file, and all logic in a php file.
rogeriopvl
+1  A: 

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.

Jani Hartikainen
Thanks for the input. I updated the code with some actual objects and followed some other of your suggestions (I think atleast :-))
Patrik Björklund
A: 

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");

A: 

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

lyrae