Can someone critique book author's code pls

acialk · April 7, 2009

Hey guys,

I've got this book, Learning PHP 5 by David Sklar. I've also got access to it via Safari Books Online(trial version). I'm currently stuck(again) on Chapter 6, Making Web Forms. I was looking forward to this chapter as I've got a form validation test for a PHP course I'm registered on coming up.

Anyhow, I've encountered some problems with the author's code and I would like some of the resident experts on here to critique it for me to confirm whether or not I should incinerate this book for the good of man kind.

First of all. On my web server that's online, all looks well and it appears fine. Except the validation fails on one section of the form making the entire form useless.

Offline running on a WAMP installation I receive this:

Here is the source code:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN"?
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<title></title>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1" />
</head>
<body>
<?php
//print a text box
function input_text($element_name, $values) {
    print '<input type="text" name="' . $element_name .'" value="';
    print htmlentities($values[$element_name]) . '"/>';
}

//print a submit button
function input_submit($element_name, $label) {
    print '<input type="submit" name="' . $element_name .'" value="';
    print htmlentities($label) .'"/>';
}

//print a textarea
function input_textarea($element_name, $values) {
    print '<textarea name="' . $element_name .'">';
    print htmlentities($values[$element_name]) . '</textarea>';
}

//print a radio button or checkbox
function input_radiocheck($type, $element_name, $values, $element_value) {
    print '<input type="' . $type . '" name="' . $element_name .'" value="' . $element_value . '" ';
    if ($element_value == $values[$element_name]) {
        print ' checked="checked"';
    }
    print '/>';
}

//print a <select> menu
function input_select($element_name, $selected, $options, $multiple = false) {
    // print out the <select> tag
    print '<select name="' . $element_name;
    // if multiple choices are permitted, add the multiple attribute
    // and add a [  ] to the end of the tag name
    if ($multiple) { print '[  ]" multiple="multiple'; }
    print '">';

    // set up the list of things to be selected
    $selected_options = array( );
    if ($multiple) {
        foreach ($selected[$element_name] as $val) {
            $selected_options[$val] = true;
        }
    } else {
        $selected_options[ $selected[$element_name] ] = true;
    }

    // print out the <option> tags
    foreach ($options as $option => $label) {
        print '<option value="' . htmlentities($option) . '"';
        if (isset($selected_options[$option])) {

            print ' selected="selected"';
        }
        print '>' . htmlentities($label) . '</option>';
    }
    print '</select>';
}
?>
<?php
// don't forget to include the code for the form
// helper functions defined in Example 6-29
//
// setup the arrays of choices in the select menus
// these are needed in display_form( ), validate_form( ),
// and process_form( ), so they are declared in the global scope
$sweets = array('puff' => 'Sesame Seed Puff',
                'square' => 'Coconut Milk Gelatin Square',
                'cake' => 'Brown Sugar Cake',
                'ricemeat' => 'Sweet Rice and Meat');

$main_dishes = array('cuke' => 'Braised Sea Cucumber',
                     'stomach' => "Sauteed Pig's Stomach",
                     'tripe' => 'Sauteed Tripe with Wine Sauce',
                     'taro' => 'Stewed Pork with Taro',
                     'giblets' => 'Baked Giblets with Salt', 
                     'abalone' => 'Abalone with Marrow and Duck Feet');

// The main page logic:
// - If the form is submitted, validate and then process or redisplay
// - If it's not submitted, display
if ($_POST['_submit_check']) {
    // If validate_form( ) returns errors, pass them to show_form( )
    if ($form_errors = validate_form( )) {
        show_form($form_errors);
    } else {
        // The submitted data is valid, so process it
        process_form( );
    }
} else {
    // The form wasn't submitted, so display
    show_form( );
}

