Refactoring this code...

phreak3r · December 4, 2018

I was working on this project earlier on in the year, I have not posted here much. I would like to get it over with and start something else. I have other files which look like the excerpt of code from a file below. I personally think that my code lacks structure and could be organized in a better fashion. I lost most of my progress and am having to backtrack and restore code. Is there a way to re-write this code and make it more readable? Please and thank you!

<?php
include('header.php');
require_once('dbcon/dbcon.php');
//include('functions.php');

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
	// sanitize values before entering them into db, no bad seeds.
	$username = mysqli_real_escape_string($conn, $_POST['username']);
	$password = mysqli_real_escape_string($conn, $_POST['password']);
	$bio = mysqli_real_escape_string($conn, $_POST['bio']);
	$hashed_password = mysqli_real_escape_string($conn, password_hash($password, PASSWORD_DEFAULT));
	$email = mysqli_real_escape_string($conn, $_POST['email_address']);
	$confirmation_status = 0;

/*	function sanitizeValues($x, string $postString) {
		$x = mysqli_real_escape_string($conn, $_POST[$postString]);
	}*/

	$username_query = "SELECT * from profiles001 WHERE username='$username'";
	$result = mysqli_query($conn, $username_query);

	// if username exists do not continue...
	if (mysqli_num_rows($result) > 0) {
		header('Location: /soapbox/signup.php');
		// let user know that username is taken...
	} else {
		// file upload stuff...
		$file = $_FILES['file'];
		$fileName = $_FILES['file']['name'];
		$fileTmpName = $_FILES['file']['tmp_name'];
		$fileSize = $_FILES['file']['size'];
		$fileError = $_FILES['file']['error'];
		$fileType = $_FILES['file']['type'];
		$fileExt = explode('.', $fileName);
		$fileActualExt = strtolower(end($fileExt));

		$allowed = array('jpg', 'jpeg', 'png');

		// avatar file constraints checks...
		if (in_array($fileActualExt, $allowed)) {
			if ($fileError === 0) {
				if ($fileSize < 1000000) {
					$fileNameNew = uniqid($_SESSION['username'], true) . "." . $fileActualExt;
					$fileDestination = 'uploads/' . $fileNameNew;
					move_uploaded_file($fileTmpName, $fileDestination);

				} else {
					echo "Your file is too big!";
				}
			} else {
				echo "There was an error uploading your file" . $fileError . $fileSize;
			}
		} else if (!(empty(in_array($fileActualExt, $allowed))) && !($allowed)) {
			echo "Cannot upload file of this type!";
		}

		mkdir("channel/" . $username);
		mkdir("channel/" . $username . "/videos");
		fopen("channel/" . $username . "/index.php", "w");

		$account_open_date = date("Y-m-d h:i:s");
		$current_date = date("Y-m-d h:i:s");
		//$account_open_date_retrieval_sql_select = "SELECT account_open_date from profile0";
		//$account_age = date_diff($row, $current_date); // acct open date - current date = account age
		//$account_age_result = mysqli_query($conn, $account_open_date_retrieval_sql_select);
		//$row = mysqli_fetch_assoc($account_age_result);

		// if-then-else-if statement to get rid of the fileDestination var undefined error when avatar photo is not submitted....
		if (!(empty($fileDestination))) {
			$sqlinsert = "INSERT INTO profiles001 (username, password, email, c_status, doc, avatar, bio) VALUES ('$username', '$hashed_password', '$email', '$confirmation_status', '$account_open_date', '$fileDestination', '$bio')";
		} else if (empty($fileDestination)) {
			$fileDestination = "assets/soap.jpg";
			$sqlinsert = "INSERT INTO profiles001 (username, password, email, c_status, doc, avatar, bio) VALUES ('$username', '$hashed_password', '$email', '$confirmation_status', '$account_open_date', '$fileDestination', '$bio')";
		}

		$result = mysqli_query($conn, $sqlinsert);
	}
}
?>

Barand · December 4, 2018

Use prepared queries (preferably with PDO instead of mysqli) intead of all the mysqli_real_escape_string() calls.

Don't needlessly create variables from othe variables. EG having called

$file = $_FILES['file'];

you can then use $file['name'], $file['type'] in your code instead of creating a new variable ($fileName, $fileType etc) for each one.

Always exit() after a header() redirect call.

Example code using PDO prepared queries

