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.

What can I improve in this form builder? I'm looking for code review, optimization, and best practices.

<?php
/**
 * Project: validator - File: form_builder.php
 * Author: danutz0501 ©copyright 2014
 * Date : (m/y) 06/2014
 */
class form_builder{
    private $form;

    public function __construct(){}

    public function __toString(){
        return $this->form;
    }
    public function add_node($type, array $args = []){
        $this->form .= '<'.$type.' '.$this->setup_args($args).'>'.PHP_EOL;
        return $this;
    }

    public function end_node($type){
        $this->form .= '</'.$type.'>'.PHP_EOL;
        return $this;
    }

    public function add_elem($text){
        $this->form .= $text.PHP_EOL;
        return $this;
    }

    public function img($data, array $args = []){
        $this->form .= '<img src="'.$data.'" '.$this->setup_args($args).'/>'.PHP_EOL;
        return $this;
    }

    public function start_form(array $args = []){
        $this->form .= '<form'.$this->setup_args($args).' >'.PHP_EOL;
        return $this;
    }

    public function end_form(){
        $this->form .= '</form>'.PHP_EOL;
        return $this;
    }

    public function input(array $args = []){
        $this->form .= '<input'.$this->setup_args($args).$this->value($args).' />'.PHP_EOL;
        return $this;
    }

    public function checkbox(array $args = []){
        $result = (count($args)) ? array_merge(['type' => 'checkbox'], $args) : ['type' => 'checkbox'];
        $this->input($result);
        return $this;
    }

    public function radio(array $args){
        $result = (count($args)) ? array_merge(['type' => 'radio'], $args) : ['type' => 'radio'];
        $this->input($result);
        return $this;
    }

    public function textarea(array $args = []){
        $this->form .= '<textarea '.$this->setup_args($args).' >'.$this->value($args, false).'</textarea>'.PHP_EOL;
        return $this;
    }

    public function select(array $value, array $args = []){
        $this->form .= '<select '.$this->setup_args($args).'>'.PHP_EOL;
        foreach($value as $key => $values){
            if(is_array($values)){
                $this->form .= "\t<optgroup label='".$key."'>".PHP_EOL;
                    foreach($values as $k => $v)
                        $this->form .= $this->populate_select($k, $v);
                $this->form .= "\t</optgroup>".PHP_EOL;
            }else{
                $this->form .= $this->populate_select($key, $values);
            }
        }
        $this->form .= '</select>'.PHP_EOL;
        return $this;
    }

    private function populate_select($key, $value){
        return "\t\t".'<option value="'.$key.'">'.ucfirst($value).'</option>'.PHP_EOL;
    }

    private function value(array $args = [], $type = true){
        return (!empty($args['value'])) ? ( $type ? ' value="'.$args['value'].'" ' : $args['value']): '';
    }

    private function setup_args(array $args = []){
        $str = '';
        if(count($args)){
            foreach($args as $key => $value){
                if($key == 'value')
                    continue;
                $str .= ' '.$key.'="'.$value.'" ';
            }
        }
        return $str;
    }
}

Usage:

$array = [
    'titlul lui optgroup1' => ['xx1'=>'xxx1', 'xx12'=>'xxx2', 'xx11'=>'xxx3'],
    'titlul lui optgroup2' => ['xx34' => 'xxx', '00' => 'xxx', 'xx232'=>'xxx1', 'xx43'=>'xxx2', 'xxdfdsf'=>'xxx3']
];
$array1 = ['val1'=>'xxl', 'xx'=>'xxx1', 'xx1'=>'xxx2', 'xx2'=>'xxx3'];
$img = 'data:image/jpeg;base64 .... etc';
$form->start_form(['action'=>'index.php', 'method'=>'post'])
     ->input(['type'=> 'text', 'name'=>'nume', 'class'=> 'test1 test2', 'value'=>'muhaha'])
     ->add_node('div')
     ->textarea(['value'=>'merge'])
     ->select($array)
     ->input(['type'=>'submit', 'value'=>'Trimite'])
     ->end_node('div')
     ->checkbox()
     ->add_elem('text1')
     ->checkbox(['class'=>'er4 34 tyd'])
    ->add_elem('text2<br />')
     ->img($img,['width'=>'200px', 'height'=>'200px'])
     ->end_form();
