2
\$\begingroup\$

I am new to the world of coding, PHP as well as XHTML. As my first stab at object oriented coding, I put together a basic registration form that is meant to serve as a means to collect user details.

What this form does not do is the following;

  • Capture information is a file or database as I have not yet gained enough experience about databases, sessions, cookies or files.
  • I looked at using onchange to populate the days dropdown menu dynamically but despise the form reloading and hence avoided doing so. I am aware I can use AJAX however considering my lack of knowledge, I was hoping someone could advise other alternatives apart from Javascript.
  • Sanitize the data using htmlentities or htmlspecialchars.

What I would like

  • I would appreciate any constructive criticisms, what I can improve on, what I could have done better

XHTML Code

<?php

require_once('includes/base.php');
require_once('functions.php');

?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
"http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">

<html xmlns="http://www.w3.org/1999/xhtml">
<head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8" />
    <meta name="keywords" content="" />
    <meta name="description" content= "" />
    <meta name="author" content="" />
    <title>Event Details</title>

    <link rel="stylesheet" type="text/css" href="web.css" />
</head>
<body>
    <h1>Details</h1>

    <p>Please provide us with your details so that we can confirm  your booking</p>

    <div>
        <?php
            $eventform = new eventform();
            $eventform -> validateform();
        ?>

    <fieldset>
        <legend>Your details</legend>
            <form name="eventdetails" method="post" action="event.php">
                <label for="fname">First Name</label>
                    <input type="text" name="fname" value="<?php echo isset($_POST['fname']) ? $_POST['fname'] : ''; ?>" tabindex="1" id="fname" />

                <label for="lname">Last Name</label>
                    <input type="text" name="lname" value="<?php echo isset($_POST['lname']) ? $_POST['lname'] : ''; ?>" tabindex="2" id="lname" />

                <label for="email">Email</label>
                    <input type="text" name="email" value="<?php echo isset($_POST['email']) ? $_POST['email'] : ''; ?>" tabindex="3" id="email" />

                <label for="cuisine">Cuisine</label>

                    <?php

                        $eventform = new eventform();
                        $eventform -> radiobuttons('cuisine');

                    ?>

                <label for="eventperiod">Please select an event day</label>

                    <?php

                        $eventform = new eventform();

                    ?>

                    <select name="eventmonth" tabindex="8" id="eventmonth">

                    <?php

                        echo $eventform -> dropdown('months');
                    ?>

                    </select>

                    <select name="eventday" tabindex="7" id="eventday">

                    <?php
                        echo $eventform -> dropdown('days');

                    ?>

                    </select>


                    <select name="eventyear" tabindex="9" id="eventyear">

                    <?php


                        echo $eventform -> dropdown('years');

                    ?>

                    </select>

                <label>
                    <input type="submit" name="register" value="register" tabindex="10" id="register" />
                    <input type="reset" name="reset" value="reset" tabindex="11" id="reset" />
                </label>
            </form>
    </fieldset>     
</body>
</html>

CSS Code

body{

font-family: arial, verdana, sans-serif;
font-size: 0.8em;
font-weight: normal;
font-style: normal;

}

h1 {

}

p {

}

fieldset {
border: 1px solid #D9D9D9;
background-color: #F5F5F5;

}

fieldset legend {

color: #FFFFFF;
background-color: #8B8378;
padding: 5px;
padding-right: 9px;
padding-left: 9px;

}

label {

display: block;
padding: 5px;

}

label.cuisine {

display: inline;

}

Inlcudes - Base

<?php

$parentdirectory = dirname(dirname(__FILE__));

define('BASE_DIRECTORY', $parentdirectory);
ini_set('include_path', $parentdirectory.'/includes/');
?>

Includes - Class

<?php

class eventform {

    //Declare data members/properties
    private $fname,
        $lname,
        $email,
        $cuisine,
        $eventday,
        $timezone,
        $eventmonth;


    //Generate months
    public function eventmonths($startingmonth=1, $endingmonth=12) {

    //Default timezone
    $this->timezone = date_default_timezone_set('UTC');

    //Loop through months
        $months = range($startingmonth, $endingmonth);

        $returnmonths = '';
        foreach($months as $key) {

            $returnmonths .= date('F', mktime(0, 0, 0, $key)).',';  
        }

        return rtrim($returnmonths, ',');
    }

