Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Just a quick preface, I'm not a web developer. I'm simply doing this as a favor for a friend. My goal is to be done with it as quickly as possible, but still not have the coding be horrendous. With that said, as I was coding this, my spidey senses were going off telling me that what I was doing was really bad programming practice and I was wondering if there are any quick fixes I can make to make it more efficient.

I have a form I need to run, but I wanted to give the user the option to choose the number of fields, so I've embedded a lot of PHP code within the body of my HTML files with if/else statements. The problem with this is that it's preventing my Javascript/CSS code in the header to run properly, so I've had to embed them within the body of the html. The code seems to be running a bit slow and probably not as neat as it should be. The code is kind of long, so I removed certain sections that were irrelevant to the discussion. I left a comment in their place to explain their function. I also need to add another block of PHP code that processes the form and enters them in the MySQl database. Any help is appreciated!

<html>
<head>
<title></title>
<script src="http://code.jquery.com/jquery-latest.js"></script>
<script src="jquery.maskedinput-1.3.js" type="text/javascript"></script>
<script type="text/javascript">
    $(document).ready(function() {
        $("#cash-amount").hide();
        $("#transfer-amount").hide();
        $(".date").mask("9999-99-99");
        $("#cash_yes").click(function(){
            $("#cash-amount").show();
        });
        $("#cash_no").click(function(){
            $("#cash-amount").hide();
        });
        $("#trnsfr_yes").click(function(){
            $("#transfer-amount").show();
        });
        $("#trnsfr_no").click(function(){
            $("#transfer-amount").hide();
        });
    });
</script>
</head>
<body>
<?php

if(isset($_POST['bill_submit'])){
?>
    <form id="expenses" method="post" action="<?php echo htmlentities($PHP_SELF); ?>">
<?php
    if($_POST['CC'] != 0)
    {
    ?>
        <table>
        <caption>Credit Cards</caption>
        <tr>
        <th></th>
        <th>CC # (last four digits)</th>
        <th>Amount</th>
        <th>Approval</th>
        <th>Date</th>
        </tr>
        <tbody>
    <?php
        for ($i = 1; $i <= $_POST['CC']; $i++)
        {
    ?>
        <tr>
        <td> <?php echo $i; ?> </td>
        <td> <input type="text" name="cc_num[]" maxlength="4" size="4" /> </td> 
        <td> <input type="text" name="cc_amnt[]" size="10"/> </td>
        <td> <input type="text" name="cc_app[]" maxlength="10" size="10" /> </td> 
        <td> <input class="date" type="text" name="cc_date[]" size="10" /> </td>
        </tr>
    <?php
        }
    ?>  
        </tbody>
        </table>
    <?php   
    }
    if($_POST['DB'] != 0)
    {
    ?>
        <table>
        <caption>Direct Bills</caption>
        <tr>
        <th></th>
        <th>DB #</th>
        <th>Amount</th>
        <th>Date</th>
        </tr>
        <tbody>
    <?php
        for ($i = 1; $i <= $_POST['DB']; $i++)
        {
    ?>
        <tr>
        <td> <?php echo $i; ?> </td>
        <td> <input type="text" name="db_num[]" size="10"/> </td> 
        <td> <input type="text" name="db_amnt[]" size="10"/> </td> 
        <td> <input class="date" type="text" name="db_date[]" size="10" />     </td>
        </tr>
    <?php
        }
    ?>  
        </tbody>
        </table>
    <?php   
    }
    if($_POST['misc'] != 0)
    {
    ?>
        <table>
        <caption>Miscellaneous</caption>
        <tr>
        <th></th>
        <th>Item</th>
        <th>Amount</th>
        <th>Date</th>
        </tr>
        <tbody>
    <?php
        for ($i = 1; $i <= $_POST['misc']; $i++)
        {
    ?>
        <tr>
        <td> <?php echo $i; ?> </td>
        <td> <input type="text" name="misc_item[]" size="10" /> </td> 
        <td> <input type="text" name="misc_amnt[]" size="10" /> </td> 
        <td> <input class="date" type="text" name="misc_date[]" size="10" /> </td>
        </tr>
    <?php
        }
    ?>  
        </tbody>
        </table>
    <?php   
    }
    ?>
    <div id="cash-check">
    Are you turning in any checks or cash today? &nbsp; <input type="radio" name="cash" value="Yes" id="cash_yes">Yes &nbsp; <input type="radio" name="cash" value="No" id="cash_no">No</br>
        <div id="cash-amount">
            Enter Amount: &nbsp; <input type="text" name="cash_amnt" id="cash_amnt" size="10" value="0.00">
        </div>
    </div>
    <div id="transfer">
    Would you like to transfer any money to another driver? &nbsp; <input type="radio" name="trnsfr" value="Yes" id="trnsfr_yes">Yes &nbsp; <input type="radio" name="trnsfr" value="No" id="trnsfr_no">No</br>
        <div id="transfer-amount">
            Enter Driver Number: &nbsp; <input type="text" name="transfer_amnt" id="transfer_amnt" size="10"> &nbsp;
            Enter Amount: &nbsp; <input type="text" name="transfer_amnt" id="transfer_amnt" size="10" value="0.00">
        </div>
    </div>

    <style type="text/css">
            #cash-amount
            {
                text-indent:50px;
            }
            #transfer-amount
            {
                text-indent:50px;
            }
    </style>
    <script type="text/javascript">
        $("#cash_yes").click(function(){
            $("#cash-amount").show();
        });
        $("#cash_no").click(function(){
            $("#cash-amount").hide();
        });
        $("#trnsfr_yes").click(function(){
            $("#transfer-amount").show();
        });
        $("#trnsfr_no").click(function(){
            $("#transfer-amount").hide();
        });
    </script>

    <input type="submit" value="Submit" name="submit" />
    </form>

    <?php
} 
else
{
?>

<div>
    Please indicate below how many credit cards, direct bills, and miscellaneous items you have to enter into the system. Note that you will have to start over if you enter the numbers incorrectly. 
</div>
</br>
<fieldset>
<legend>Numer of Items</legend>
    <form id="past_record" method="post" action="<?php echo htmlentities($PHP_SELF); ?>">
        Credit Cards: <select name="CC">
            <!--List of options from 0-20!-->
        </select>
        &nbsp; Direct Bills: <select name="DB">
            <!--List of options from 0-20!-->
        </select>
        &nbsp; Miscellaneous: <select name="misc">
            <!--List of options from 0-20!-->
        </select>
        &nbsp; <input type="submit" value="Submit" name="bill_submit" />
</form>
</fieldset>
</div>


<?php 
}
?>
</body>
</html>
share|improve this question
add comment

