orgzchaos Posted November 17, 2012 Share Posted November 17, 2012 Please find below a part of code that I am working with. Please suggest some improvements as to make this code more presentable professionally. <?php include "base.php"; echo" <!DOCTYPE HTML PUBLIC \"-//W3C//DTD HTML 4.01//EN\" \"http://www.w3.org/TR/html4/strict.dtd\"> <html> <head> <title>User Management</title> <script language='Javascript'> checked = false; function checkedAll () { if (checked == false){checked = true}else{checked = false} for (var i = 0; i < document.getElementById('userlisting').elements.length; i++) { document.getElementById('userlisting').elements[i].checked = checked; } } </script> <style type=\"text/css\"> img.footerimgA {vertical-align:-23%; margin-right: 5px;border: none!important;} img.footerimgB {vertical-align:-26%; margin-right: 5px;border: none!important;} img.footerimgC {vertical-align: -30%; margin-right: 5px;border: none!important;} .breaklarge{ margin-left: 320px; } .breaksmall{ margin-left: 30px; } .pagination{ margin:2px; width:100px; } a:link {color:#000000;} a:visited {color:#000000;} a:hover {color:#000000;} a:active {color:#000000;} a:link {text-decoration:none;} a:visited {text-decoration:none;} a:hover {text-decoration:none;} a:active {text-decoration:none;} #mainbox { margin-left:200px; margin-right:200px; } #footer { font-family:\"Arial\",\"Trebuchet MS\", Arial, Helvetica, sans-serif; font-size:0.7em; width:100%; margin-top:20px; } #customers { font-family:\"Arial\",\"Trebuchet MS\", Arial, Helvetica, sans-serif; width:100%; border-collapse:collapse; } #customers td, #customers th { font-size:0.8em; border:1px solid #006699; padding:3px 7px 2px 7px; } #customers tr td:first-child, #customers tr th:first-child {background-color:#FFFFFF; width:1px; text-align:center;} #customers tr td:last-child, #customers tr th:last-child {width:78px;} #customers tr td:nth-child(5), #customers tr th:nth-child(5) {width:10px; text-align:center;} #customers th { font-size:0.8em; text-align:left; padding-top:2px; padding-bottom:1px; background-color:#006699; color:#ffffff; } #customers td { padding-top:1.2px; padding-bottom:0.7px; font-size:0.75em; color:#000000; background-color:#FFFFFF; } #customers tr.alt td { color:#000000; background-color:#EAF2D3; } #customers imgspc { margin-left:5px; margin-right:5px; } </style> </head> <body> <div id=\"mainbox\"> <form id=\"userlisting\" action=\"users.php\" method=\"post\"> <table id=\"customers\"> <tr> <th><input name=\"checkall\" type=\"checkbox\" onclick=\"checkedAll();\" title=\"Select/Deselect All\"></th> <th>Username</th> <th>Name</th> <th>Email Address</th> <th>Level</th> <th>Options</th> </tr>"; if (isset($_POST['resultsperpage'])) { $_SESSION['resultsperpage'] = $_POST['resultsperpage']; } if (isset($_GET['id']) && is_numeric($_GET['id'])) { // get id value $id = $_GET['id']; // delete the entry $result = mysql_query("DELETE FROM persons WHERE id=$id") or die(mysql_error()); } //check if multiple entries are selected if (isset($_POST['usercheckbox'])) { if (is_array($_POST['usercheckbox'])){ $id=$_POST['usercheckbox']; $result = mysql_query("DELETE FROM persons WHERE id IN (" . implode(', ', $id) . ")") or die(mysql_error()); } } $result = mysql_query("SELECT * FROM persons ORDER BY id DESC"); $total_results = mysql_num_rows($result); $total_pages = ceil($total_results / $_SESSION['resultsperpage']); if ($_SESSION['resultsperpage'] == 100) { $_SESSION['resultsperpage'] = $total_results; } if (isset($_GET['page']) && is_numeric($_GET['page'])) { $show_page = $_GET['page']; // make sure the $show_page value is valid if ($show_page > 0 && $show_page <= $total_pages) { $start = ($show_page -1) * $_SESSION['resultsperpage']; $end = $start + $_SESSION['resultsperpage']; } else { // error - show first set of results $start = 0; $end = $_SESSION['resultsperpage']; } } else { // if page isn't set, show first set of results $show_page = 1; $start = 0; $end = $_SESSION['resultsperpage']; } echo "</p>"; for ($i = $start; $i < $end; $i++) { // make sure that PHP doesn't try to show results that don't exist if ($i == $total_results) { break; } echo" <tr><td><input name=\"usercheckbox[]\" type=\"checkbox\" value=\"".mysql_result($result, $i, 'id')."\" title=\"Select/Deselect User\"></td> <td>".mysql_result($result, $i, 'username')."</td> <td>".mysql_result($result, $i, 'name')."</td> <td>".mysql_result($result, $i, 'email')."</td> <td>".mysql_result($result, $i, 'level')."</td> <td><imgspc><a href=\"view.php?id=".mysql_result($result, $i, 'id')."\"><img src=\"images/view.png\" title=\"View\"></imgspc><imgspc><a href=\"edit.php?id=".mysql_result($result, $i, 'id')."\"><img src=\"images/edit.png\" title=\"Edit\"></imgspc><imgspc><a href=\"delete.php?id=".mysql_result($result, $i, 'id')."\"><img src=\"images/remove.png\" title=\"Delete\"></imgspc></tr>"; } echo"</table></form><form id=\"resultsperpage\" action=\"users.php\" method=\"post\"><div id=\"footer\"> <div style=\"float: left\">Results per page: <select style=\"font-size:10px;\" name=\"resultsperpage\" onchange=\"this.form.submit()\">"; for ($i = 10; $i <= 90; $i += 10) { if ( $_SESSION['resultsperpage'] == $i ) { echo "<option value=\"".$i."\" selected=\"yes\">".$i."</option>"; } else { echo "<option value=\"".$i."\">".$i."</option>"; } } if ( $_SESSION['resultsperpage'] == $total_results ) { echo "<option value=\"100\" selected=\"yes\">All</option>"; } else { echo "<option value=\"100\">All</option>"; } echo "</select></div></form><div style=\"float: right\";><a href=\"search.php\" title=\"Search\"><img src=\"images/search.png\" class=\"footerimgC\">Search</a><span class=\"breaksmall\"> </span><a href=\"newuser.php\" title=\"Add New User\"><img src=\"images/adduser.png\" class=\"footerimgA\">Add New</a><span class=\"breaksmall\"> </span><a href=\"#\" title=\"Delete Selected Users\" onclick=\"userlisting.submit()\"><img src=\"images/deletefooter.png\" class=\"footerimgA\">Delete Selected</a><span class=\"breaksmall\"></span><span class=\"pagination\">"; if ( $total_pages == 0 ) { echo "No entries"; } else { if ( $show_page > 1 ) { $tmppage=$show_page-1; } else { $tmppage=1; }; echo "<a href='users.php?page=".$tmppage."'><img src='images/goback.png' class='footerimgB' title='Previous Page'></a>"; } for ($i = 1; $i <= $total_pages; $i++) { if ( $i == $show_page ) { echo"<b> <a href='users.php?page=$i'>$i</a></b>"; } else { echo" <a href='users.php?page=$i'>$i</a>"; } } if ( $show_page < $total_pages ) { $tmppage=$show_page+1; } else { $tmppage=$show_page; }; echo "  <a href='users.php?page=".$tmppage."'><img src='images/goforward.png' class='footerimgB' title='Next Page'></a></span></div> </div> </div> </body> </html>"; ?> Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/ Share on other sites More sharing options...
requinix Posted November 17, 2012 Share Posted November 17, 2012 There's one big suggestion I'll put forward: don't echo giant strings of HTML. Drop out of PHP with a ?> then go back in with a <?php when you're done. Biggest advantage is that your IDE can provide support for the HTML (can't do that when it's in a string) but another important point is how you won't have to worry about escaping quotes or variables. Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/#findComment-1393168 Share on other sites More sharing options...
orgzchaos Posted November 17, 2012 Author Share Posted November 17, 2012 There's one big suggestion I'll put forward: don't echo giant strings of HTML. Drop out of PHP with a ?> then go back in with a <?php when you're done. Biggest advantage is that your IDE can provide support for the HTML (can't do that when it's in a string) but another important point is how you won't have to worry about escaping quotes or variables. Noted, any others? Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/#findComment-1393170 Share on other sites More sharing options...
jcbones Posted November 17, 2012 Share Posted November 17, 2012 Put all of your HTML together, and all of your PHP Processing together. You should only have "echo'd" PHP inside the HTML, this will help with de-bugging. We call it: separation of logic and output. Which is why most of us use some sort of template system. Other than that, I have never been a fan of that style of pagination. I think it is wasteful use of resources, and on huge tables, it would be much slower than a 'count' query feeding a limited query. Small tables, I think it would be equally as fast. Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/#findComment-1393182 Share on other sites More sharing options...
orgzchaos Posted November 17, 2012 Author Share Posted November 17, 2012 Put all of your HTML together, and all of your PHP Processing together. You should only have "echo'd" PHP inside the HTML, this will help with de-bugging. We call it: separation of logic and output. Which is why most of us use some sort of template system. Other than that, I have never been a fan of that style of pagination. I think it is wasteful use of resources, and on huge tables, it would be much slower than a 'count' query feeding a limited query. Small tables, I think it would be equally as fast. Noted the earlier comment and adjusting my codes. Secondly, on the later half you are referring to something like this? Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/#findComment-1393237 Share on other sites More sharing options...
PFMaBiSmAd Posted November 17, 2012 Share Posted November 17, 2012 (edited) Yes, pagination normally just retrieves the rows you are interested in by using a LIMIT x,y clause in the query. edit: what you are currently doing, I jokingly refer to as rowination. Also, for your existing code, mysql_result is the slowest way of accessing data because each time you call it, it performs a data seek. Your existing code would be better off by performing an explicit data seek to the 1st row you are interested in before the start of your loop, then use a normal mysql_fetch_xxxxxx statement in the loop to fetch an entire row at a time and advance to the next row in the result set. Edited November 17, 2012 by PFMaBiSmAd Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/#findComment-1393239 Share on other sites More sharing options...
orgzchaos Posted November 17, 2012 Author Share Posted November 17, 2012 I have adjusted the code reading the tips above besides getting rid of the PHP echos. If I invert the process i.e. use <?php ?> tags instead of the HTML echos then there will be way too many php tags in the code. Any comments on that? Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/#findComment-1393293 Share on other sites More sharing options...
Christian F. Posted November 18, 2012 Share Posted November 18, 2012 You can always use the HereDoc syntax to write/define long section of strings, but personally I prefer to avoid long strings like that in my PHP scripts. If possible, I recommend moving it to the end of the file, as pure HTML code. Then use <?php echo $var; ?> whenever you want the variable content to be displayed. You would, of course, need to store the content you'd otherwise echo inside said variable(s) instead. This is quite possible with the first echo in your code, especially since it doesn't contain any variable content. Which means you can just add it all to the bottom, below the closing PHP tag. Then save the content created inside the IF-blocks, and print it out inside the HTML content (as shown above). Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/#findComment-1393305 Share on other sites More sharing options...
PFMaBiSmAd Posted November 18, 2012 Share Posted November 18, 2012 Here's a post about this very subject - pagination, building content in variables, and outputting the content in a html document at the end of the code, after all the 'business' logic - http://forums.phpfreaks.com/topic/270523-can-you-echo-a-var-that-is-defined-later-on-in-the-script/#entry1391569 Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/#findComment-1393331 Share on other sites More sharing options...
orgzchaos Posted November 18, 2012 Author Share Posted November 18, 2012 (edited) You can always use the HereDoc syntax to write/define long section of strings, but personally I prefer to avoid long strings like that in my PHP scripts. If possible, I recommend moving it to the end of the file, as pure HTML code. Then use <?php echo $var; ?> whenever you want the variable content to be displayed. You would, of course, need to store the content you'd otherwise echo inside said variable(s) instead. This is quite possible with the first echo in your code, especially since it doesn't contain any variable content. Which means you can just add it all to the bottom, below the closing PHP tag. Then save the content created inside the IF-blocks, and print it out inside the HTML content (as shown above). Here's a post about this very subject - pagination, building content in variables, and outputting the content in a html document at the end of the code, after all the 'business' logic - http://forums.phpfre...t/#entry1391569 All recommendations followed and below is what I got. <?php include "base.php"; if (!isset($table_content)) { $table_content=""; } if (!isset($pagination_options)) { $pagination_options=""; } if (!isset($pagination_links)) { $pagination_links=""; } if (!isset($total_results)) { $total_results=""; } $result = mysql_query("SELECT COUNT(*) FROM persons"); $rows = mysql_fetch_row($result); $numrows = $rows[0]; if (isset($_POST['resultsperpage'])) { $_SESSION['resultsperpage'] = $_POST['resultsperpage']; } else if (!isset($_SESSION['resultsperpage'])) { $_SESSION['resultsperpage'] = 10; } $totalpages = ceil($numrows / $_SESSION['resultsperpage']); if (isset($_GET['id']) && is_numeric($_GET['id'])) { // get id value $id = $_GET['id']; // delete the entry $result = mysql_query("DELETE FROM persons WHERE id=$id") or die(mysql_error()); } //check if multiple entries are selected if (isset($_POST['usercheckbox'])) { if (is_array($_POST['usercheckbox'])){ $id=$_POST['usercheckbox']; $result = mysql_query("DELETE FROM persons WHERE id IN (" . implode(', ', $id) . ")") or die(mysql_error()); } } if (isset($_GET['currentpage']) && is_numeric($_GET['currentpage'])) { $currentpage = (int) $_GET['currentpage']; } else { $currentpage = 1; } if ($currentpage > $totalpages) { $currentpage = $totalpages; } if ($currentpage < 1) { $currentpage = 1; } $offset = ($currentpage - 1) * $_SESSION['resultsperpage']; $limit = $_SESSION['resultsperpage']; $result = mysql_query("SELECT id,username,name,email,level FROM persons LIMIT $offset,$limit"); while ($list = mysql_fetch_assoc($result)) { $id = $list['id']; $username = $list['username']; $name = $list['name']; $email = $list['email']; $level = $list['level']; $table_content .= "<tr><td><input name='usercheckbox[]' type='checkbox' value='".$id."' title='Select/Deselect User'></td> <td>".$username."</td> <td>".$name."</td> <td>".$email."</td> <td>".$level."</td> <td><a href='view.php?id=".$id."'><img src='images/view.png' title='View details' class='menuentry'><a href='edit.php?id=".$id."'><img src='images/edit.png' title='Edit' class='menuentry'><a href='delete.php?id=".$id."'><img src='images/remove.png' title='Delete' class='menuentry'></tr>"; } for ($i = 10; $i <= 90; $i += 10) { if ( $_SESSION['resultsperpage'] == $i ) { $pagination_options .= "<option value='".$i."' selected='yes'>".$i."</option>"; } else { $pagination_options .= "<option value='".$i."'>".$i."</option>"; } } if ( $_SESSION['resultsperpage'] == $total_results ) { $pagination_options .= "<option value='100' selected='yes'>All</option>"; } else { $pagination_options .= "<option value='100'>All</option>"; } if ( $totalpages == 0 ) { $pagination_links .= "No entries"; } else { if ( $currentpage > 1 ) { $tmppage=$currentpage-1; } else { $tmppage=1; }; $pagination_links .= "<a href='usersopt.php?currentpage=".$tmppage."'><img src='images/goback.png' class='footerimgB' title='Previous Page'></a>"; } for ($i = 1; $i <= $totalpages; $i++) { if ( $i == $currentpage ) { $pagination_links .= "<b> <a href='usersopt.php?currentpage=$i'>$i</a></b>"; } else { $pagination_links .= " <a href='usersopt.php?currentpage=$i'>$i</a>"; } } if ( $currentpage < $totalpages ) { $tmppage=$currentpage+1; } else { $tmppage=$currentpage; }; $pagination_links .= "  <a href='usersopt.php?currentpage=".$tmppage."'><img src='images/goforward.png' class='footerimgB' title='Next Page'></a></span></div>"; ?> <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd"> <html> <head> <title>User Management</title> <script language='Javascript'> checked = false; function checkedAll () { if (checked == false){checked = true}else{checked = false} for (var i = 0; i < document.getElementById('userlisting').elements.length; i++) { document.getElementById('userlisting').elements[i].checked = checked; } } </script> <style type="text/css"> img.footerimgA {vertical-align:-23%; margin-right: 5px;border: none!important;} img.footerimgB {vertical-align:-26%; margin-right: 5px;border: none!important;} img.footerimgC {vertical-align: -30%; margin-right: 5px;border: none!important;} img.menuentry { border: none!important; margin-left:5px; margin-right:5px; } .breaklarge{ margin-left: 320px; } .breaksmall{ margin-left: 30px; } .pagination{ margin:2px; width:100px; } a:link {color:#000000;} a:visited {color:#000000;} a:hover {color:#000000;} a:active {color:#000000;} a:link {text-decoration:none;} a:visited {text-decoration:none;} a:hover {text-decoration:none;} a:active {text-decoration:none;} #mainbox { margin-left:200px; margin-right:200px; margin-top:50px; } #footer { font-family:"Arial","Trebuchet MS", Arial, Helvetica, sans-serif; font-size:0.7em; width:100%; margin-top:20px; } #customers { font-family:"Arial","Trebuchet MS", Arial, Helvetica, sans-serif; width:100%; border-collapse:collapse; } #customers td, #customers th { font-size:0.8em; border:1px solid #006699; padding:3px 7px 2px 7px; } #customers tr td:first-child, #customers tr th:first-child {background-color:#FFFFFF; width:1px; text-align:center;} #customers tr td:last-child, #customers tr th:last-child {width:78px;} #customers tr td:nth-child(5), #customers tr th:nth-child(5) {width:10px; text-align:center;} #customers th { font-size:0.8em; text-align:left; padding-top:2px; padding-bottom:1px; background-color:#006699; color:#ffffff; } #customers td { padding-top:1.2px; padding-bottom:0.7px; font-size:0.75em; color:#000000; background-color:#FFFFFF; } #customers tr.alt td { color:#000000; background-color:#EAF2D3; } </style> </head> <body> <div id="mainbox"> <form id="userlisting" action="usersopt.php" method="post"> <table id="customers"> <tr> <th><input name="checkall" type="checkbox" onclick="checkedAll();" title="Select/Deselect All"></th> <th>Username</th> <th>Name</th> <th>Email Address</th> <th>Level</th> <th>Options</th> </tr> <?php echo $table_content; ?> </table></form><form id="resultsperpage" action="usersopt.php" method="post"><div id="footer"> <div style="float: left">Results per page: <select style="font-size:10px;" name="resultsperpage" onchange="this.form.submit()"> <?php echo $pagination_options; ?> </select></div></form><div style="float: right";><a href="search.php" title="Search"><img src="images/search.png" class="footerimgC">Search</a><span class="breaksmall"> </span><a href="newuser.php" title="Add new user"><img src="images/adduser.png" class="footerimgA">Add New</a><span class="breaksmall"> </span><a href="#" title="Delete selected users" onclick="userlisting.submit()"><img src="images/deletefooter.png" class="footerimgA">Delete Selected</a><span class="breaksmall"></span><span class="pagination"> <?php echo $pagination_links; ?> </div> </div> </body> </html> I am happy how PHP tags are reduced to just three but still the long concatenated and initial declarations ( the ones at the top ) didn't made things looks perfect thou I have explored the options and there seems little that can be done about it without getting more PHP tags in the code, unless off course there is a work around that I am not aware of. Anyways, please let me know if there is something else that I should be looking out for. Thanks Edited November 18, 2012 by orgzchaos Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/#findComment-1393349 Share on other sites More sharing options...
ignace Posted November 18, 2012 Share Posted November 18, 2012 (edited) while ($list = mysql_fetch_assoc($result)) { $id = $list['id']; $username = $list['username']; $name = $list['name']; $email = $list['email']; $level = $list['level']; $table_content .= "<tr><td><input name='usercheckbox[]' type='checkbox' value='".$id."' title='Select/Deselect User'></td> <td>".$username."</td> <td>".$name."</td> <td>".$email."</td> <td>".$level."</td> <td><a href='view.php?id=".$id."'><img src='images/view.png' title='View details' class='menuentry'><a href='edit.php?id=".$id."'><img src='images/edit.png' title='Edit' class='menuentry'><a href='delete.php?id=".$id."'><img src='images/remove.png' title='Delete' class='menuentry'></tr>"; } You can reduce this further so that you have one part that retrieves the rows and puts them into an array and then afterwards in your html: <?php foreach ($rows as $row): ?> <tr> <td><?php echo $row['username'] ?></td> <td><?php echo $row['name'] ?></td> <td><?php echo $row['email'] ?></td> <td><?php echo $row['level'] ?></td> <td> <a href="/view.php?id=<?php echo $row['id'] ?>"> <img src="/images/view.png" title="View details" class="menuentry"> </a><!-- you forgot these --> <a href="/edit.php?id=<?php echo $row['id'] ?>"> <img src="/images/edit.png" title="Edit" class="menuentry"> </a><!-- you forgot these --> <a href="/delete.php?id=<?php echo $row['id'] ?>"> <img src="/images/delete.png" title="Delete" class="menuentry"> </a><!-- you forgot these --> </td><!-- you forgot these --> </tr> <?php endforeach ?> Furthermore introducing functions/classes would help you a lot: function getAllUsers(mysqli $dbconn, $count = null, $page = 1) { $sql = ' SELECT user_id, user_name, user_email FROM users '; if ($count !== null) { $sql .= sqlGenerateLimit($count, $page); } $result = mysqli_query($dbconn, $sql); return dbResultToArray($result); } function sqlGenerateLimit($count = null, $page = 1) { $limit = "\nLIMIT %d, %d"; $offset = ($page - 1) * $count; return sprintf($limit, $offset, $count); } function dbResultToArray($result, $mode = MYSQLI_ASSOC) { $rows = array(); if ($result instanceof mysqli_result) { if (mysqli_num_rows($result)) { while ($row = mysqli_fetch_array($result, $mode)) { $rows[] = $row; } } } return $rows; } Though you would need to add validation before using these. Edited November 18, 2012 by ignace Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/#findComment-1393352 Share on other sites More sharing options...
orgzchaos Posted November 18, 2012 Author Share Posted November 18, 2012 (edited) while ($list = mysql_fetch_assoc($result)) { $id = $list['id']; $username = $list['username']; $name = $list['name']; $email = $list['email']; $level = $list['level']; $table_content .= "<tr><td><input name='usercheckbox[]' type='checkbox' value='".$id."' title='Select/Deselect User'></td> <td>".$username."</td> <td>".$name."</td> <td>".$email."</td> <td>".$level."</td> <td><a href='view.php?id=".$id."'><img src='images/view.png' title='View details' class='menuentry'><a href='edit.php?id=".$id."'><img src='images/edit.png' title='Edit' class='menuentry'><a href='delete.php?id=".$id."'><img src='images/remove.png' title='Delete' class='menuentry'></tr>"; } You can reduce this further so that you have one part that retrieves the rows and puts them into an array and then afterwards in your html: <?php foreach ($rows as $row): ?> <tr> <td><?php echo $row['username'] ?></td> <td><?php echo $row['name'] ?></td> <td><?php echo $row['email'] ?></td> <td><?php echo $row['level'] ?></td> <td> <a href="/view.php?id=<?php echo $row['id'] ?>"> <img src="/images/view.png" title="View details" class="menuentry"> </a><!-- you forgot these --> <a href="/edit.php?id=<?php echo $row['id'] ?>"> <img src="/images/edit.png" title="Edit" class="menuentry"> </a><!-- you forgot these --> <a href="/delete.php?id=<?php echo $row['id'] ?>"> <img src="/images/delete.png" title="Delete" class="menuentry"> </a><!-- you forgot these --> </td><!-- you forgot these --> </tr> <?php endforeach ?> Furthermore introducing functions/classes would help you a lot: function getAllUsers(mysqli $dbconn, $count = null, $page = 1) { $sql = ' SELECT user_id, user_name, user_email FROM users '; if ($count !== null) { $sql .= sqlGenerateLimit($count, $page); } $result = mysqli_query($dbconn, $sql); return dbResultToArray($result); } function sqlGenerateLimit($count = null, $page = 1) { $limit = "\nLIMIT %d, %d"; $offset = ($page - 1) * $count; return sprintf($limit, $offset, $count); } function dbResultToArray($result, $mode = MYSQLI_ASSOC) { $rows = array(); if ($result instanceof mysqli_result) { if (mysqli_num_rows($result)) { while ($row = mysqli_fetch_array($result, $mode)) { $rows[] = $row; } } } return $rows; } Throwing this error: Warning: illegal string offset $row is a string, so I was wondering how $row['xxxxx'] would work? Thanks Edited November 18, 2012 by orgzchaos Quote Link to comment https://forums.phpfreaks.com/topic/270825-please-comment-on-this-coding-style/#findComment-1393354 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.