2
\$\begingroup\$

I'd like this improved.

<!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="utf-8">
        <title>Add New Games</title>
        <meta name="viewport" content="width=device-width, initial-scale=1.0">
        <meta name="description" content="">
        <meta name="author" content="">

        <!-- Le styles -->
        <link href="css/bootstrap.css" rel="stylesheet">
        <style>
            body {
                padding-top: 60px; /* 60px to make the container go all the way to the bottom of the topbar */
            }
        </style>
        <link href="css/bootstrap-responsive.css" rel="stylesheet">

        <!-- HTML5 shim, for IE6-8 support of HTML5 elements -->
        <!--[if lt IE 9]>
            <script src="js/html5shiv.js"></script>
        <![endif]-->

        <!-- Fav and touch icons -->
        <link rel="apple-touch-icon-precomposed" sizes="144x144" href="ico/apple-touch-icon-144-precomposed.png">
        <link rel="apple-touch-icon-precomposed" sizes="114x114" href="ico/apple-touch-icon-114-precomposed.png">
        <link rel="apple-touch-icon-precomposed" sizes="72x72" href="ico/apple-touch-icon-72-precomposed.png">
        <link rel="apple-touch-icon-precomposed" href="ico/apple-touch-icon-57-precomposed.png">
        <link rel="shortcut icon" href="ico/favicon.png">
    </head>
    <body>
        <div class="navbar navbar-inverse navbar-fixed-top">
            <div class="navbar-inner">
                <div class="container">
                    <button type="button" class="btn btn-navbar" data-toggle="collapse" data-target=".nav-collapse">
                        <span class="icon-bar"></span>
                        <span class="icon-bar"></span>
                        <span class="icon-bar"></span>
                    </button>
                    <a class="brand" href="#">Game Admin Area</a>
                    <div class="nav-collapse collapse">
                        <ul class="nav">
                            <li class="active"><a href="#">Add New Games</a></li>
                            <li><a href="#about">Update Game Scores</a></li>
                            <li><a href="#contact">Reports</a></li>
                        </ul>
                    </div><!--/.nav-collapse -->
                </div>
            </div>
        </div>
        <div class="container">
            <h1>Add New Games to Database</h1>
            <p>Use the form below to quickly add upcoming games to the database.<br>Players will not be able to place their predictions until you inputted these games.</p>
            <?php
            $dbc = mysql_connect(,,);
            $db = mysql_select_db();
            $results= mysql_query("SELECT * FROM tbl_teams");

            $myHTML = '';
            while($row = mysql_fetch_array($results)) {
                $myHTML .= '<option value="'.$row['team_ID'].'">'. $row['team_name'].'</option>';
            }
            $myHTMLdate = '';
            for($i = 1; $i <= 14; $i ++){
              $startdate = strtotime("today + $i day");
                  $myHTMLdate .= '<option value="'.date('Y-m-d', $startdate).'">'.date('l', $startdate).', '.date('d M Y', $startdate).'</option>';
            }

            ?>
            <div id="wrapper">
                <div class="datagrid">
                    <table class="table table-striped table-bordered table-condensed">
                        <thead>
                            <tr>
                                <th>Game No.</th>
                                <th>Team 1</th>
                                <th>Vs.</th>
                                <th>Team 2</th>
                                <th>Game Date</th>
                            </tr>
                        </thead>
                        <tbody>
                            <?php
                                for ($i=1; $i <=30; $i++) {
                                    echo "<tr>
                                        <td>".$i."</td>
                                        <td><select name='game".$i."_team1'>".$myHTML."</select></td>
                                        <td>Vs.</td>
                                        <td><select name='game".$i."_team2'>".$myHTML."</select></td>
                                        <td><select name='game".$i."_date'>".$myHTMLdate."</select></td>
                                    </tr>";
                                }
                             ?>
                        </tbody>
                    </table>
                    <button class="btn btn-large btn-primary" type="submit">Send these games to the server!</button>
                </div>
            </form>
        </div>
<!-- /container -->
        <!-- Javascript -->
        <!-- Placed at the end of the document so the pages load faster -->
        <script src="js/jquery.js"></script>
        <script src="js/bootstrap-transition.js"></script>
        <script src="js/bootstrap-alert.js"></script>
        <script src="js/bootstrap-modal.js"></script>
        <script src="js/bootstrap-dropdown.js"></script>
        <script src="js/bootstrap-scrollspy.js"></script>
        <script src="js/bootstrap-tab.js"></script>
        <script src="js/bootstrap-tooltip.js"></script>
        <script src="js/bootstrap-popover.js"></script>
        <script src="js/bootstrap-button.js"></script>
        <script src="js/bootstrap-collapse.js"></script>
        <script src="js/bootstrap-carousel.js"></script>
        <script src="js/bootstrap-typeahead.js"></script>

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