    //Generate years
    public function eventyears() {

        //Get current year
        $year = date('Y');

        //Loop through years
        $startyear = $year;

        // $endyear = date('Y', strtotime('+5 years'));
        $endyear = date('Y', mktime(0,0,0,12,30,$year+5));

        $returnyears = '';
        for($startyear; $startyear < $endyear; $startyear=$startyear+1) {

            $returnyears .= $startyear.',';
        }

        return rtrim($returnyears, ',') ;
    }

    //Loop through days for a specific month and year
    public function eventdays($month, $year) {

        // return date('t', strtotime($someYear . '-' . $someMonth . '-01'));
        // return date('t', strtotime($someYear . '-' . $someMonth ));
        // $numberofdays = date('t', strtotime($Year. '-' .$Month));
        $numberofdays = date('t', mktime(0,0,0,$month,1,$year));

        $startdays = 1;
        $returndays = '';

        for($startdays; $startdays <= $numberofdays; $startdays=$startdays+1) {

            $returndays .= $startdays.',';

        }

        return rtrim($returndays, ',');
        // return substr($returndays, 0, -1);
    }

    //Define the type of dropdown
    public function dropdown($type) {

        switch($type) {

            case('days'):

                $stringdays = $this->eventdays(1, 2011);
                $arraydays = explode(',', $stringdays);

                foreach($arraydays as $day) {
                    echo '<option value="'.$day.'">'.$day.'</option>';
                }
            break;

            case('months'):

                $stringmonths = $this->eventmonths();
                $arraymonths = explode(',', $stringmonths);

                foreach($arraymonths as $month) {

                    echo '<option value="'.date('n', strtotime($month)).'">'.$month.'</option>';
                }
            break;

            case('years'):

                $stringyears = $this->eventyears();
                $arrayyears = explode(',', $stringyears);

                foreach($arrayyears as $key) {

                    echo '<option value="'.$key.'">'.$key.'</option>';

                }
            break;
        }           

    }


    //Define the type of radiobuttons
    public function radiobuttons ($type) {

        switch($type) {

            case('cuisine'):

            $cuisines = array('italian', 'russian', 'malaysian');
            $cuisinetype = array_combine(range(1, count($cuisines)), $cuisines);

                foreach($cuisinetype as $key => $value) {

                    if(isset($_POST['register']) && $_POST['register'] == 'register') {
                        if(isset($_POST['cuisine']) && $_POST['cuisine'] == $value) {

                            $status = 'checked=checked';
                        }

                        else {
                            $status = '';
                        }
                    }

                    else {

                        $status = '';
                    }

                    echo '<input type="radio" name="cuisine" value="'.$value.'" tabindex="'.$key.'" id="'.$value.'" '.$status.'>',
                        '<label for="'.$value.'" class="cuisine">'.ucfirst($value).'</label>';

                }
            break;
        }

    }


    //Validate form
    public function validateform() {

        if(isset($_POST['register']) && $_POST['register'] == 'register') {

            $this->fname = isset($_POST['fname']) ? $_POST['fname'] : '';
            $this->lname = isset($_POST['lname']) ? $_POST['lname'] : '';
            $this->email = isset($_POST['email']) ? $_POST['email'] : '';
            $this->cuisine = isset($_POST['cuisine']) ? $_POST['cuisine'] : '';
            $this->eventday = isset($_POST['eventday']) ? $_POST['eventday'] : '';

            $errormsg = array();

            if(empty($this->fname)) {
                $errormsg[0] = 'Please enter your first name';
            }

            if(empty($this->lname)) {
                $errormsg[1] = 'Please enter your last name';
            }

            if(empty($this->email)) {
                $errormsg[2] = 'Please specify an email address';
            }

            if(empty($this->cuisine)) {
                $errormsg[3] = 'Please select a cuisine';
            }

            if(empty($this->eventday)) {
                $errormsg[4] = 'Please select an event day';
            }

            foreach($errormsg as $key => $value) {  

                echo $value.'<br />';
            }


            //Basic email validation        
            if(!empty($this->email)) {

                if(!preg_match('/^[a-zA-Z0-9_\.\-]+[a-zA-Z0-9_\.\-]+@[a-zA-Z0-9_\.\-]+\.[a-zA-Z]{1,6}$/', $this->email)) {

                echo 'Please enter a valid email address';

                }
            }

        }

    }
}

