Jump to content
PrinceTaz

Optimize My Code

Recommended Posts

Hey so I made a ToDo List as my first php script after starting to learn. I want you guys to just look at my code and tell me where I can improve. I want to drop any bad habits early on so they don't move forward with me. Also tell me where I can optimize and minimize the code so I don't write unnecessary code. Thanks! I'm going to omit things like some html that isn't crucial. You can see it live here: https://taziamoma.com/ToDoList/ .  Also try to do an SQL injection so I can see if I protected myself from it properly. Thank you!

index.php

<?php
include_once("includes/connection.php");

$errors = "";

if (isset($_POST['submit'])) {
	if (empty($_POST['task'])) {
		$errors = "You must fill in the task";
	} else {
		try {
			$task = $_POST['task'];
			$sql = "INSERT INTO todopost (title) VALUES ('$task')";
			$db->exec($sql);;
			header('Location: index.php');
		} catch(PDOException $e) {
			echo $sql. "<br>". $e->getMessage();
		}
		
	}
}

if (isset($_GET['del_task'])) {
	$stmt = $db->prepare("DELETE FROM todopost WHERE id = :id");
	$stmt->execute(array(':id' => $_GET['del_task']));
	header('Location: index.php');
}

<body>
<div class="outside">
	<div class="container">
		<div id="myDIV" class="header">
		  <h2 style="margin:5px">My To Do List</h2>
		  <form method="post" action="index.php" class="input_form">
		  	<?php if (isset($errors)) { ?>
			<p><?php echo $errors; ?></p>
			<?php } ?>
			<input type="text" name="task" class="input">
			<button type="submit" name="submit" class="addBtn">Add</button>
		</form>
		</div>


		<ul id="myUL">
			<?php 
			try {
				$stmt = $db->query('SELECT id, title FROM  todopost ORDER BY id DESC');
				while($row = $stmt->fetch()) { ?>
					<div class="li_cont">
						<li><?php echo $row['title']; ?></li>
						<a class="right" href="index.php?del_task=<?php echo $row['id'] ?>">x</a>
					</div>
				<?php }
			} catch(PDOException $e) {
				echo $e->getMessage();
			}
		  
		  	?>
		</ul>

	</div>
</div>

</body>
</html>

connection.php

<?php 
ob_start();
session_start();

$host = "localhost";
$username = "root";
$password = "";

try {
	$db = new PDO("mysql:host=$host;dbname=tazejesa_todo", $username, $password);
	$db->setAttribute(PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION);
} catch(PDOException $e) {
	echo "Connection failed: ". $e->getMessage();
}

?>

 

Share this post


Link to post
Share on other sites
Posted (edited)

There are a few things. From the top down..

$errors should be an array.
You need to check the REQUEST METHOD, not the name of a button.
You need to use Prepared Statements
You need to kill the script after header redirects
Do not output internal system errors to the user
Do not create variables for nothing
 

 

Edited by benanamen

Share this post


Link to post
Share on other sites

your INSERT query is open to sql injection. anyone can cause that query to insert data from any of your database tables, especially since you are using the 'root' user, and the rest of your code on this page will happily show the content that was just inserted from the other database/tables.

i recommend that you take this code off of a live/public site until you actually secure it against sql injection, don't use the 'root' user in you applications, create a user that has just the database permissions you need, your delete operation should use a post method form, and in real life data isn't actually deleted, it is updated to indicate it is not in use, in case it ever needs to be recovered.

Share this post


Link to post
Share on other sites
1 hour ago, benanamen said:

There are a few things. From the top down..

$errors should be an array.
You need to check the REQUEST METHOD, not the name of a button.
You need to use Prepared Statements
You need to kill the script after header redirects
Do not output internal system errors to the user
Do not create variables for nothing
 

 

What do you mean by outputing internal system errors? Also what do you mean by creating variables for nothing.

10 minutes ago, mac_gyver said:

your INSERT query is open to sql injection. anyone can cause that query to insert data from any of your database tables, especially since you are using the 'root' user, and the rest of your code on this page will happily show the content that was just inserted from the other database/tables.

i recommend that you take this code off of a live/public site until you actually secure it against sql injection, don't use the 'root' user in you applications, create a user that has just the database permissions you need, your delete operation should use a post method form, and in real life data isn't actually deleted, it is updated to indicate it is not in use, in case it ever needs to be recovered.

The "root" is just dummy data I used when I uploaded. What should I use instead of INSERT? Also, by not deleting the code, how do you manage it being getting so large?

Share this post


Link to post
Share on other sites

"Variable for nothing"

$task = $_POST['task'];

There is no change or transformation of the data. The $task variable is for nothing. You already have the POST variable, just use it.

 

"Internal System Error"

echo $sql. "<br>". $e->getMessage();

That info is completely useless to the user unless they are a hacker.

 

Share this post


Link to post
Share on other sites

Benanamen had good coverage already.  I'm repeating a few of his points I'm sure.  For a first project, it is not bad.  You are doing some good things including the use of prepared statements, although you neglected to use them for your insert.  Be consistent!  All SQL DML (insert, update, delete) should use prepared statements, and in most cases so should your select.  Always treat user input as suspect.

I posted in your other thread, so I'm repeating myself now----