First of all, I'm not a fan of having one single page with everything -- I typically prefer to separate my HTML from my PHP.

Putting that aside:

$dbc = mysql_connect(,,);

The documentation for mysql_connect says:

This extension is deprecated as of PHP 5.5.0, and will be removed in the future. Instead, the MySQLi or PDO_MySQL extension should be used. See also MySQL: choosing an API guide and related FAQ for more information.

Generally speaking, when starting a new project, it's a bad idea to use deprecated APIs.

Besides, I assume you just removed the parameters from the function and you have mysql_connect("myserver", "myusername", "mypassword"). I wouldn't recommend having passwords in the same file as the rest of the code. Use a configuration file instead.

 $myHTML .= '<option value="'.$row['team_ID'].'">'. $row['team_name'].'</option>';

Are you familiar with XSS injections? Is it possible someone could create a new team with a name like <script>/*do bad stuff here*/</script>?

You close a form(</form>) but I don't see you opening it. This is not valid HTML. Did you forget to add it?

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

Here is some more food for thought.

Try not to create spaghetti code (database calls intermingled with html).

It becomes very hard to maintain. I have moved your php database code to the top of the file, that way you can catch errors, and handle them before you display any html.

Splitting you file into parts

Put your database configuration into a separate file and include it. (others have mentioned this too)

If you do repeat your navbar and header and footer on other pages, then a good idea to split files and include them, so you are not repeating code/markup again. Other have mentioned this too.

Mysql/Database

Others have mentioned that mysql is deprecated, so I am not going to revisit that.

Use mysql_fetch_assoc not mysql_fetch_array, you can read the manual for the differences, but in your particular case mysql_fetch_array is not necessary.

Check for mysql_error() after database calls. In the code below I have thrown exceptions, that is not necessary, but does make it easy to handle all errors in one place.

It is also quite common to catch mysql errors like this

$results= mysql_query("SELECT * FROM tbl_teams") or die (mysql_error());

html/css/js

Wrap any html you render in htmlentities() in case the data contains characters like this < > as these will break your html

Bootstrap includes a bootstrap-min.js which has all the plugins you have in 1 file, this will speed up load times.

Also jquery-min.js will speed up load times

Bootstrap 2 is not deprecated, but bootstrap 3 has been released, if you want to go with the latest and greatest.

<?php

    try {
        $dbc = mysql_connect($host, $user, $pass);

        if (mysql_error()) {
            throw new Exception("Database Connection Failed: " . mysql_error());
        }

        $db = mysql_select_db($dbname);

        if (mysql_error()) {
            throw new Exception("Database Select DB Failed: " . mysql_error());
        }

        $results= mysql_query("SELECT * FROM tbl_teams");

        if (mysql_error()) {
            throw new Exception("Database Query Failed: " . mysql_error());
        }

        $myHTML = '';
        // use mysql_fetch_assoc not mysql_fetch_array
        while($row = mysql_fetch_assoc($results)) {

            // wrap in html entities in case team names include < >, etc as these will break your html
            $myHTML .= '<option value="'.$row['team_ID'].'">'. htmlentities($row['team_name']).'</option>';
        }

        $myHTMLdate = '';
        for($i = 1; $i <= 14; $i ++){
            $startdate = strtotime("today + $i day");
            $myHTMLdate .= '<option value="'.date('Y-m-d', $startdate).'">'.date('l', $startdate).', '.date('d M Y', $startdate).'</option>';
        }

    } catch (Exception $ex) {

        // can redirect to an error page, etc
        die ("Application Error: ".$ex->getMessage());
    }

    // only presentation logic after here

