Shoutbox system

Posted on

Problem

I wrote a little Shoutbox System which is working fine, but I want to know if there is something I can improve about this code.

Here is my Shoutbox Overlay:

<div class="article_body" id="article_body" name="article_body">
<?php
    require('./tools/php/connector/connector.php');

    $query = "SELECT * FROM `shoutbox` ORDER BY entryid DESC LIMIT 5";
    $stmt = $db->prepare($query);
    $stmt->execute();
    $result = $stmt->get_result();
    $stmt->close();
    while($obj = $result->fetch_object()) {
        if ($obj != null || !empty($obj)) {

            echo("
            <p style='font-size: 14px;'>
                $obj->text
            </p>
            <table style='width: 100%; font-size: 11px;'>
                <tr>
                    <td style='text-align: start;'>
                        Von: <a style='font-size: 11px'; href='?".$obj->user."'>".$obj->user."
                    </td>
                    <td style='text-align: end;'>
                        Am: ".$obj->date_time."
                    </td>
                </tr>
            </table>
            <hr>
            ");
        }
        else {
            echo("<center>Shout-Box is empty</center>");
        }
    }
    if ($_SESSION['besucht'] == 1 && $_SESSION['user-ban-status'] - 1 == 0) {
        echo("<form action='./tools/php/shoutbox/shoutbox.php' method='POST'>
            <input type='text' name='shout' maxlength='500' style='width: 186px;'><button>Senden</button>
        </fomr");
    }
    if ($_SESSION['besucht'] == 1 && $_SESSION['user-ban-status'] - 1 == 1) {
        echo("<h5 style='color: red;'>You Are Banned. This Function Isn't Allowed For Banned Members.</h5>");
    }
    mysqli_close($db);
?>
</div>

The $_SESSION[‘besucht’] is for the Visited and logged in check.
The $_SESSION[‘user-ban-status’] is to check if the user is banned and if that is true the post function is disabled.
I limited the View per to the latest 5 entrys in my database so only the newest thing would be shown.

Here is my Backend Script for the input of the Shoutbox:

<?php
    if (!empty($_POST['shout']) || $_POST['shout'] != null) {

        session_start();

        if (!empty($_SESSION['user-username']) || $_SESSION['user-username'] != null) {
            require('../connector/connector.php');

            $text = $_POST['shout'];

            $user = $_SESSION['user-username'];

            date_default_timezone_set("Europe/Berlin");
            $timestamp = time();
            $datum = date("Y-m-d H:i:s",$timestamp);

            echo($text."<br>");
            echo($user."<br>");
            echo($datum."<br>");
            $stmt = $db->prepare("INSERT INTO `shoutbox`(`text`, `user`, `date_time`) VALUES (?,?,?)");
            $stmt->bind_param('sss', $text, $user, $datum);
            $stmt->execute();

            mysqli_close($db);
            header("Location: ../");
            exit;
        }
        else {
            header("Location: ../");
            exit;
        }
    }
    else {
        header("Location: ../");
        exit;
    }
?>

Solution

  • I recommend writing session_start(); first and unconditionally.

  • !empty() performs two checks: if the variable isset() AND contains a non-falsey value. This means $obj != null is not necessary. That said, your while() loop will halt if $obj is falsey.

  • There’s nothing wrong with using prepared statements, but for the record your first query will be just as stable/secure without it.

  • I noticed mysqli_close($db);. You should keep all of your query syntax object-oriented.

  • Rather than declaring new variables to feed to $stmt->bind_param() (single-use variables), just write the original variables as parameters. There isn’t much benefit in adding more variables to global scope.

  • It looks like all roads lead to:

    header("Location: ../");
    

    If this is true for you actual project script, write a single if block, then whether its contents are executed or not, after the condition block execute your redirect.

  • Don’t mix database logic and display logic (HTML). Prepare the data first and then display it in HTML when you have everything ready.
  • Try to avoid mysqli in new projects. Use PDO whenever possible. If you must use mysqli then stick to the object-oriented style.
  • Don’t close connection object or the statement. PHP will do that for you and you avoid mistakes by not doing it manually.
  • Don’t use date() and time() functions. Use DateTime class.
  • Don’t put parentheses after echo. It’s not a function and these parentheses are extremely confusing. Same applies to require
  • Reduce cyclomatic complexity by performing inverse checks. exit will kill the script so you can use that to finish early without wrapping whole code blocks in braces.
  • Use strict comparison (===).
  • Use htmlspecialchars() to avoid XSS.

Leave a Reply

Your email address will not be published. Required fields are marked *