1
\$\begingroup\$

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;
    }
?>
\$\endgroup\$
  • 2
    \$\begingroup\$ Hey, welcome to Code Review! Here we review working code and try to make it better. What we cannot do is solve your problem for you or make your code do new things. Have a look at our help center for more information. \$\endgroup\$ – Graipher Aug 2 '19 at 12:16
  • \$\begingroup\$ My code works. I'm just asking for a suggestion for a hint or a recommendation for this code. If you do something like that you can also ask if someone has an idea for their own idea xD I don't think that will be a shame or against the "rules" \$\endgroup\$ – RyffLe Aug 2 '19 at 12:20
  • 1
    \$\begingroup\$ From your question it sounds like what you are asking is for us to implement a new feature: "So I mean how can I add numbers for Pages for the useres to swipe forward and backward to the discussions in the Shoutbox?" If your code cannot currently do this, it is off-topic to ask about here. What you can ask about instead is, describe what your code does do and how that could be done in a better way. \$\endgroup\$ – Graipher Aug 2 '19 at 12:22
  • \$\begingroup\$ Is taht better? xD \$\endgroup\$ – RyffLe Aug 2 '19 at 12:26
  • \$\begingroup\$ I believe this question has been edited to be on-topic, so I have voted to reopen. \$\endgroup\$ – mickmackusa Aug 3 '19 at 21:01
0
\$\begingroup\$
  • 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.

\$\endgroup\$

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.