1
\$\begingroup\$

I'm still learning PHP OOP, so I'd like to seek for some expert advice or criticism regarding this code I created.

In this script, I created the objects I need to process the user input from a form. While the $homepage object is responsible for displaying the whole webpage.

My questions are:

  1. Is this consider clean code?
  2. Can I completely hide the fact that I actually include the autoload and the configuration files?
  3. Any recommendation or tips to improve my code?

<?php

/**
*
* OOP practice
* @author Joey
*/

require_once 'config/autoload.php';
require_once 'config/config.php';

// Database credentials

$host = $config['host'];
$dbname = $config['dbname'];
$username = $config['username'];
$password = $config['password'];

// Short variable names

$fname = isset($_POST['fname']) ? clean_data($_POST['fname']) : null;
$lname = isset($_POST['lname']) ? clean_data($_POST['lname']) : null;
$your_question = isset($_POST['question']) ? clean_data($_POST['question']) : null;

try {

    // Page object

    $homepage = new Page('Questions and Answers', 'QA', '', 'You ask, we answer');

    // Author object

    $author = new Author($fname, $lname);

    // Question object

    $question = new Question($your_question, $author);

    // Database object

    $pdo = new DbConnect($host, $dbname, $username, $password);

    // Connect to the database

    $pdo->open_connection();
} catch (Exception $e) {
    echo $e->getMessage(), '<br>';
    exit;
} catch (PDOException $e) {
    echo $e->GetMessage(), '<br>';
    exit;
}

if (isset($_POST['submit'])) {
    // Insert the user input into the database

    $pdo->execute_query('INSERT INTO author (first_name, last_name) VALUES (?, ?)', array($author->getFirstName(), $author->GetLastName()));
    $pdo->close_connection();
    $homepage->content = '<h3>Your question</h3><br><p>Hello ' . $author->getFirstName() . ' ' . $author->getLastName() . '!' . '</p><br>';
    $homepage->content .= '<p>You ask: ' . $question->getQuestion() . '</p><br>';
    $homepage->content .= 'Do you want to ';
    $homepage->content .= '<a href="index.php">Ask another question?</a>' . ' ' . '<br>';
} else {
    // Display the form

    $homepage->content = <<<_END

    <h3>Your question here</h3><br>
    <form action="{$_SERVER['PHP_SELF']}" method="post" role="form">
        <label for="fname">Enter your first name</label><br>
     <input type="text" id="fname" name="fname" size="30"><br>
        <label for="lname">Enter your last name</label><br>
        <input type="text" id="lname" name="lname" size="30"><br>
        <label for="question">What is your question?</label><br>
        <textarea id="question" name="question" rows="10" cols="70"></textarea><br>
        <input type="submit" name="submit" value="Submit">
    </form>
_END;
}

// Display the webpage using the Page class

$homepage->display();

function clean_data($data)
{
    $data = trim($data);
    $data = strip_tags($data);
    $data = addslashes($data);
    return $data;
}
\$\endgroup\$
0

2 Answers 2

1
\$\begingroup\$

A short list of issues I've noticed after a first glance at your code:

  • Pokemon exception handling (gotta catch 'em all): wrapping everything in a huge try-catch block is considered bad practice. What makes your code stand out even more is the pointless second catch
  • pointless second catch: a PDOException is a custom exception class, which is to say: PDOException extends Exception, therefore, your catch (Exception $e) block will catch instances of PDOExceptions, too. As a result, the second catch block is obsolete. Pointless. Code-smell. Noise. Get rid of it.
  • clean_data is not great. It's bad even. I can understand the strip_tags call, but addslashes is really rather pointless here. And a possible source of bugs. If you forget to call addslashes when inserting a password, or in some other piece of code that handles login information, the passwords in the DB won't match, even though the user has filled out the form correctly. You're using prepared statements, no need to add slashes. If anything, it does more harm than good
  • From the variable name ($pdo), it's quite obvious what kind of class DbConnect is. And, like I've posted on this site so many times (links below): it's evil. PDO offers a clean, well balanced, easy to use and powerful, fully object oriented API. You can't wrap it into something and end up with a better tool. Far from it. The only thing that you can do with it is to use it as a base in a fully-fledged DBAL like doctrine. And since doctrine already exists, writing it yourself is, basically, reinventing the wheel.
  • Generally, I don't like PHP code that strings together markup. It's error prone, and hard to maintain. That's why I didn't pay much attention to the HTML bit, but as tim pointed out: $_SERVER['PHP_SELF'] is a bad idea. If the form should be processed by the same script (or on the same url) that spawned it, just don't specify an action attribute. The default behavior is to post the form back to the same url anyway.

Links to previous answers explaining why you shouldn't try to wrap PDO:

\$\endgroup\$
3
  • \$\begingroup\$ PDO derived classes are pointless. Ok I got you. But, what if I don't intend to share my code to public. And the methods I created inside the class are those I only need. Is this still violating the practice? I admit I'm not extending the PDO, but it gives me fewer lines of code when I do that. Don't you all advice that the code should be shorter and concise? \$\endgroup\$
    – Joey
    Oct 24, 2014 at 23:45
  • \$\begingroup\$ @Joey: in theory, writing a PDO wrapper to reduce the amount of code you have to write is fine. But 99% of the time, you start writing such a wrapper, you find yourself adding workarounds and methods to expose more and more of the PDO API. As your project grows, so does the need to expose certain methods (like beginTransaction, commit and inTransaction). At that point, you'll find a wrapper is just too cumbersome and too limiting. Honestly: a wrapper is a means to hide functionality, or create a clean API. \$\endgroup\$ Oct 27, 2014 at 8:05
  • \$\begingroup\$ PDO's API is as clean as they come, so a PDO wrapper has nothign to do on that front. In a PHP project, its job (DB connectivity) is just too important to hide any part of its functionality. A wrapper is like a straight jacket: unless you're a coding Houdini, you're better of leaving it as is. Even if you don't share the code with anyone \$\endgroup\$ Oct 27, 2014 at 8:07
1
\$\begingroup\$

XSS

Your clean_data currently protects you against XSS via $fname, $your_question, etc. But it is hard to see that , because the cleaning happens so far away from the printing. And if you follow the advice of @Elias Van Ootegem and remove the clean_data method (which you should), then you are open to XSS. So you should prevent this directly when printing user input.

The other (and a lot more dangerous) XSS attack is $_SERVER['PHP_SELF']. You should definitely filter this with htmlspecialchars (or use an empty form action, or hardcode it).

Naming

  • be consistent. See for example getFirstName vs GetLastName (the first version with a lowercase starting letter is more conventional).
  • don't shorten (too much): For example, firstName isn't that much longer than fname, but it is clearer.

Misc

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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