5
\$\begingroup\$

I am very new to this and am using php and MySQLi to create a form which will create a record in a third table. This works fine but I can't help but think that there is a way to do it with a single query rather than a query for each table as I have now. The third table is called tblcars, and I will use this form to build records for that table by selecting data from these dropdowns.

    <?php

    //Database Login
    require_once 'includes/connection.php';

    $sql = ("SELECT * FROM tblmake");

    $getclass = ("SELECT * FROM tblclass");

    $result = $conn->query($sql);
    $results = $conn->query($getclass);

    ?>

    <html>
    <head>
        <title>Car Makes</title>
    </head>
    <body>

        <h3>Car Makes</h3>      
            <form action="" method="get">
            <select name="makeID">
                <option value="">Choose Make</option>
                <?php 
                    while($make = $result->fetch_assoc()) {
                        echo "<option value={$make['makeID']}>";
                        echo $make['carmake'];
                        echo "</option>";
                    }
                ?>
            </select>
            <select name="classID">
                <option value="">Choose Class</option>
                <?php 
                    while($class = $results->fetch_assoc()) {
                        echo "<option value={$class['classID']}>";
                        echo $class['carclass'];
                        echo "</option>";
                    }
                ?>
            </select>
            <input type="submit" value="Insert">
        </form>

</body>
</html>
\$\endgroup\$

2 Answers 2

Reset to default
3
\$\begingroup\$

As @tim already pointed out, I don't think there is a way of combining these two SQL queries together since they aren't relevant to each other. Also, your variable, table and table column naming is a bit odd.

What I've done to your code:

  • Renamed the variables.
  • Added the HTML5 doctype.
  • Omitted the form's action attribute.
  • Disabled the first select options.
  • Created a short alias function to handle html output to prevent XSS attacks.
  • Used alternative while() syntax more suited for going in and out of PHP to output HTML.
  • Used <?= $x (shorthand for <?php echo $x;).

Ended up with the following code:

<?php
require_once 'includes/connection.php';

$carMakeSql    = 'SELECT * FROM tblmake';
$carMakeResult = $conn->query($carMakeSql);

$carClassSql    = 'SELECT * FROM tblclass';
$carClassResult = $conn->query($carClassSql);

function html($string) {
    return htmlspecialchars($string, ENT_QUOTES, 'UTF-8');
}
?>
<!DOCTYPE html>
<html>
    <head>
        <title>Car Makes</title>
    </head>
    <body>
        <h3>Car Makes</h3>

        <form method="GET">
            <select name="makeID">
                <option selected disabled>Choose Make</option>
            <?php while ($result = $carMakeResult->fetch_assoc()): ?>
                <option value="<?= html($result['makeID']); ?>"><?= html($result['carmake']); ?></option>
            <?php endwhile; ?>
            </select>

            <select name="classID">
                <option selected disabled>Choose Class</option>
            <?php while ($result = $carClassResult->fetch_assoc()): ?>
                <option value="<?= html($result['classID']); ?>"><?= html($result['carclass']); ?></option>
            <?php endwhile; ?>
            </select>

            <input type="submit" value="Insert">
        </form>
    </body>
</html>

(not tested)

\$\endgroup\$
3
\$\begingroup\$

I don't see how you could do this in a single queries, because both queries have nothing to do with each other.

Your code does contain a bit of duplication, but otherwise it looks good to me.

You can simplify your code by introducing a function:

<?php
    createTableSelectForm($conn, $table, $columnValue, $columnShow, $title) {
        $sql = ("SELECT * FROM " . $table);
        $results = $conn->query($sql);
        $form = '
                <select name="makeID">
                    <option value="">Choose ' . htmlspecialchars($title, ENT_QUOTES, 'UTF-8') . '</option>';
        while($result = $results->fetch_assoc()) {
            $form .= '<option value="' . htmlspecialchars($result[$columnValue], ENT_QUOTES, 'UTF-8') . '">';
            $form .=  htmlspecialchars($result[$columnShow], ENT_QUOTES, 'UTF-8');
            $form .= '</option>';
        }
        $form .= '</select>';
        return $form;
    }

    require_once 'includes/connection.php';
?>
    <html>
    <head>
        <title>Car Makes</title>
    </head>
    <body>

        <h3>Car Makes</h3>      
            <form action="" method="get">
            <?php echo createTableSelectForm($conn, "tblmake", "makeID", "carmake", "Make");
            echo createTableSelectForm($conn, "tblclass", "classID", "carclass", "Class"); ?>
            <input type="submit" value="Insert">
        </form>

    </body>
    </html>

You really only need the first two arguments for the createTableSelectForm, but I added the other ones so that the method is more reusable.

I also added XSS protection, because with variable data, you can never be sure where it comes from (maybe, in the future you will allow visitors to add makes? Or now that the function is reusable, it will be reused somewhere else with user input? Who knows, and better safe than sorry; it also doesn't add that much overhead).

Misc

  • your naming is a bit confusing. The same thing should always have the same name, because it increases readability . But you have $sql and $getclass for basically the same thing. The same goes for $result vs $results (both hold multiple results). $sqlSelectMake/$sqlSelectClass and $resultMakes/$resultClasses would be better.
\$\endgroup\$
1
  • \$\begingroup\$ Thank you for the suggestions. I don't see any reason why I would ever allow other users to use this but you're right, better safe than sorry. \$\endgroup\$
    – Bob J
    Commented May 15, 2015 at 18:35

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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