function show_form($errors = '') {
    // If the form is submitted, get defaults from submitted parameters
    if ($_POST['_submit_check']) {
        $defaults = $_POST;
    } else {
        // Otherwise, set our own defaults: medium size and yes to delivery
        $defaults = array('delivery' => 'yes',
                          'size'     => 'medium');
    }
    
    // If errors were passed in, put them in $error_text (with HTML markup)
    if ($errors) {
        $error_text = '<tr><td>You need to correct the following errors:';
        $error_text .= '</td><td><ul><li>';
        $error_text .= implode('</li><li>',$errors);
        $error_text .= '</li></ul></td></tr>';
    } else {
        // No errors? Then $error_text is blank
        $error_text = '';
    }

    // Jump out of PHP mode to make displaying all the HTML tags easier
?>
<form method="POST" action="<?php print $_SERVER['PHP_SELF']; ?>">
<table>
<?php print $error_text ?>

<tr><td>Your Name:</td>
<td><?php input_text('name', $defaults) ?></td></tr>

<tr><td>Size:</td>
<td><?php input_radiocheck('radio','size', $defaults, 'small');  ?> Small <br/>
<?php input_radiocheck('radio','size', $defaults, 'medium'); ?> Medium <br/>
<?php input_radiocheck('radio','size', $defaults, 'large');  ?> Large
</td></tr>

<tr><td>Pick one sweet item:</td>
<td><?php input_select('sweet', $defaults, $GLOBALS['sweets']); ?>
</td></tr>

<tr><td>Pick two main dishes:</td>
<td>
<?php input_select('main_dish', $defaults, $GLOBALS['main_dishes'], true) ?>
</td></tr>

<tr><td>Do you want your order delivered?</td>
<td><?php input_radiocheck('checkbox','delivery', $defaults, 'yes'); ?> Yes
</td></tr>

<tr><td>Enter any special instructions.<br/>
If you want your order delivered, put your address here:</td>
<td><?php input_textarea('comments', $defaults); ?></td></tr>

<tr><td colspan="2" align="center"><?php input_submit('save','Order'); ?>
</td></tr>

</table>
<input type="hidden" name="_submit_check" value="1"/>
</form>
<?php
      } // The end of show_form( )

function validate_form( ) {
    $errors = array( );

    // name is required
    if (! strlen(trim($_POST['name']))) {
        $errors[  ] = 'Please enter your name.';
    }
    // size is required
    if (($_POST['size'] != 'small') && ($_POST['size'] != 'medium') &&
        ($_POST['size'] != 'large')) {
        $errors[  ] = 'Please select a size.';
    }
    // sweet is required
    if (! array_key_exists($_POST['sweet'], $GLOBALS['sweets'])) {
        $errors[  ] = 'Please select a valid sweet item.';
    }
    // exactly two main dishes required
    if (count($_POST['main_dish']) != 2) {
        $errors[  ] = 'Please select exactly two main dishes.';
    } else {
        // We know there are two main dishes selected, so make sure they are 
        // both valid
        if (! (array_key_exists($_POST['main_dish'][0], $GLOBALS['main_dishes']) &&
               array_key_exists($_POST['main_dish'][1], $GLOBALS['main_dishes']))) {
            $errors[  ] = 'Please select exactly two valid main dishes.';
        }
    }
    // if delivery is checked, then comments must contain something
    if (($_POST['delivery'] == 'yes') && (! strlen(trim($_POST['comments'])))) {
        $errors[  ] = 'Please enter your address for delivery.';
    }

    return $errors;
}

function process_form( ) {
    // look up the full names of the sweet and the main dishes in
    // the $GLOBALS['sweets'] and $GLOBALS['main_dishes'] arrays
    $sweet = $GLOBALS['sweets'][ $_POST['sweet'] ];
    $main_dish_1 = $GLOBALS['main_dishes'][ $_POST['main_dish'][0] ];
    $main_dish_2 = $GLOBALS['main_dishes'][ $_POST['main_dish'][1] ];
    if ($_POST['delivery'] == 'yes') {
        $delivery = 'do';
    } else {
        $delivery = 'do not';
    }
    // build up the text of the order message
    $message=<<<_ORDER_
Thank you for your order, $_POST[name].
You requested the $_POST[size] size of $sweet, $main_dish_1, and $main_dish_2.
You $delivery want delivery.
_ORDER_;
    if (strlen(trim($_POST['comments']))) {
        $message .= 'Your comments: '.$_POST['comments'];
    }

    // send the message to the chef
    mail('chef@restaurant.example.com', 'New Order', $message);
    // print the message, but encode any HTML entities
    // and turn newlines into <br/> tags
    print nl2br(htmlentities($message));
}
?>
</body>
</html>

