patternphpMinor
Web page to add a notice, with some validation
Viewed 0 times
withnoticevalidationpagewebsomeadd
Problem
I'm displaying multiple queries and
I was just wondering if someone could help me make this code simpler.
MySQL
add_notice.php
`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 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[
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
`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 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[
Solution
Good things
You are using
Dead query
This query is never used:
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:
So what you are doing is:
-
Get all the
-
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):
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.
So we could do something like this:
Naming Things
There are several things which could use better names.
Those variables don't add any notices, they just contain them. Therefore, this would make more sense:
Your queries are also not named very well. Instead of numbering them, just name them according to what they do...
The rest of the code looks pretty decent I think. I noticed
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 $query3The rest of the code looks pretty decent I think. I noticed
$addNoticeSuccessMessage is also never used. Perhaps you intended to complete this later.Code Snippets
$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++;
};$query1 = $connection->prepare("SELECT MAX(notice_category_id) + 1 FROM notices")
$notice_category_id = $query1->execute();SELECT auto_increment FROM information_schema.tables WHERE
table_name = 'the_table_you_want'$query1 = $connection->prepare("SELECT auto_increment FROM information_schema.tables where table_name = 'notices'");
$notice_category_id = $query1->execute();Context
StackExchange Code Review Q#109571, answer score: 6
Revisions (0)
No revisions yet.