PrinceTaz Posted June 1, 2019 Share Posted June 1, 2019 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(); } ?> Quote Link to comment Share on other sites More sharing options...
benanamen Posted June 1, 2019 Share Posted June 1, 2019 (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 June 1, 2019 by benanamen Quote Link to comment Share on other sites More sharing options...
mac_gyver Posted June 2, 2019 Share Posted June 2, 2019 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. Quote Link to comment Share on other sites More sharing options...
PrinceTaz Posted June 2, 2019 Author Share Posted June 2, 2019 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? Quote Link to comment Share on other sites More sharing options...
benanamen Posted June 2, 2019 Share Posted June 2, 2019 "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. Quote Link to comment Share on other sites More sharing options...
gizmola Posted June 2, 2019 Share Posted June 2, 2019 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. 1 Quote Link to comment Share on other sites More sharing options...
PrinceTaz Posted June 3, 2019 Author Share Posted June 3, 2019 (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 Edited June 3, 2019 by PrinceTaz Quote Link to comment Share on other sites More sharing options...
kicken Posted June 3, 2019 Share Posted June 3, 2019 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. 1 Quote Link to comment Share on other sites More sharing options...
PrinceTaz Posted June 3, 2019 Author Share Posted June 3, 2019 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? Quote Link to comment Share on other sites More sharing options...
Recommended Posts
Join the conversation
You can post now and register later. If you have an account, sign in now to post with your account.