Why are you using ob_start()?  That is an advanced feature that is not warranted here, nor appropriate.  Remove ob_start().

Use require_once() not include_once().  Look at what the functions do.  Ask yourself this question:  Does my script REQUIRE  connection.php to work, or would it be ok if it wasn't loaded?   It's pretty clear here that you need the script to be loaded for anything to work, so you should require it.  I never use include_once, nor should you.  

Again repeating myself, remove the closing php tag from connection.php.

Standard indentation would be 4 spaces.  You are indenting like 8 spaces --- it's crazy ?

The next thing I'd suggest would be to move all the markup into separate files that you include.  This would start you towards templates/views.  This is essentially the idea behind "separation of logic and presentation".  

Again for a first project, you are doing great.  I've seen some "professional" projects where the code wasn't as good.  Nice work, and continue to ask for clarifications when you aren't clear about the advice being given.

 

 

  • Great Answer 1

Share this post


Link to post
Share on other sites
Posted (edited)
20 hours ago, benanamen said:

"Variable for nothing"


$task = $_POST['task'];

There is no change or transformation of the data. The $task variable is for nothing. You already have the POST variable, just use it.

 

"Internal System Error"


echo $sql. "<br>". $e->getMessage();

That info is completely useless to the user unless they are a hacker.

 

Okay awesome, thank you!
 

18 hours ago, gizmola said:

Benanamen had good coverage already.  I'm repeating a few of his points I'm sure.  For a first project, it is not bad.  You are doing some good things including the use of prepared statements, although you neglected to use them for your insert.  Be consistent!  All SQL DML (insert, update, delete) should use prepared statements, and in most cases so should your select.  Always treat user input as suspect.

I posted in your other thread, so I'm repeating myself now----

Why are you using ob_start()?  That is an advanced feature that is not warranted here, nor appropriate.  Remove ob_start().

Use require_once() not include_once().  Look at what the functions do.  Ask yourself this question:  Does my script REQUIRE  connection.php to work, or would it be ok if it wasn't loaded?   It's pretty clear here that you need the script to be loaded for anything to work, so you should require it.  I never use include_once, nor should you.  

Again repeating myself, remove the closing php tag from connection.php.

Standard indentation would be 4 spaces.  You are indenting like 8 spaces --- it's crazy ?

The next thing I'd suggest would be to move all the markup into separate files that you include.  This would start you towards templates/views.  This is essentially the idea behind "separation of logic and presentation".  

Again for a first project, you are doing great.  I've seen some "professional" projects where the code wasn't as good.  Nice work, and continue to ask for clarifications when you aren't clear about the advice being given.

 

 

Okay I'll keep that in mind. I'll switch to require_once(). Also, by separation of files, you're referring to moving things like the header html to a separate file and just including it? Or should I require it? I understand "templates" but what do you mean by views? Also, I normally indent to know what is nested into each other and I normally just press the tab key. Should I just use the space bar or just not indent as much?

I appreciate you for your honesty and your criticism :D

Edited by PrinceTaz

Share this post


Link to post
Share on other sites
6 hours ago, PrinceTaz said:

Also, I normally indent to know what is nested into each other and I normally just press the tab key. Should I just use the space bar or just not indent as much?

Your editor probably has a setting to control how wide a tab is displayed as.  That's one of the arguments in the tabs vs spaces debate when it comes to indenting code.  Maybe your editor displays a tab as 2 or 4 spaces so it looks reasonable but here in the browser it's shown very wide, making it annoying because it doesn't take long for half the line to be empty space.

You'll have to decide for your self if you prefer tabs or spaces, just be consistent and configure your editor appropriately.  Many coding editors have a tab-inserts-spaces option so you can still hit tab to indent but it'll insert space characters rather than a tab character.

6 hours ago, PrinceTaz said:

I understand "templates" but what do you mean by views?

A view is a template essentially, just another name for it.  You'd move the HTML out of your main code file into it's own file.  Limit the PHP usage in your html files to simple echo/conditional/loop statements.

Do all your typical logic and processing code in your main PHP file and assign variables as needed for your template, then require whichever template is needed for that branch of code.

Your common header/footer html can be in separate files then either required in from either your php code or as part of your template file.

  • Like 1

Share this post


Link to post
Share on other sites
8 hours ago, kicken said:

Your editor probably has a setting to control how wide a tab is displayed as.  That's one of the arguments in the tabs vs spaces debate when it comes to indenting code.  Maybe your editor displays a tab as 2 or 4 spaces so it looks reasonable but here in the browser it's shown very wide, making it annoying because it doesn't take long for half the line to be empty space.

You'll have to decide for your self if you prefer tabs or spaces, just be consistent and configure your editor appropriately.  Many coding editors have a tab-inserts-spaces option so you can still hit tab to indent but it'll insert space characters rather than a tab character.

A view is a template essentially, just another name for it.  You'd move the HTML out of your main code file into it's own file.  Limit the PHP usage in your html files to simple echo/conditional/loop statements.

Do all your typical logic and processing code in your main PHP file and assign variables as needed for your template, then require whichever template is needed for that branch of code.

Your common header/footer html can be in separate files then either required in from either your php code or as part of your template file.

Ah okay thank you! Now would it be smart to move the middle area to another file as well? Like header, middle content, and then footer? Then index.php would just be three include files?

Share this post


Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.


×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.