It's not only this book that I've encountered errors with the author's code. I've had the same problem with PHP Cookbook also by O'Reilly & 2 Apress books.

Perhaps it's my sheer stupidity and I'm doing something wrong? Or is error logging overly cautious? Even if it is, in some instances the author's code just completely fails anyhow.

I find it an odd coincidence that I've only had issues with PHP books. As I've got books on HTML, CSS and Javascript and I've never encountered as many errors as I've had with PHP books. Or perhaps I've just been unfortunate and have picked some really bad books. I got these books based on Amazon reviews and the publishers reputation.

The reason I bought a book was to avoid pestering people on forums like these, haha.

David's book was a really good introduction to PHP but with having no experience with things such as form validation I really wanted to see how an expert tackled such tasks so that I could try and understand how and why it was done that way and emulate it myself. That goes down the crapper when the code examples fail to work :-\

Lodius2000 · April 7, 2009

can you mark lines 89 and 104, I used this book too and didnt have too much problem with it... didnt type out all the examples though, mostly just looked at them so that I can see how they work, looking at this one, it looks like it should work, now for that validation error that is "making the whole form useless"... i bet its in validate form

// size is required
   if (($_POST['size'] != 'small') && ($_POST['size'] != 'medium') &&
       ($_POST['size'] != 'large')) {
       $errors[  ] = 'Please select a size.';
   }

it looks like you need the logical 'or' operator here... what you have, to me, looks like the radio buttons would only validate if all 3 of them are selected, try replacing both sets of && with || then see if it validates

//I LOVE this book, it taught me a million and one good things. if there is an error in one of the included scripts, its no big deal, they all seem to have errors, this book is great, stay with it, and continue with its models, and do the exercises at the ends of the chapters

Edit: bravo for being so new and creating a near technicaally perfect post, so many newbs here wouldnt use the code tags and the inline images help too

welcome

PFMaBiSmAd · April 7, 2009

Change both lines 89 and 104 to -