echo $form;
share|improve this question
add comment

2 Answers 2

Let's start with the dry review, and then jump to the juicy stuff.

  • Naming convention, it's common in PHP to name Class names in uppercase, class FormBuilder.
  • Empty constructor, if you don't need a constructor, why make one?
  • XSS vulnerabilities all over the place - You aren't escaping text or attributes which are directly injected into HTML! That's a big no-no. See this XSS cheat sheet.
  • Non-standard coding style. I'm talking about spacing and indentation mostly, but that's more up to your personal preference.

Now for the more juicy stuff. Is your library useful? Why would I, a user, want to use your library instead of just writing HTML? Or generating HTML for that matter? The usage code seems a bit over-engineered, too large and "clumsy" to actually be useful.

Truth be told, it's a nice project to improve your knowledge with XSS and the structure of HTML and perhaps even OOP, but it's not actually useful in the real world, because generating HTML is simply easier.

share|improve this answer
add comment
  • There're so many libraries that already do this. Yours is unfortunately not very good (lacks features, very limited, hard to expand, hard to maintain, difficult to follow/read, etc.). However I'll still review it.

  • Formatting is one of the most important parts of coding. The style you choose to code in can really make a huge difference for future maintenance men or maybe, code reviewers on an internet Q&A site. That said, I suggest you find a style, one you like, or one a framework you're working with suggests. To get you started, PHP FIG offers a good 4-part tale of standards and styles.

  • There's really no benefit of having a constructor do nothing. It only is adding clutter.

  • Your usage code never uses $array1. Strange that it'd be there.

  • Something such as '<'.$type.' '.$this->setup_args($args).'>'.PHP_EOL could be made much more readable with the sprintf function.

  • You need to work on method names. img is beyond awful.

  • What's the point in start_form()? Can't you just use add_node() for this?

  • value() is confusing as heck. After getting past the initial scare of such an ugly teranary, I realized that it has no need. It can be eliminated!

  • I can think of many ways to construct select(). Your way was not one of them. It looks confusing, which is generally a sign there are neater ways to write it, and having a foreach in a foreach can't be a good sign. See about working that down to only one loop.

  • Why all of a sudden, in populate_select() you decide to make the output all pretty with \t\t? Consistency is key!

  • In setup_args(), there's no point in having the count() check. If there's nothing in $args then the foreach won't bother to run.

  • Snippet improvement!

This right here:

if($key == 'value')
    continue;
$str .= ' '.$key.'="'.$value.'" ';

is needlessly over-the-top. How's about just

if ($key != 'value') {
    $str .= sprintf(' %s="%s" ', $key, $value);
}

Since we're here. What in the world is $str? Please please please don't name your variables like this.

  • Regarding checkbox(['class'=>'er4 34 tyd']): Those class names are absurd. I pray you just let your cat walk on the keyboard to come with this temporary lorem ipsum. They better not be real CSS class names.

  • @MadaraUchilha is right about the XSS vulnerabilities. They're a big no-no.

  • Last but not least, comments! Woo-hoo! Take a look at PHPDocs, they really can turn out to be a lifesaver.

share|improve this answer
    
nice review, some good points there, but xss? this is for generating forms(html), not for security, i use other class(function) for security and filter data before sending data to a html helper –  danutz0501 Jun 28 at 0:58
    
There could possibly be an exploitable XSS chance because we don't have any other calling code, and it would be incredibly easy to spoof something if you accepted user-input to build the DOM. –  Alex L Jun 28 at 1:02
    
1 more question, what's better for avoiding nested foreach: a spl recursive iterator or a recursive function? –  danutz0501 Jun 29 at 4:40
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.