# How clean is my close_account code?

Hello i'm coding a social site and I need to know how clean this code is. I read other answers people told me from my previous questions like this and used it as guidelines. So now I wanna know if anything could be improved. Thank you.

<?php

mysqli_report(MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT);
error_reporting(-1);

$userLoggedIn =$_SESSION['thesocial_username'];

$post_id = isset($_POST['cancel']) ? header("Location: settings.php") : '';

if(isset($_POST['close_account'])) {$close_query = $con->prepare('UPDATE users SET user_closed = "yes" WHERE username = ?');$close_query->bind_param("s", $userLoggedIn);$close_query->execute();

session_destroy();
}

?>

<center>
<div class="close_column close_account">

<h4>Close Account</h4>
Are you sure you want to close your account ?<br><br>

You can reopen your account at anytime by logging in.<br><br>

<form action="close_account.php" method="POST">

<input type="submit" name="close_account" id="close_account" value="Yes, Close my account.">
&nbsp;&nbsp;
<input type="submit" name="cancel" id="update_details" value="Nevermind.">

</form>
</div>
</center>

• I don't support that usage of a ternary variable declaration which contains a page redirection. I mean, you aren't setting a value to $post_id so I find it to be an inappropriate syntax. There's not much to review here. – mickmackusa Jul 10 '20 at 0:22 • Ok thank you. So I should do it the normal way ? – user13477176 Jul 10 '20 at 0:48 • Definitely use if for that. Actually the $post_id variable should not be there at all. Further, you should exit the script after sending location header. I can close the account and get redirected to settings.php by posting cancel=1&close_account=1. The script would actually send two location headers, I assume the browser will redirect to the adress of the first of them. – slepic Jul 10 '20 at 4:32
• Ohhhh lol. I was confused when everyone was talking about $post_id. I just took that code and threw in it there. I honestly don't know why but I was up for a long time trying to fix the code and I must've screwed up. Thanks a lot though. – user13477176 Jul 10 '20 at 21:03 ## 2 Answers The code is overall good basic PHP and above the average. However, I would like it more structured. For example, why you are setting the error reporting manually in this file? Apparently you have sort of a bootstrap file as includes/header.php, why not to put these lines there? Besides, i don't see any point in the line assigning$post_id variable. What does a post id to do with deleting an account? And yes, such a syntax structure looks embarrassing.

It is also a good rule of thumb to follow Location header with an explicit exit call. Although it is not critical in this particular case, sometimes the lack of the script termination could pose a serious threat, as the header itself doesn't stop the script execution.

Some notes on the database structure

• The computer world has its own representation for "yes" and "no", it's 1 and 0 respectively. So consider making the field of tinyint type instead of varchar.
• it is considered a good practice to address all database entities by the unique id. It's better to store the user id in the session and use it for all the database operations.

## The code structure

One big issue I overlooked at first is the structure of your script. It seems that includes/header.php is not just a bootstrap file but it also starts some HTML output. that's a big NO. An HTTP header such as Location cannot be sent after HTML. Your PHP lets you to do it due to some permissive setting, but it is nevertheless wrong, from the logical and technological points of view. It just makes no sense to start any output when your script is not going to display anything but would only redirect to another page.

Split your includes/header.php file into two: bootstrap.php and header.php. The former should contain all the configuration options such as database connection, error reporting etc, but not a byte should it output. And the latter would contain all the HTML.

Than make your code like this

<?php

include 'includes/bootstrap.php';

$id=$_SESSION['user_id'];

if(isset($_POST['close_account'])) {$stmt = $con->prepare('UPDATE users SET user_closed = 1 WHERE id = ?');$stmt->bind_param("s", $id);$stmt->execute();

session_destroy();

• I would just like to add about the "yes" and "no" part - it also helps reduce the instances where values become YES, Yes or even yes  as well as many other combination, these can then fail to get recognised as the case does not match. 1 and 0 are more difficult to get wrong. – Nigel Ren Jul 10 '20 at 8:21
• I was confused when everyone was talking about $post_id. I just took that code and threw in it there. I honestly don't know why but I was up for a long time trying to fix the code and I must've screwed up. Thanks a lot though. – user13477176 Jul 10 '20 at 21:04 • Ok I'm going to recreate my project and do it the best way I can because I was following a tutorial and it was a 2019/2020 tutorial but it apparently it was loaded with bad habits and practices. He said he wanted to do it that way to make it simple for beginners which makes zero sense. – user13477176 Jul 10 '20 at 21:12 • Would it be ok if I just started each page with html or php instead of having includes folders/files ? – user13477176 Jul 10 '20 at 21:39 To add to "Your Common Sense" answer: 1. I would suggest that the next step would be to separate your HTML and PHP code. i.e. moving out the logic for account closure into its own class since you might want to use that logic elsewhere. (DRY Principle) 1. It's best to let JS/HTML handle redirects too since that is something related to the front end facing application. 2. In a larger application, speed is of the essence and every data byte may count, due to which I would suggest declaring variables only when and where you need them. i.e. • $userLoggedIn is used within an if statement, yet it was declared outside of an if statement.
• $userLoggedIn is used only once. The question to ask is - does having this variable improve the readability of my code? If not, then just use $_SESSION['thesocial_username'].