1 Answer 1

I cleaned up your code a bit:

<!doctype html>
<html>
<head>
    <meta charset="utf-8" />
    <title></title>
    <style type="text/css">
            #cash-amount
            {
                text-indent:50px;
            }
            #transfer-amount
            {
                text-indent:50px;
            }
    </style>
</head>
<body>
<?php

if(isset($_POST['bill_submit'])){
?>
    <form id="expenses" method="post" action="<?php echo htmlentities($PHP_SELF); ?>">
<?php
    if($_POST['CC'] != 0)
    {
    ?>
        <table>
        <caption>Credit Cards</caption>
        <tr>
        <th></th>
        <th>CC # (last four digits)</th>
        <th>Amount</th>
        <th>Approval</th>
        <th>Date</th>
        </tr>
        <tbody>
    <?php
        for ($i = 1; $i <= $_POST['CC']; $i++)
        {
    ?>
        <tr>
        <td> <?php echo $i; ?> </td>
        <td> <input type="text" name="cc_num[]" maxlength="4" size="4" /> </td> 
        <td> <input type="text" name="cc_amnt[]" size="10"/> </td>
        <td> <input type="text" name="cc_app[]" maxlength="10" size="10" /> </td> 
        <td> <input class="date" type="text" name="cc_date[]" size="10" /> </td>
        </tr>
    <?php
        }
    ?>  
        </tbody>
        </table>
    <?php   
    }
    if($_POST['DB'] != 0)
    {
    ?>
        <table>
        <caption>Direct Bills</caption>
        <tr>
        <th></th>
        <th>DB #</th>
        <th>Amount</th>
        <th>Date</th>
        </tr>
        <tbody>
    <?php
        for ($i = 1; $i <= $_POST['DB']; $i++)
        {
    ?>
        <tr>
        <td> <?php echo $i; ?> </td>
        <td> <input type="text" name="db_num[]" size="10"/> </td> 
        <td> <input type="text" name="db_amnt[]" size="10"/> </td> 
        <td> <input class="date" type="text" name="db_date[]" size="10" />     </td>
        </tr>
    <?php
        }
    ?>  
        </tbody>
        </table>
    <?php   
    }
    if($_POST['misc'] != 0)
    {
    ?>
        <table>
        <caption>Miscellaneous</caption>
        <tr>
        <th></th>
        <th>Item</th>
        <th>Amount</th>
        <th>Date</th>
        </tr>
        <tbody>
    <?php
        for ($i = 1; $i <= $_POST['misc']; $i++)
        {
    ?>
        <tr>
        <td> <?php echo $i; ?> </td>
        <td> <input type="text" name="misc_item[]" size="10" /> </td> 
        <td> <input type="text" name="misc_amnt[]" size="10" /> </td> 
        <td> <input class="date" type="text" name="misc_date[]" size="10" /> </td>
        </tr>
    <?php
        }
    ?>  
        </tbody>
        </table>
    <?php   
    }
    ?>
    <div id="cash-check">
    Are you turning in any checks or cash today? &nbsp; <input type="radio" name="cash" value="Yes" id="cash_yes">Yes &nbsp; <input type="radio" name="cash" value="No" id="cash_no">No</br>
        <div id="cash-amount">
            Enter Amount: &nbsp; <input type="text" name="cash_amnt" id="cash_amnt" size="10" value="0.00">
        </div>
    </div>
    <div id="transfer">
    Would you like to transfer any money to another driver? &nbsp; <input type="radio" name="trnsfr" value="Yes" id="trnsfr_yes">Yes &nbsp; <input type="radio" name="trnsfr" value="No" id="trnsfr_no">No</br>
        <div id="transfer-amount">
            Enter Driver Number: &nbsp; <input type="text" name="transfer_amnt" id="transfer_amnt" size="10"> &nbsp;
            Enter Amount: &nbsp; <input type="text" name="transfer_amnt" id="transfer_amnt" size="10" value="0.00">
        </div>
    </div>

    <input type="submit" value="Submit" name="submit" />
    </form>

    <?php
} 
else
{
?>

<div>
    Please indicate below how many credit cards, direct bills, and miscellaneous items you have to enter into the system. Note that you will have to start over if you enter the numbers incorrectly. 
</div>
</br>
<fieldset>
<legend>Numer of Items</legend>
    <form id="past_record" method="post" action="<?php echo htmlentities($PHP_SELF); ?>">
        Credit Cards: <select name="CC">
            <!--List of options from 0-20!-->
        </select>
        &nbsp; Direct Bills: <select name="DB">
            <!--List of options from 0-20!-->
        </select>
        &nbsp; Miscellaneous: <select name="misc">
            <!--List of options from 0-20!-->
        </select>
        &nbsp; <input type="submit" value="Submit" name="bill_submit" />
</form>
</fieldset>
</div>


<?php 
}
?>
<script src="http://code.jquery.com/jquery-latest.js"></script>
<script src="jquery.maskedinput-1.3.js"></script>
<script>
    $(function() {
        $("#cash-amount").hide();
        $("#transfer-amount").hide();
        $(".date").mask("9999-99-99");
        $("#cash_yes").on('click', function(e){
            $("#cash-amount").show();
        });
        $("#cash_no").on('click', function(e){
            $("#cash-amount").hide();
        });
        $("#trnsfr_yes").on('click', function(e){
            $("#transfer-amount").show();
        });
        $("#trnsfr_no").on('click', function(e){
            $("#transfer-amount").hide();
        });
    });
</script>
</body>
</html>

Script references should be moved to the bottom of the <body> tag, so that they don't block the HTML parser from doing it's job. Also, you were missing the doctype and character-set declarations, which can throw your browser into Quirks mode (which is not always fun to deal with). If you were getting paid to do this, I'd give you some grief about using $_POST references directly in your code, but for one-off stuff...

Also, avoid adding <style> elements anywhere except the <head>.

share|improve this answer
    
To be honest, the reason why I have all those annoying $_POST references directly in my code is because I'm running all my codes through wordpress (using embedded PHP within posts...I know, this is a bad idea). I originally chose to use wordpress because it had the benefit of nice looking themes and a user database setup, but I've come to regret that decision more and more. I was just hoping to prevent from having to learn how to make a secure user login system because I don't have the time to do so. I'm now really considering switching from Wordpress to a normal style website. –  user1562781 Jan 20 '13 at 19:49
    
@user1562781 Nice looking sites aren't too difficult if you start with something like the HTML5Boilerplate and/or Twitter Bootstrap. Also, if you're thinking of moving away from WP, the PEAR library might be worth looking into for ready-made components: pear.php.net –  Tieson T. Jan 21 '13 at 8:01
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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