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 variableisset()
AND contains a non-falsey value. This means$obj != null
is not necessary. That said, yourwhile()
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()
andtime()
functions. UseDateTime
class. - Don’t put parentheses after
echo
. It’s not a function and these parentheses are extremely confusing. Same applies torequire
- 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.