Web page to add a notice, with some validation

Question 1

I'm displaying multiple queries and if statements. In the if statement, it increments notice_category_id by 1 each time a category is created, but when I code the update/delete/edit (or if truncated), sections won't rowCount() increment the row and make duplicates. I'm just wondering if I should change the way the the notice_category_id is working. If so, any ideas? Also, how can I display each error message near the field that has the error using the $assNoticeError and $addNoticeErrorMessage?

I was just wondering if someone could help me make this code simpler.

MySQL

CREATE TABLE `categories`
(
 category_id INT(3) NOT NULL AUTO_INCREMENT, 
 category_type VARCHAR(255) NOT NULL, 
 PRIMARY KEY (`category_id`)
) ENGINE = InnoDB;
CREATE TABLE notices
(
 notice_id INT(3) NOT NULL AUTO_INCREMENT, 
 notice_category_id INT(3) NOT NULL, 
 notice_user_id INT(3) NOT NULL, 
 notice_title VARCHAR(255) NOT NULL, 
 notice_content VARCHAR(500) NOT NULL, 
 notice_date TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP, 
 PRIMARY KEY (notice_id), 
 FOREIGN KEY (notice_category_id)
 REFERENCES categories(category_id), 
 FOREIGN KEY (notice_user_id)
 REFERENCES users(user_id)
) ENGINE = InnoDB;

add_notice.php