?>
\$\endgroup\$
6
  • \$\begingroup\$ One quick critique: you shouldn't have echos in your class. Instead you should return strings and echo those strings in the xhtml. It makes things clearer. \$\endgroup\$ – generalhenry May 9 '11 at 4:42
  • \$\begingroup\$ @generalhenry - Thanks. How would I return strings prior to a break. My understanding is that a return would stop a function immediately especially if I have grouped case statements? \$\endgroup\$ – PeanutsMonkey May 9 '11 at 16:58
  • \$\begingroup\$ That's actually one of the nicer parts of using returns, you don't need to use a break unless there's more stuff added later on. Which makes it easy to read through the code and see where things end. A break might be the end, a return IS the end. \$\endgroup\$ – generalhenry May 9 '11 at 18:12
  • \$\begingroup\$ @generalhenry - When you say more stuff is added later on, do you mean other case statements? \$\endgroup\$ – PeanutsMonkey May 9 '11 at 19:35
  • \$\begingroup\$ I mean code beyond the case statements, replacing the breaks with returns makes it very clear that there is none with a quick glace at the code. \$\endgroup\$ – generalhenry May 9 '11 at 22:54
1
\$\begingroup\$

Moving to an answer to post some code examples

class file:

public function radiobuttons ($type) {
    switch($type) {
        case('cuisine'):
        $cuisines = array('italian', 'russian', 'malaysian');
        $cuisinetype = array_combine(range(1, count($cuisines)), $cuisines);
        $output = '';
          foreach($cuisinetype as $key => $value) {
                if(isset($_POST['register']) && $_POST['register'] == 'register') {
                    if(isset($_POST['cuisine']) && $_POST['cuisine'] == $value) {
                        $status = 'checked=checked';
                    }
                    else {
                        $status = '';
                    }
                }
                else {
                    $status = '';
                }
                $output .= '<input type="radio" name="cuisine" value="'.$value.'" tabindex="'.$key.'" id="'.$value.'" '.$status.'>',
                    '<label for="'.$value.'" class="cuisine">'.ucfirst($value).'</label>';
            }
        return $output;
    }
    //code here would run with a break
    // but not run with a return
    // so when reviewing the code you'd need to scroll down and check here if you use a break
    // but not if you use a return
}

xhtml file :

  <?php
      //makes it clear that it's outputing text
      echo $eventform -> radiobuttons('cuisine');
 ?>
\$\endgroup\$
1
\$\begingroup\$
  • Why you create class eventform 3 times in XHTML? I think one is enough.

  • <?php echo isset($_POST['email']) ? $_POST['email'] : ''; ?> This is a classical example of XSS-attack, don't do that. Use htmlspecialchars() to escape possible HTML-code.

  • Instead of $errormsg[0] = '...' you may just write $errormsg[] = '...' In this case PHP append your message to the end of array. Also this eliminates from magick numbers in your program.

Updated:

\$\endgroup\$
5
  • \$\begingroup\$ I wasn't aware I could instantiate the object once. I thought I had to instantiate each time I started a new code block. I didn't have a look at sanitizing the data as at this stage I was only looking at my attempt at object oriented code. Thanks for the reminder though. Should look at incorporating it from the get go. Cool. I'll bear that in mind. What are magick numbers? \$\endgroup\$ – PeanutsMonkey May 9 '11 at 16:57
  • \$\begingroup\$ @peanutsmonkey Magick numbers is "unique values with unexplained meaning or multiple occurrence which could (preferably) be replaced with named constants" (from en.wikipedia.org/wiki/Magic_number_%28programming%29) \$\endgroup\$ – php-coder May 9 '11 at 18:34
  • \$\begingroup\$ Thanks. Will have a read of that. How was the rest of my coding? Did my object oriented coding make sense? Was it easy to follow? \$\endgroup\$ – PeanutsMonkey May 9 '11 at 19:36
  • \$\begingroup\$ @peanutsmonkey When you use one object your code is not object oriented yet IMHO. I updated my answer and include more info. \$\endgroup\$ – php-coder May 10 '11 at 4:25
  • \$\begingroup\$ Thanks. Had no idea of single responsibility principle nor about the coding standards. Sorry I apologise ahead of time but when you say code style, what does that actually mean? I don't quite follow what the standards define. \$\endgroup\$ – PeanutsMonkey May 10 '11 at 23:23

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.