if (isset($_POST['_submit_check'])) {

The code was probably developed on a system with error reporting set to hide notice errors, thinking that notice errors are ok, which they are not. Each line that generates a notice error takes about 10 times longer to execute because php must still go through the error response logic to figure out what is wrong even if the error reporting or display errors settings hide the error message.

Lodius2000 · April 7, 2009

pfm, error_reporting(E_ALL); reports notices right?, I use this same method in my scripts and always have E_ALL on to me if($var) checks if the variable even exists, where if(isset($var)) checks to see if it both exists and has a value that is not null, in the case above, we know that _submit_check will have a value if it exists, so we don't have to check that it does, we just need to see if its there so what he has is correct, UNLESS it generates a notice... then I have alot of code to rework

gizmola · April 7, 2009

I have to disagree with Lodius2000, the line of code is correct. It will only add to errors if the size has no acceptable value. Using OR will guarantee that it always errors out.

The reason you get the warning, is that the code attampts to use the value of $_POST['_submit_check'] when it doesn't exist. You could mitigate this in a couple of ways, one being adding the @$_POST['_submit_check'], or adding an isset($_POST['_submit_check']) &&

I'm not really sure why the form doesn't work -- I didn't see anything obviously wrong with the code. What is the exact validation error you get, and what was the input?

gizmola · April 7, 2009

Also, for what it's worth, using Global variables is not the best idea in my opinion.

acialk · April 7, 2009

I have to disagree with Lodius2000, the line of code is correct. It will only add to errors if the size has no acceptable value. Using OR will guarantee that it always errors out.

The reason you get the warning, is that the code attampts to use the value of $_POST['_submit_check'] when it doesn't exist. You could mitigate this in a couple of ways, one being adding the @$_POST['_submit_check'], or adding an isset($_POST['_submit_check']) &&

I'm not really sure why the form doesn't work -- I didn't see anything obviously wrong with the code. What is the exact validation error you get, and what was the input?

It keeps requesting you to select two choices even though you have.

Also, on the initial load the value boxes from these two functions print out undefined index. I want to check $values[$element_name] is defined, if it isn't print out blank, " " else print $values[$element_name].

function input_text($element_name, $values) {
    print '<input type="text" name="' . $element_name .'" value="';
    print htmlentities($values[$element_name]) . '"/>';
}

//print a textarea
function input_textarea($element_name, $values) {
    print '<textarea name="' . $element_name .'">';
    print htmlentities($values[$element_name]) . '</textarea>';
}

I tried to write something like this:

function input_text($element_name, $values) {
    print '<input type="text" name="' . $element_name .'" value="';
if (isset($values[$element_name])); {
    print htmlentities($values[$element_name]) . '"/>';
}
else {
print ("%20" . '"/>');
}
}

Which didn't work ::)

Lodius2000 · April 7, 2009

yeah ive been unsuccessful in fixing the 'multiple' part of the select function of formhelpers.php too, but I havent ever needed to use it so i havent put my all into it, but just to let you know, I found an error in it too

also, in my expirience most unsubmitted form field have an empty 'value' attribute, I have made many forms using formhelpers.php and tey validate transitional just fine

PFMaBiSmAd · April 7, 2009

For the multi-select problem, display the $_POST data to see what it is -

echo "<pre>";
echo "POST:";
print_r($_POST);
echo "</pre>";

Edit: multi-select problem is because in line 46 -

if ($multiple) { print '[ ]" multiple="multiple'; }

There are two spaces between the []. No spaces is correct. One space does work, but two spaces does not work.

acialk · April 7, 2009

I changed from POST to GET to see what was being submitted and this is the problem:

&main_dish[++]=stomach&main_dish[++]=taro

When I changed the pluses to 0 and 1 respectively the form worked. I can't work out what in the code is causing that though.

Once that's fixed and those undefined index errors in the value fields this will work fine

PFMaBiSmAd · April 7, 2009

I edited my post above while you were making your last post. The problem is extra spaces in line 46.

acialk · April 7, 2009

For the multi-select problem, display the $_POST data to see what it is -
echo "<pre>";
echo "POST:";
print_r($_POST);
echo "</pre>";
Edit: multi-select problem is because in line 46 -

if ($multiple) { print '[ ]" multiple="multiple'; }

There are two spaces between the []. No spaces is correct. One space does work, but two spaces does not work.

Ah!

The output from what you recommended to try was an empty array key, I replaced all the instances of [ ] with [] and it fixed the problem.

I just have those undefined index errors I'd like to fix.

//print a text box
function input_text($element_name, $values) {
print '<input type="text" name="' . $element_name .'" value="';
print htmlentities($values[$element_name]) . '"/>';
}

//print a textarea
function input_textarea($element_name, $values) {
print '<textarea name="' . $element_name .'">';
print htmlentities($values[$element_name]) . '</textarea>';
}

On the first load of the form the text fields are prefilled with the following:

Notice: Undefined index: name in C:\wamp\www\phpplay\index.php on line 13

Notice: Undefined index: comments in C:\wamp\www\phpplay\index.php on line 25

I've tried to write my own code to spit out blank if $values[$element_name] is not set but it fails to work. Can you help me with this last hurdle?

Thanks!

acialk · April 7, 2009

I managed to fix those two particular errors by using the following code:

//print a text box
function input_text($element_name, $values) {
if (isset($values[$element_name])) {
    print '<input type="text" name="' . $element_name .'" value="';
print htmlentities($values[$element_name]) . '"/>';
}
else {
    print '<input type="text" name="' . $element_name .'" value="';
print '"/>';

}
}

//print a textarea
function input_textarea($element_name, $values) {
if (isset($values[$element_name])) {
    print '<textarea name="' . $element_name .'">';
    print htmlentities($values[$element_name]) . '</textarea>';
	}
else {
    print '<textarea name="' . $element_name . '">';
    print '</textarea>';
}
}

I've found some more undefined indexes whilst testing the form further. I think I can fix those with what I've done above. I'd like to get your thoughts on the above code. Is that ok?

Sign In

Can someone critique book author's code pls

Recommended Posts

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Link to comment

Share on other sites

Join the conversation

Important Information