?>

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="utf-8">
    <title>Add New Games</title>
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <meta name="description" content="">
    <meta name="author" content="">

    <!-- Le styles -->
    <link href="css/bootstrap.css" rel="stylesheet">
    <style>
        body {
            padding-top: 60px; /* 60px to make the container go all the way to the bottom of the topbar */
        }
    </style>
    <link href="css/bootstrap-responsive.css" rel="stylesheet">

    <!-- HTML5 shim, for IE6-8 support of HTML5 elements -->
    <!--[if lt IE 9]>
    <script src="js/html5shiv.js"></script>
    <![endif]-->

    <!-- Fav and touch icons -->
    <link rel="apple-touch-icon-precomposed" sizes="144x144" href="ico/apple-touch-icon-144-precomposed.png">
    <link rel="apple-touch-icon-precomposed" sizes="114x114" href="ico/apple-touch-icon-114-precomposed.png">
    <link rel="apple-touch-icon-precomposed" sizes="72x72" href="ico/apple-touch-icon-72-precomposed.png">
    <link rel="apple-touch-icon-precomposed" href="ico/apple-touch-icon-57-precomposed.png">
    <link rel="shortcut icon" href="ico/favicon.png">
</head>
<body>
<div class="navbar navbar-inverse navbar-fixed-top">
    <div class="navbar-inner">
        <div class="container">
            <button type="button" class="btn btn-navbar" data-toggle="collapse" data-target=".nav-collapse">
                <span class="icon-bar"></span>
                <span class="icon-bar"></span>
                <span class="icon-bar"></span>
            </button>
            <a class="brand" href="#">Game Admin Area</a>
            <div class="nav-collapse collapse">
                <ul class="nav">
                    <li class="active"><a href="#">Add New Games</a></li>
                    <li><a href="#about">Update Game Scores</a></li>
                    <li><a href="#contact">Reports</a></li>
                </ul>
            </div><!--/.nav-collapse -->
        </div>
    </div>
</div>
<div class="container">
    <h1>Add New Games to Database</h1>
    <p>Use the form below to quickly add upcoming games to the database.<br>Players will not be able to place their predictions until you inputted these games.</p>
    <div id="wrapper">
        <div class="datagrid">
            <table class="table table-striped table-bordered table-condensed">
                <thead>
                <tr>
                    <th>Game No.</th>
                    <th>Team 1</th>
                    <th>Vs.</th>
                    <th>Team 2</th>
                    <th>Game Date</th>
                </tr>
                </thead>
                <tbody>
                <?php
                for ($i=1; $i <=30; $i++) {
                    echo "<tr>
                                        <td>".$i."</td>
                                        <td><select name='game".$i."_team1'>".$myHTML."</select></td>
                                        <td>Vs.</td>
                                        <td><select name='game".$i."_team2'>".$myHTML."</select></td>
                                        <td><select name='game".$i."_date'>".$myHTMLdate."</select></td>
                                    </tr>";
                }
                ?>
                </tbody>
            </table>
            <button class="btn btn-large btn-primary" type="submit">Send these games to the server!</button>
        </div>
        </form>
    </div>
    <!-- /container -->
    <!-- Javascript -->
    <!-- Placed at the end of the document so the pages load faster -->
    <script src="js/jquery.js"></script>

    <!-- double check if this is the correct filename, but there is a minified version of the complete bootstrap js library -->
    <script src="js/bootstrap-min.js"></script>

</body>
</html>
\$\endgroup\$
2
\$\begingroup\$
  • This isn't really something you did wrong, but it will make managing the site easier if you break it into chunks and include those. For example, if you broke the header, navbar, and footer out into PHP files you can include them in multiple pages and have one spot to edit them from. For example:

header.php

<!DOCTYPE html>
<html lang="en">
    <head>
        <meta charset="utf-8">
        <title>Add New Games</title>
        <meta name="viewport" content="width=device-width, initial-scale=1.0">
        <meta name="description" content="">
        <meta name="author" content="">

        <!-- Le styles -->
        <link href="css/bootstrap.css" rel="stylesheet">
        <style>
            body {
                padding-top: 60px; /* 60px to make the container go all the way to the bottom of the topbar */
            }
        </style>
        <link href="css/bootstrap-responsive.css" rel="stylesheet">

        <!-- HTML5 shim, for IE6-8 support of HTML5 elements -->
        <!--[if lt IE 9]>
            <script src="js/html5shiv.js"></script>
        <![endif]-->

        <!-- Fav and touch icons -->
        <link rel="apple-touch-icon-precomposed" sizes="144x144" href="ico/apple-touch-icon-144-precomposed.png">
        <link rel="apple-touch-icon-precomposed" sizes="114x114" href="ico/apple-touch-icon-114-precomposed.png">
        <link rel="apple-touch-icon-precomposed" sizes="72x72" href="ico/apple-touch-icon-72-precomposed.png">
        <link rel="apple-touch-icon-precomposed" href="ico/apple-touch-icon-57-precomposed.png">
        <link rel="shortcut icon" href="ico/favicon.png">
    </head>
    <body>

