3
\$\begingroup\$

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')");
    }
}
?>

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>
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – JRSofty Nov 9 '12 at 8:27
  • \$\begingroup\$ 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 \$\endgroup\$ – LoVeSmItH Nov 9 '12 at 8:29
  • \$\begingroup\$ When i use mysqli, should i be creating a new object inside every class of the mysqli connection? \$\endgroup\$ – Patrick Reck Nov 9 '12 at 8:43
  • \$\begingroup\$ @PatrickReck: No, you create one instance, and pass it around you objects/methods. \$\endgroup\$ – Madara's Ghost Nov 9 '12 at 9:28
7
\$\begingroup\$

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).

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ How would i separate it even more than i've done here? \$\endgroup\$ – Patrick Reck Nov 9 '12 at 8:46
  • \$\begingroup\$ @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. \$\endgroup\$ – Madara's Ghost Nov 9 '12 at 9:27
6
\$\begingroup\$

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
| improve this answer | |
\$\endgroup\$
6
\$\begingroup\$

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.

| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

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

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

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.