Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I'm brand new at OOP PHP and I would like to get as many of the coding conventions right as fast as possible. I made this tiny guestbook script, and I would like to know if there's anything not done as it should.

Index.php:

<?php
$username = "root";
$password = "";
$database = "oop";
$host = "localhost";

mysql_connect($host, $username, $password);
mysql_select_db($database);
?>
<html>
<head>
</head>
<body>
    <?php
    include("views/gaestebog.php");
    ?>
</body>
</html>

Guestbook.php, the class:

<?php
class Gaestebog {

    public function __contruct() {

    }

    public function getPosts() {
        $query = mysql_query("SELECT * FROM gaestebog");
        while ($row = mysql_fetch_array($query)) {
            echo '
            <tr>
                <td>'.$row['navn'].'</td>
            </tr>
            <tr>
                <td>'.$row['besked'].'</td>
            </tr>
            ';
        }
    }

    public function addPost($navn, $besked) {
        mysql_query("INSERT INTO gaestebog VALUES('', '$navn', '$besked')");
    }
}
?>

and guestbook.php, the view:

<?php
include("classes/Gaestebog.php");
$gaestebog = new Gaestebog();
if (isset($_POST['opret'])) {
    $navn = $_POST['navn'];
    $besked = $_POST['besked'];
    $gaestebog->addPost($navn, $besked);
}
?>
<table>
    <?php
    $gaestebog->getPosts();
    ?>
</table>

<hr />

<form action="" method="post">
    <table>
        <tr>
            <td>Navn:</td>
            <td><input type="text" name="navn" value="Patrick" /></td>
        </tr>
        <tr>
            <td>Besked:</td>
            <td><input type="text" name="besked" value="Hej med dig !!" /></td>
        </tr>
        <tr>
            <td><input type="submit" name="opret" value="Opret" /></td>
        </tr>
    </table>
</form>
share|improve this question
You might want to take a look at the PHP manual online about your use of database operations for MySQL for example mysql_query() is no longer suggested php.net/manual/en/function.mysql-query.php one of the alternatives is to use the mysqli interface with is OOP. – JRSofty Nov 9 '12 at 8:27
instead of using mysql go for mysqli or PDO. Use parameterized query. make a use of database abstraction class instead of using core database functions in your classes. Thats all I can think of now – LoVeSmItH Nov 9 '12 at 8:29
When i use mysqli, should i be creating a new object inside every class of the mysqli connection? – Patrick Reck Nov 9 '12 at 8:43
@PatrickReck: No, you create one instance, and pass it around you objects/methods. – Madara Uchiha Nov 9 '12 at 9:28

4 Answers

up vote 6 down vote

This is often called procedural programming with outdated libraries:

mysql_connect($host, $username, $password);
mysql_select_db($database);

And that was only the beginning of your script. Maybe it's better you don't focus so much on fancy programming terms but instead just get the work done.

Example guestbook.php:

<?php
require('bootstrap.php');

$page = new Htmlpage('My Guestbook');
$page->start($_SERVER['REQUEST_URI']);

$gaestebog = new Gaestebog();

echo '<table border="1">';

foreach ($gaestebog->getPosts() as $post)
{
    $row = new Htmlencoded($post);

    echo <<<OUT
            <tr>
                <td>$row['navn']</td>
            </tr>
            <tr>
                <td>$row['besked']</td>
            </tr>
OUT;
}
echo '</table>';

$page->closeRequest();
?>

Otherwise if you want to learn about object oriented programming, use object interfaces and practice, practice, practice.

Start with something simple, like a form field. Not with a whole script.

share|improve this answer
up vote 6 down vote

Yes, the first, and most obvious thing to note is your vulnerability to SQL injection.

Please, don't use mysql_* functions in new code. They are no longer maintained and the deprecation process has begun on it. See the red box? Learn about prepared statements instead, and use PDO or MySQLi - this article will help you decide which. If you choose PDO, here is a good tutorial.

Also, it's considered good practice to separate logic from presentation (i.e. PHP from HTML).

share|improve this answer
How would i separate it even more than i've done here? – Patrick Reck Nov 9 '12 at 8:46
@PatrickReck: Well, you need very little PHP code on your view or template (that's what it's called). Form processing, heavy business logic, database interactions, those should be on their own files, with zero HTML output on them. – Madara Uchiha Nov 9 '12 at 9:27
up vote 5 down vote

Couple of points:

  • Don't use the deprecated mysql library, try PDO or MySQLi instead
  • __contruct() has a typo, should be __construct().
  • Don't return raw HTML from get methods such as getPosts(), instead return an array of posts and let the view create HTML if that's the output you want
  • Protect against, SQL Injection, your INSERT query is vulnerable
share|improve this answer
up vote 3 down vote

This is not at all OOP. guestbook.php has a class and contains few methods but that doesn't mean that you are applying OO concepts. I am afraid that your example has little scope to apply OO. Few improvements you can do here, separate presentation logic from getPosts().

Thus getPosts() will be responsible for retrieving posts from db and then create separate methods how you want to present the data, for example getPostInTabularFormat(), getPostInDivformat() etc...

Also you can convert the Form in the view to a class. take a look at this post

share|improve this answer
+1 but note that it's not necessarily a bad thing that this code isn't OOP... – Konrad Rudolph Nov 9 '12 at 18:09

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.