3
\$\begingroup\$

I am trying to learn OOP in PHP. My PHP knowledge is pretty good, but haven't tried OOP before. I'm currently building a formbuilder class. Can you take a look and tell me if this is the right way?

<?php
class Forms {

    protected $_inputs;
    private $_method, $_action;

    public function __construct($method,$action = NULL)
    {
        $this->_method = $method;
        $this->_action = $action;
    }

    public function addInput($type,$name,$value,$placeholder = NULL,$extra = NULL) {
        $this->_inputs[] = array (
                                'type' => $type,
                                'name' => $name,
                                'value' => $value,
                                'placeholder' => $placeholder,
                                'extra' => $extra
        );
    }

    public function addLabel($name) {
        $this->_inputs[] = array (
                                'type' => 'label',
                                'name' => $name

        );
    }

    public function addTextarea($name,$value = NULL, $placeholder = NULL) {
        $this->_inputs[] = array (
                                'type' => 'textarea',
                                'name' => $name,
                                'value' => $value,
                                'placeholder' => $placeholder   
        );
    }

    public function printForm() {
        $html = '<form action="'.$this->_action.'" method="'.$this->_method.'">';
        for ($i = 0; $i < count($this->_inputs); $i++) {
            switch($this->_inputs[$i]['type']) {

                case "label":
                    $html .= '<label for="'.$this->_inputs[$i]['name'].'">'.$this->_inputs[$i]['name'].'</label>';
                    break;
                case "text":
                case "password":
                case "submit":
                case "email":
                    $html .= '<input type="'.$this->_inputs[$i]['type'].'" ';
                    $html .= ' name="'.$this->_inputs[$i]['name'].'" ';
                    $html .= ' value="'.$this->_inputs[$i]['value'].'" ';
                    $html.= ' placeholder="'.$this->_inputs[$i]['placeholder'].'"> ';
                    $html .= ' '.$this->_inputs[$i]['extra'].'';
                    break;  
                case "textarea":
                    $html .= '<textarea name="'.$this->_inputs[$i]['name'].' value="'.$this->_inputs[$i]['value'].'" placeholder="'.$this->_inputs[$i]['placeholder'].'"></textarea>';
                    break;      
            }

        }
        $html .= '</form>';
        print($html);
    }
 } 
?>

This is how I make the form on a page:

<?php
$form3 = new Forms('post');
$form3->addLabel('username');
$form3->addInput('text','username','','Username','<br>');
$form3->addLabel('password');
$form3->addInput('password','password','','Password','<br>');
$form3->addInput('submit','submit_login','Login2');
$form3->printForm();
?>
\$\endgroup\$
2
\$\begingroup\$

I think your code is good overall, I only have a couple of small points:

  • I wouldn't print inside the form class, but return the HTML instead, and then print inside the calling class.
  • I don't like the way you handle labels, as it's very inflexible. I have to use the same for value as the text of the label (what if the text is "5 Cats", which isn't allowed as id?). It's also not clear from the outside that this is how it works (as you are missing PHPDoc comments, and the parameter is called name, not id or for.
  • You are missing some functionality, such as adding classes or ids to the tags. Maybe you can add a generic addAttributeTo($name $key, $value) method, which adds the key/value pair to the element specified by name.
  • I would think about defending XSS in this class, so the caller doesn't have to remember doing it.
|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ Thanks for your answer, and advices. now i know my way of thinking was good while writing this, i will continue this way. I will take your advices in mind while programming. The addAtributeTo was something i was thinking of, but i wanted to check if my start was good. \$\endgroup\$ – Rflujowa Mar 7 '15 at 10:52
  • \$\begingroup\$ its for with the labels. check this link : w3schools.com/tags/tag_label.asp \$\endgroup\$ – Rflujowa Mar 11 '15 at 8:32

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.