navbar.php

 <div class="navbar navbar-inverse navbar-fixed-top">
        <div class="navbar-inner">
            <div class="container">
                <button type="button" class="btn btn-navbar" data-toggle="collapse" data-target=".nav-collapse">
                    <span class="icon-bar"></span>
                    <span class="icon-bar"></span>
                    <span class="icon-bar"></span>
                </button>
                <a class="brand" href="#">Game Admin Area</a>
                <div class="nav-collapse collapse">
                    <ul class="nav">
                        <li class="active"><a href="#">Add New Games</a></li>
                        <li><a href="#about">Update Game Scores</a></li>
                        <li><a href="#contact">Reports</a></li>
                    </ul>
                </div><!--/.nav-collapse -->
            </div>
        </div>
    </div>

footer.php

        <!-- Javascript -->
        <!-- Placed at the end of the document so the pages load faster -->
        <script src="js/jquery.js"></script>
        <script src="js/bootstrap-transition.js"></script>
        <script src="js/bootstrap-alert.js"></script>
        <script src="js/bootstrap-modal.js"></script>
        <script src="js/bootstrap-dropdown.js"></script>
        <script src="js/bootstrap-scrollspy.js"></script>
        <script src="js/bootstrap-tab.js"></script>
        <script src="js/bootstrap-tooltip.js"></script>
        <script src="js/bootstrap-popover.js"></script>
        <script src="js/bootstrap-button.js"></script>
        <script src="js/bootstrap-collapse.js"></script>
        <script src="js/bootstrap-carousel.js"></script>
        <script src="js/bootstrap-typeahead.js"></script>

    </body>
</html>

index.php

<?php include_once('header.php'); ?>
<?php include_once('navbar.php'); ?>
<div class="container">
            <h1>Add New Games to Database</h1>
            <p>Use the form below to quickly add upcoming games to the database.<br>Players will not be able to place their predictions until you inputted these games.</p>
            <?php
            $dbc = mysql_connect(,,);
            $db = mysql_select_db();
            $results= mysql_query("SELECT * FROM tbl_teams");

            $myHTML = '';
            while($row = mysql_fetch_array($results)) {
                $myHTML .= '<option value="'.$row['team_ID'].'">'. $row['team_name'].'</option>';
            }
            $myHTMLdate = '';
            for($i = 1; $i <= 14; $i ++){
              $startdate = strtotime("today + $i day");
                  $myHTMLdate .= '<option value="'.date('Y-m-d', $startdate).'">'.date('l', $startdate).', '.date('d M Y', $startdate).'</option>';
            }

            ?>
            <div id="wrapper">
                <div class="datagrid">
                    <table class="table table-striped table-bordered table-condensed">
                        <thead>
                            <tr>
                                <th>Game No.</th>
                                <th>Team 1</th>
                                <th>Vs.</th>
                                <th>Team 2</th>
                                <th>Game Date</th>
                            </tr>
                        </thead>
                        <tbody>
                            <?php
                                for ($i=1; $i <=30; $i++) {
                                    echo "<tr>
                                        <td>".$i."</td>
                                        <td><select name='game".$i."_team1'>".$myHTML."</select></td>
                                        <td>Vs.</td>
                                        <td><select name='game".$i."_team2'>".$myHTML."</select></td>
                                        <td><select name='game".$i."_date'>".$myHTMLdate."</select></td>
                                    </tr>";
                                }
                             ?>
                        </tbody>
                    </table>
                    <button class="btn btn-large btn-primary" type="submit">Send these games to the server!</button>
                </div>
            </form>
        </div>
<!-- /container -->

<?php include_once('footer.php'); ?>
  • As luiscubal mentioned, use a different method of connecting to the database. mysql_connect() is deprecated. PDO and mysqli are both good options, my personal preference is PDO though because it has the ability to swap out the database type (MySQL to SQLite) and still use the same method calls.

  • Your query does a "SELECT *". You should consider listing out the columns so that you only pull what you need. This will keep things snappier because you won't be pulling information you don't need to render on this page. This is a pretty good answer with some more reasons to not use "SELECT *", most of which are not relevant to this simple of a page.

\$\endgroup\$

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.