if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    $hashed_password = password_hash($_POST['password'], PASSWORD_DEFAULT); 
    $username_query = $conn->prepare("SELECT * from profiles001 WHERE username=?");
    $username_query->execute( [ $_POST['username'] ] );
    
    if ($username_query->fetch() != false) {
        header('Location: /soapbox/signup.php');
        exit;
    }
    else {
        $file = $_FILES['file'];
        
                 # other code here #
                 
        if (empty($fileDestination)) $fileDestination = "assets/soap.jpg";
        
        $sqlInsert = $conn->prepare("INSERT INTO profiles001 
                                    (username, password, email, c_status, doc, avatar, bio) 
                                    VALUES 
                                    (?, ?, ?, ?, NOW() , ? , ? )");
        $sqlInsert->execute( [ $username, $hashed_password, $email, $confirmation_status, $fileDestination, $bio ] );
    }
}

phreak3r · December 4, 2018

2 hours ago, Barand said:
Use prepared queries (preferably with PDO instead of mysqli) intead of all the mysqli_real_escape_string() calls.

Don't needlessly create variables from othe variables. EG having called
$file = $_FILES['file'];
you can then use $file['name'], $file['type'] in your code instead of creating a new variable ($fileName, $fileType etc) for each one.

Always exit() after a header() redirect call.

