HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

Web page to add a notice, with some validation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withnoticevalidationpagewebsomeadd

Problem

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

`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 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.

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.