0
\$\begingroup\$

I have MySQL data that I am rendering in an HTML table using PHP. This is the full code of my table and PHP. It does output fine and looks how I would expect but I am wondering if there is a better way to write this code. Also, the 'Delete' button specifically, is there a better way to output the button itself as right now, I noticed especially on a mobile device, I will see a text cursor indicator when tapping the button as if I am trying to select the text on the button itself.

<div class="container" style="width: 95%;">
    <table class="table table-bordered table-striped table-hover">
        <thead>
            <tr>
                <th class="col-sm-6">
                    Description
                </th>
                <th class="col-sm-2">
                    Date Created
                </th>
                <th class="col-sm-1">
                    Created By
                </th>
                <th class="col-sm-2" style="text-align: right;">
                    Edit - Delete
                </th>
            </tr>
        <?php

        $noLists = "<div>You have no lists. Why dont you create one?</div>";

        if ($db->connect_error) {
            die("Connection failed: " . $db->connect_error);
        }

        $sql = "SELECT DISTINCT lists.id, lists.listdescription, lists.createdon, lists.createdby, lists.sharedlist FROM lists INNER JOIN users ON users.fname = '$displayname' AND users.fname = lists.createdby";
        $result = $db->query($sql);

        if ($result->num_rows > 0) {
            // output data of each row
            while($row = $result->fetch_assoc()) {

                $listID = $row["id"];
                $listDescription = $row["listdescription"];
                $createdOn = $row["createdon"];
                $createdBy = $row["createdby"];

                echo "<form>";
                echo "<tr>";
                echo "<td>$listDescription</td>";
                echo "<td>$createdOn</td>";
                echo "<td>$createdBy</td>";
                echo "<td align=\"right\">";
                echo "<form action='account.php' method='post'>";
                echo "<input class=\"btn btn-success\" type=\"submit\" name=\"\" value=\"Edit\">";

                echo "<a href='includes/deletelist.php?id=$row[id]'><input class=\"btn btn-danger\" value=Delete style=\"width: 85px;\"></a>
                <!--<a href=\"index.php?id=$id\">Delete</a>-->
                </td>";


            }
        } else {
            echo "</thead>";
            echo "<tbody>";

            echo "</tbody>";
            echo "</table>";
            echo "<h4> $noLists</h4>";
        }
        $db->close();

        ?>
\$\endgroup\$
-1
\$\begingroup\$

Generally speaking, you should keep your markup and your PHP logic separate.

And the intermediate variables don't really have a purpose, just use the values from the array.

You've got two opening form tags and no closing form tags.

Your first condition (i there are records in the table) does not close the thead or the create a tbody.

Your delete button is no button at all, it's an input wrapped in an anchor tag (<a>). That's why it's not working as expected. Either remove the anchor and give the input a type attribute of "button", or remove the input and put some text in there instead.

Use a prepared statement. It's not cool to throw variables into your queries all willy-nilly. You will anger the code gods.

Here's a quick re-write (assumes you're using PDO - If you're using MySQLi, see the docs to convert the prepared statement to MySQLi)

I'm just going to go ahead and post this since i spend three or four minutes on it, but after looking at this code, there is no way this was working as you had it. This site is for reviewing working code, not fixing broken code. If your code doesn't work you need to ask on StackOverflow in the future.

<?php

    // Do all the PHP stuff before you start ouputting
    $noLists = "<div>You have no lists. Why dont you create one?</div>";

    if ($db->connect_error) {
        die("Connection failed: " . $db->connect_error);
    }

    $sql = "SELECT DISTINCT lists.id, lists.listdescription, lists.createdon, lists.createdby, lists.sharedlist FROM lists INNER JOIN users ON users.fname = ? AND users.fname = lists.createdby";
    $result = $db->prepare($sql);
    $result->execute(array($displayname));
    $output = array();
    if ($result->rowCount() > 0) {
        // output data of each row

        $output[] = "</thead>";
        $output[] = "<tbody>";

        while($row = $result->fetch(PDO::FETCH_ASSOC)) {

            $output[] = "<tr>";
            $output[] = "<td>{$row["listdescription"]}</td>";
            $output[] = "<td>{$row["createdon"]}</td>";
            $output[] = "<td>{$createdBy}</td>";
            $output[] = "<td align=\"right\">";
            $output[] = "<form action='account.php' method='post'>";
            $output[] = "<input class=\"btn btn-success\" type=\"submit\" name=\"\" value=\"Edit\">";
            $output[] = "</form>";
            $output[] = "<a href='includes/deletelist.php?id={$row["id"]}' class=\"btn btn-danger\" style=\"width: 85px;\">Delete</a></td>";
        }

        $output[] = "</tbody>";
        $output[] = "</table>";

    } else {
        $output[] = "</thead>";
        $output[] = "<tbody>";
        $output[] = "</tbody>";
        $output[] = "</table>";
        $output[] = "<h4> $noLists</h4>";
    }
    $db->close();

    // Turn the output array back into a string
    $output = implode('', $output);

?><div class="container" style="width: 95%;">
    <table class="table table-bordered table-striped table-hover">
        <thead>
            <tr>
                <th class="col-sm-6">
                    Description
                </th>
                <th class="col-sm-2">
                    Date Created
                </th>
                <th class="col-sm-1">
                    Created By
                </th>
                <th class="col-sm-2" style="text-align: right;">
                    Edit - Delete
                </th>
            </tr>
        <?php echo $output; ?>

Edit... you're not closing your table row in the loop either. I'm not gonna bother updating the code snippet.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thanks. The code works. It runs as I would expect. I'll look at what you have said \$\endgroup\$ – Marty Lavender Jul 21 '17 at 17:54
  • \$\begingroup\$ It might appear to work but it's certainly not generating valid markup \$\endgroup\$ – I wrestled a bear once. Jul 21 '17 at 17:55
  • \$\begingroup\$ Well it has been extremely helpful. I appreciate your time. \$\endgroup\$ – Marty Lavender Jul 21 '17 at 22:27
  • \$\begingroup\$ Happy to help :) don't forget about that </tr> in the first part of the if condition. \$\endgroup\$ – I wrestled a bear once. Jul 21 '17 at 22:30
  • \$\begingroup\$ Do you mind if I ping you from time to time? \$\endgroup\$ – Marty Lavender Jul 22 '17 at 0:06

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.