<?php
session_start();
include_once("database/connect.php");
if(empty($_POST["form"])) {
$user_id = $_SESSION['user_id'];
$query0 = $connection->prepare("SELECT 
 user_id 
 FROM users
 ");
$query0->execute();
$query1 = $connection->prepare("SELECT 
 notice_category_id 
 FROM notices
 ");
$query1->execute();
$notice_category_id = $query1->rowCount();
if($notice_category_id <= $notice_category_id) {
 $notice_category_id++;
};
if($_SERVER["REQUEST_METHOD"] == "POST") {
 $addNoticeError = array();
 $addNoticeErrorMessage = "";
 $addNoticeSuccessMessage = "";
 if(empty($_POST["notice_title"])) {
 $addNoticeError[] = "You must enter a Notice Title.";
 }
 if(empty($_POST["notice_content"])) {
 $addNoticeError[] = "You must enter a Notice Description.";
 }
 if(empty($_POST["category_type"])) {
 $addNoticeError[] = "You must select a Category Type.";
 }
 if(!empty($addNoticeError)) {
 foreach($addNoticeError as $error) {
 $addNoticeErrorMessage .= $error . '<br />';
 }
 } else {
 if(!isset($error)) {
 $category_type = $_POST['category_type'];
 $query2 = "INSERT INTO categories (category_type) VALUES (:category_type)";
 $query2 = $connection->prepare($query2);
 $query2->bindParam(":category_type", $_POST["category_type"], PDO::PARAM_STR, 255);
 $query2->execute();
 $notice_category_id = $_POST['notice_category_id'];
 $notice_user_id = $_POST['notice_user_id'];
 $notice_title = $_POST['notice_title'];
 $notice_content = $_POST['notice_content'];
 $query3 = "INSERT INTO notices (notice_category_id, notice_user_id, notice_title, notice_content) VALUES (:notice_category_id, :notice_user_id, :notice_title, :notice_content)";
 $query3 = $connection->prepare($query3);
 $query3->bindParam(":notice_category_id" , $_POST["notice_category_id"] , PDO::PARAM_INT, 3);
 $query3->bindParam(":notice_user_id" , $_POST["notice_user_id"] , PDO::PARAM_INT, 3);
 $query3->bindParam(":notice_title" , $_POST["notice_title"] , PDO::PARAM_STR, 255);
 $query3->bindParam(":notice_content" , $_POST["notice_content"] , PDO::PARAM_STR, 500);
 $query3->execute();
 header("Location: notices.php");
 }
 }
}
}
?>
<!DOCTYPE html>
<html lang="en">
<head>
<title>Add a Notice</title>
</head>
<body>
<?php if(!empty($addNoticeErrorMessage)) : ?>
 <p><?php echo $addNoticeErrorMessage ?></p>
<?php endif ?>
<?php if(!empty($addNoticeSuccessMessage)) : ?>
 <p><?php echo $addNoticeSuccessMessage ?></p>
<?php endif ?>
 <h4>Add a Notice</h4>
 <form method="post" name="add_notice" action="">
 <input name="notice_category_id" type="hidden" value="<?php echo $notice_category_id; ?>" />
 <input name="notice_user_id" type="hidden" value="<?php echo $user_id; ?>" />
 <input type="text" name="notice_title" id="notice_title" placeholder="notice title" maxlength="25" value="<?php if(isset($_POST["notice_title"])) { echo htmlentities($_POST["notice_title"]); } ?>" />
 <br />
 <textarea name="notice_content" id="notice_content" placeholder="notice content"/><?php if(isset($_POST["notice_content"])) { echo htmlentities($_POST["notice_content"]); } ?></textarea>
 <br /><br />
 <select name="category_type">
 <option value="">Select...</option>
 <option value="Content1">Content1</option>
 <option value="Content2">Content2</option>
 <option value="Content3">Content3</option>
 <option value="Content4">Content4</option>
 <option value="Content5">Content5</option>
 </select>
 <br />
 <br />
 <input type="hidden" name="add_notice">
 <button type="submit" id="btnSubmit" name="btnSubmit">Add a new Notice</button>
 </form>
 </body>
 </html>

Question 2

Welcome to Code Review! I have rolled back the last edit. Please see what you may and may not do after receiving answers .

Question 3

Good things

You are using PDO with parameterized queries, which is a really good thing. That should save you potential troubles between the application and the database. Kudos.

Dead query

This query is never used:

$query0 = $connection->prepare("SELECT 
 user_id 
 FROM users
 ");
$query0->execute();

There is no point in getting all the user IDs from the database at this point if you don't reuse it later. Dead code should be removed.

Row count

This part doesn't need to be this tortuous:

$query1 = $connection->prepare("SELECT 
 notice_category_id 
 FROM notices
 ");
$query1->execute();
$notice_category_id = $query1->rowCount();
if ($notice_category_id <= $notice_category_id) {
 $notice_category_id++;
};

So what you are doing is:

Get all the notice_category_id
Count them
Check that a value is lesser than or equal to itself (A value will always equal itself no matter what)
Increment by one to the next ID

That could all be done in one step (plus assignment from execution):

$query1 = $connection->prepare("SELECT MAX(notice_category_id) + 1 FROM notices")
$notice_category_id = $query1->execute();

You get the idea, I think.

Note, if you are using MySQL, which I believe is the case, there is another trick, which is more natural as it will ask the database to give you the next ID (assuming the ID columns are auto-increment, which is generally the case), and that should prevent any duplication as well as take account of deleted records and such.

SELECT auto_increment FROM information_schema.tables WHERE
table_name = 'the_table_you_want'

So we could do something like this:

$query1 = $connection->prepare("SELECT auto_increment FROM information_schema.tables where table_name = 'notices'"); 
$notice_category_id = $query1->execute();

Naming Things

There are several things which could use better names.

 $addNoticeError = array();
 $addNoticeErrorMessage = "";
 $addNoticeSuccessMessage = "";

Those variables don't add any notices, they just contain them. Therefore, this would make more sense:

 $errors = array();
 $errorMessage = "";
 $successMessage = "";

Your queries are also not named very well. Instead of numbering them, just name them according to what they do...

$selectAllUserIds // instead of $query0
$selectAllNoticeCategoryIds // instead of $query1
$selectNextNoticeCategoryId // instead of the above refactored $query1
$insertCategoryType // instead of $query2
$insertNotice // instead of $query3

The rest of the code looks pretty decent I think. I noticed $addNoticeSuccessMessage is also never used. Perhaps you intended to complete this later.

Question 4

thank you I made the changes! Also I added my MySQL in the main post. With notice_category_id it is a foreign key so with $query1 = $connection->prepare("SELECT MAX(notice_category_id) + 1 FROM notices") $notice_category_id = $query1->execute(); that adds an increment to the database so I need to do another SELECT query when someone creates another noitce? because it doesn't increment the hidden field it stays as 1 but increments the database

Question 5

Ah, concurrency is what you are dealing with, then? In that case, it would be better to get the category_id from the primary table once you insert a record in it, and then use that value as the foreign key in the other table. Look up mysql_insert_id, it's made specifically for that purpose.

Question 6

oh okay no idea ha but i'll give it ago can I use PDO::lastInsertId? as I specially need to use PDO

Question 7

I didn't know PDO had an equivalent, my bad! Yes, by all means, go with PDO::lastInsertId that's even better!

Question 8

I'm having some trouble with lastInsertId() it keeps returning 0 and when i click the button to create a notice it gives a mysql error and doesn't add a notice but adds the category_id everytime

Question 9

This statement here is completely redundant:

$notice_category_id = $query1->rowCount();
if($notice_category_id <= $notice_category_id) {
 $notice_category_id++;
};

Why on Earth do you need to check if something is less than or equal to itself!? The above chunk of could should simply become the following one-liner:

$notice_category_id = $query1->rowCount() + 1;

Also, I've noticed that many of your variables are phrased like functions, which are commonly statements beginning with a verb. A few examples are:

$addNoticeError
$addNoticeErrorMessage
$addNoticeSuccesMessage

You should probably just drop the add from these.

Credit goes to user @Phrancis for finding these.

Question 10

Your indentation is a bit strange:

$addNoticeError = array();
$addNoticeErrorMessage = "";
$addNoticeSuccessMessage = "";

 $query3->bindParam(":notice_category_id" , $_POST["notice_category_id"] , PDO::PARAM_INT, 3);
 $query3->bindParam(":notice_user_id" , $_POST["notice_user_id"] , PDO::PARAM_INT, 3);
 $query3->bindParam(":notice_title" , $_POST["notice_title"] , PDO::PARAM_STR, 255);
 $query3->bindParam(":notice_content" , $_POST["notice_content"] , PDO::PARAM_STR, 500);

You can use array_join instead:

 foreach($addNoticeError as $error) {
 $addNoticeErrorMessage .= $error . '<br />';
 }

$addNoticeErrorMessage = array_join($addNoticeError, '<br />');

Phrancis Phrancis 20.5k6 gold badges69 silver badges155 bronze badges · Answer 1 · 2015-11-02 22:56:58Z

Good things

You are using PDO with parameterized queries, which is a really good thing. That should save you potential troubles between the application and the database. Kudos.

Dead query

This query is never used:

$query0 = $connection->prepare("SELECT 
 user_id 
 FROM users
 ");
$query0->execute();

There is no point in getting all the user IDs from the database at this point if you don't reuse it later. Dead code should be removed.

Row count

This part doesn't need to be this tortuous:

$query1 = $connection->prepare("SELECT 
 notice_category_id 
 FROM notices
 ");
$query1->execute();
$notice_category_id = $query1->rowCount();
if ($notice_category_id <= $notice_category_id) {
 $notice_category_id++;
};

So what you are doing is:

Get all the notice_category_id
Count them
Check that a value is lesser than or equal to itself (A value will always equal itself no matter what)
Increment by one to the next ID

That could all be done in one step (plus assignment from execution):

$query1 = $connection->prepare("SELECT MAX(notice_category_id) + 1 FROM notices")
$notice_category_id = $query1->execute();

You get the idea, I think.

Note, if you are using MySQL, which I believe is the case, there is another trick, which is more natural as it will ask the database to give you the next ID (assuming the ID columns are auto-increment, which is generally the case), and that should prevent any duplication as well as take account of deleted records and such.

SELECT auto_increment FROM information_schema.tables WHERE
table_name = 'the_table_you_want'

So we could do something like this:

$query1 = $connection->prepare("SELECT auto_increment FROM information_schema.tables where table_name = 'notices'"); 
$notice_category_id = $query1->execute();

Naming Things

There are several things which could use better names.

 $addNoticeError = array();
 $addNoticeErrorMessage = "";
 $addNoticeSuccessMessage = "";

Those variables don't add any notices, they just contain them. Therefore, this would make more sense:

 $errors = array();
 $errorMessage = "";
 $successMessage = "";

Your queries are also not named very well. Instead of numbering them, just name them according to what they do...

$selectAllUserIds // instead of $query0
$selectAllNoticeCategoryIds // instead of $query1
$selectNextNoticeCategoryId // instead of the above refactored $query1
$insertCategoryType // instead of $query2
$insertNotice // instead of $query3

The rest of the code looks pretty decent I think. I noticed $addNoticeSuccessMessage is also never used. Perhaps you intended to complete this later.

thank you I made the changes! Also I added my MySQL in the main post. With notice_category_id it is a foreign key so with $query1 = $connection->prepare("SELECT MAX(notice_category_id) + 1 FROM notices") $notice_category_id = $query1->execute(); that adds an increment to the database so I need to do another SELECT query when someone creates another noitce? because it doesn't increment the hidden field it stays as 1 but increments the database
Ah, concurrency is what you are dealing with, then? In that case, it would be better to get the category_id from the primary table once you insert a record in it, and then use that value as the foreign key in the other table. Look up mysql_insert_id, it's made specifically for that purpose.
oh okay no idea ha but i'll give it ago can I use PDO::lastInsertId? as I specially need to use PDO
I didn't know PDO had an equivalent, my bad! Yes, by all means, go with PDO::lastInsertId that's even better!
I'm having some trouble with lastInsertId() it keeps returning 0 and when i click the button to create a notice it gives a mysql error and doesn't add a notice but adds the category_id everytime

score 3 · Answer 2 · 2015-11-02 22:19:04Z

This statement here is completely redundant:

$notice_category_id = $query1->rowCount();
if($notice_category_id <= $notice_category_id) {
 $notice_category_id++;
};

Why on Earth do you need to check if something is less than or equal to itself!? The above chunk of could should simply become the following one-liner:

$notice_category_id = $query1->rowCount() + 1;

Also, I've noticed that many of your variables are phrased like functions, which are commonly statements beginning with a verb. A few examples are:

$addNoticeError
$addNoticeErrorMessage
$addNoticeSuccesMessage

You should probably just drop the add from these.

Credit goes to user @Phrancis for finding these.

Quill Quill 12k5 gold badges41 silver badges93 bronze badges · Answer 3 · 2015-11-02 23:33:36Z

Your indentation is a bit strange:

$addNoticeError = array();
$addNoticeErrorMessage = "";
$addNoticeSuccessMessage = "";

 $query3->bindParam(":notice_category_id" , $_POST["notice_category_id"] , PDO::PARAM_INT, 3);
 $query3->bindParam(":notice_user_id" , $_POST["notice_user_id"] , PDO::PARAM_INT, 3);
 $query3->bindParam(":notice_title" , $_POST["notice_title"] , PDO::PARAM_STR, 255);
 $query3->bindParam(":notice_content" , $_POST["notice_content"] , PDO::PARAM_STR, 500);

You can use array_join instead:

 foreach($addNoticeError as $error) {
 $addNoticeErrorMessage .= $error . '<br />';
 }

$addNoticeErrorMessage = array_join($addNoticeError, '<br />');

Stack Exchange Network

Web page to add a notice, with some validation

3 Answers 3

Good things

Dead query

Row count

Naming Things

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Hot Network Questions

Web page to add a notice, with some validation

3 Answers 3

Good things

Dead query

Row count

Naming Things

Your Answer

Sign up or log in

Post as a guest

Post as a guest

Related

Hot Network Questions