Example code using PDO prepared queries
if ($_SERVER['REQUEST_METHOD'] == 'POST') {
    $hashed_password = password_hash($_POST['password'], PASSWORD_DEFAULT); 
    $username_query = $conn->prepare("SELECT * from profiles001 WHERE username=?");
    $username_query->execute( [ $_POST['username'] ] );
    
    if ($username_query->fetch() != false) {
        header('Location: /soapbox/signup.php');
        exit;
    }
    else {
        $file = $_FILES['file'];
        
                 # other code here #
                 
        if (empty($fileDestination)) $fileDestination = "assets/soap.jpg";
        
        $sqlInsert = $conn->prepare("INSERT INTO profiles001 
                                    (username, password, email, c_status, doc, avatar, bio) 
                                    VALUES 
                                    (?, ?, ?, ?, NOW() , ? , ? )");
        $sqlInsert->execute( [ $username, $hashed_password, $email, $confirmation_status, $fileDestination, $bio ] );
    }
}

PDO, yes. I was using it before, but as I said, I lost my recent work and have to start with what I have got. My database connection configuration 'stuff' is PDO, I am moving away from mysqli slowly but surely. Thank you.

Psycho · December 4, 2018

Instead of one long procedural body of code, create functions or classes for certain operations - especially if you need to do the same thing anywhere else in your application. That way you can create intuitive calls within your code that makes it much easier to read/manage. For example, you could create a function called usernameExists($uname) that returns a TRUE?FALSE based on whether the passed username already exists or not. Then also create a function to create a new user.

Try to avoid "SELECT *" in your queries. Only select the fields you need. Otherwise, you can create conditions that leak data. In this case you are just checking if the record exists, so select the username or some other innocuous field. Alternatively, you could do a COUNT(*) query.

Your process to see if a record exists with one query before running another query to create a record is problematic. It is possible for a "race condition" to occur which would allow a duplicate to be created. You should instead create the DB table to ensure that field is unique. Then just try to perform the insert. If it fails, check the error to see if it was due to a duplicate.

Lastly, use comments! It may seem obvious when you are writing code what is happening, but when you have to come back later or if someone else has to work on the code it is invaluable.

Here's a slight update to the code Barand posted with some modifications.

	//Function to see if a username exists
function usernameExists($uname)
{
    $username_query = $conn->prepare("SELECT username from profiles001 WHERE username=?");
    $username_query->execute( [ $_POST['username'] ] );
    return ($username_query->fetch() != false);
}
	//Function to create a new user
function createUser($userDataAry)
{
    $sqlInsert = $conn->prepare("INSERT INTO profiles001 
                                (username, password, email, c_status, doc, avatar, bio) 
                                VALUES 
                                (?, ?, ?, ?, NOW() , ? , ? )");
    $sqlInsert->execute( $userDataAry );
}
	if ($_SERVER['REQUEST_METHOD'] == 'POST') {
	    //Check if username already eists
    if (usernameExists($uname)) {
        header('Location: /soapbox/signup.php');
        exit;
    }
	    //Get data from the $_FILES array
    $file = $_FILES['file'];
	             # other code here #
                 
    if (empty($fileDestination)) $fileDestination = "assets/soap.jpg";
     //Create the user
    $hashed_password = password_hash($_POST['password'], PASSWORD_DEFAULT); 
    createUser( [ $username, $hashed_password, $email, $confirmation_status, $fileDestination, $bio ] );
}

Barand · December 4, 2018

Don't forget to pass the "$conn" value, as well as the data, as arguments to those functions.

phreak3r · December 4, 2018

11 minutes ago, Psycho said:
Instead of one long procedural body of code, create functions or classes for certain operations - especially if you need to do the same thing anywhere else in your application. That way you can create intuitive calls within your code that makes it much easier to read/manage. For example, you could create a function called usernameExists($uname) that returns a TRUE?FALSE based on whether the passed username already exists or not. Then also create a function to create a new user.

Try to avoid "SELECT *" in your queries. Only select the fields you need. Otherwise, you can create conditions that leak data. In this case you are just checking if the record exists, so select the username or some other innocuous field. Alternatively, you could do a COUNT(*) query.

Your process to see if a record exists with one query before running another query to create a record is problematic. It is possible for a "race condition" to occur which would allow a duplicate to be created. You should instead create the DB table to ensure that field is unique. Then just try to perform the insert. If it fails, check the error to see if it was due to a duplicate.

Lastly, use comments! It may seem obvious when you are writing code what is happening, but when you have to come back later or if someone else has to work on the code it is invaluable.

Here's a slight update to the code Barand posted with some modifications.
	//Function to see if a username exists
function usernameExists($uname)
{
    $username_query = $conn->prepare("SELECT username from profiles001 WHERE username=?");
    $username_query->execute( [ $_POST['username'] ] );
    return ($username_query->fetch() != false);
}
	//Function to create a new user
function createUser($userDataAry)
{
    $sqlInsert = $conn->prepare("INSERT INTO profiles001 
                                (username, password, email, c_status, doc, avatar, bio) 
                                VALUES 
                                (?, ?, ?, ?, NOW() , ? , ? )");
    $sqlInsert->execute( $userDataAry );
}
	if ($_SERVER['REQUEST_METHOD'] == 'POST') {
	    //Check if username already eists
    if (usernameExists($uname)) {
        header('Location: /soapbox/signup.php');
        exit;
    }
	    //Get data from the $_FILES array
    $file = $_FILES['file'];
	             # other code here #
                 
    if (empty($fileDestination)) $fileDestination = "assets/soap.jpg";
     //Create the user
    $hashed_password = password_hash($_POST['password'], PASSWORD_DEFAULT); 
    createUser( [ $username, $hashed_password, $email, $confirmation_status, $fileDestination, $bio ] );
}
	
I am working towards getting various things grouped in functions. I have this one particular problem, now it is a parsing error:

[Tue Dec 04 13:54:03.526532 2018] [php7:emerg] [pid 2463] [client 127.0.0.1:56624] PHP Parse error: syntax error, unexpected '$options' (T_VARIABLE) in /var/www/html/soapbox/dbcon/dbcon.php on line 9, referer: http://localhost/soapbox/signup.php

And here's the dbcon.php file code:
<?php
$host   = "localhost";
$database = "soapbox";
$username = "drb";
$password = "m1n3craft";
$charset = "utf8mb4"

// Create connection
$options = [
	PDO::ATTR_ERRMODE 		=> 	PDO::ERRMODE EXCEPTION,
	PDO::ATTR_DEFAULT_FETCH_MODE 	=>	PDO::FETCH_ASSOC,
	PDO::ATTR_EMULATE_PREPARES	=>	false,
];

	try {
 		$pdo = new PDO('mysql:host=$host;dbname=$database;charset=$charset', $username, $password, $options);
	} catch (\PDOException $e){
		throw new \PDOException($e->getMessage(), (int)$e->getCode());
	}

?>

phreak3r · December 4, 2018

30 minutes ago, Barand said:

Don't forget to pass the "$conn" value, as well as the data, as arguments to those functions.

Can't pass something that is broken, haha. I am having to re-write and fix things.

Barand · December 4, 2018

Stop quoting whole posts in your replies. If you need to quote, just quote the bit/s you are responding to.

Sign In

Refactoring this code...

Recommended Posts

phreak3r

Link to comment

Share on other sites

Barand

Link to comment

Share on other sites

phreak3r

Link to comment

Share on other sites

Psycho

Link to comment

Share on other sites

Barand

Link to comment

Share on other sites

phreak3r

Link to comment

Share on other sites

phreak3r

Link to comment

Share on other sites

Barand

Link to comment

Share on other sites

Join the conversation

Browse

Activity

Important Information