Skip to main content
    if ( ! $this->self_contained) {
        if( ! empty($this->text)) { $result .= $this->text; }
        $result .= "</{$this->element}>";
    }
    if ( ! $this->self_contained) {
        if( ! empty($this->text)) { $result .= $this->text; }
        $result .= "</{$this->element}>";
    }
    else { substr_replace($result, '/>', -1); }
    else { substr_replace($result, '/>', -1); }
$form = new Form(array(
    'action' = 'index.php',
    'method' = Form::METHOD_GET,
);
//Compared to:
$form  = new Form("index.php", Form::METHOD_GET);
$form = new Form(array(
    'action' => 'index.php',
    'method' => Form::METHOD_GET,
);
//Compared to:
$form  = new Form("index.php", Form::METHOD_GET);
    if ( ! $this->self_contained) {
        if( ! empty($this->text)) { $result .= $this->text; }
        $result .= "</{$this->element}>";
    }
    else { substr_replace($result, '/>', -1); }
$form = new Form(array(
    'action' = 'index.php',
    'method' = Form::METHOD_GET,
);
//Compared to:
$form  = new Form("index.php", Form::METHOD_GET);
    if ( ! $this->self_contained) {
        if( ! empty($this->text)) { $result .= $this->text; }
        $result .= "</{$this->element}>";
    }
    else { substr_replace($result, '/>', -1); }
$form = new Form(array(
    'action' => 'index.php',
    'method' => Form::METHOD_GET,
);
//Compared to:
$form  = new Form("index.php", Form::METHOD_GET);
Source Link
mseancole
  • 6.2k
  • 1
  • 16
  • 27

If you cannot write form mockup, you leave it to the designer. Or if you are the only one working on the project you use online tools or similar libraries. I'm not saying what you have here is bad, far from it, I'm just saying solutions for this already exist and more well known solutions will be used before one by an unknown author. Just a simple fact of life, people are trendy and suspicious. If you want to use it for your own purposes, that is fine, just don't expect to create the new goto for form creation :) <- (the smile is there to let you know I don't mean this harshly).

I will review your code, though there is little for me to go over, it is already quite clear. Your code is well formed, well documented, and self documenting, so mostly I will be giving observations and suggestions. I will also provide you with input on what I think of your program, as I think that is more what you were looking for. My first input has already been given, so I'll continue with the code review then give my final thoughts towards the end.

There is no need to perform a check on the $self_contained variable twice, it is redundant.

    if ( ! $this->self_contained) {
        if( ! empty($this->text)) { $result .= $this->text; }
        $result .= "</{$this->element}>";
    }

Correct me if I'm wrong, but from what I can tell, your $self_contained check doesn't actually terminate the tag. Simply closing a tag, adding the greater-than bracket ">", does not terminate an element, you must either use the matched closing tag or a self terminator "/>". Most browsers (I believe all) will still process this, but it is still improper and could result in errors in the future. To do this proper add the following else statement to the end of the if statement I gave above.

    else { substr_replace($result, '/>', -1); }

Why are you not checking for empty strings and such? What if I were to create an input with no type? Not saying someone would do so purposefully, but what if they were using a variable to set the type and it somehow got emptied? Adding an error check here could save a lot of debugging time.

You've thrown all these errors but not caught any :(

Well, thats it for the code review, told you there wasn't much. Here's some thoughts I had in the form of suggestions.

I would think about passing associative arrays to your construction methods. This will make it more clear what you are doing. For example.

$form = new Form(array(
    'action' = 'index.php',
    'method' = Form::METHOD_GET,
);
//Compared to:
$form  = new Form("index.php", Form::METHOD_GET);

Yes, because of your self documenting code, it is a little more obvious, but assume someone just passes "get" as the second parameter. Two things will happen, the first is that it will throw an error, the second will be that people reading this won't know what it means. Adding strtoupper to your code will prevent that from throwing any errors and will allow people to continue to be lazy. People like being lazy. I'm people, I like being lazy, therefore my statement stands :) Making it use associative arrays will make it more apparent what you are doing.

Have you thought about adding a depth check? Something to determine the initial tabs to add to each new element? Not all forms will be added to the body tag. Some will be deeper.

No class for textareas?

I do believe your code could use some clarity on implementation. As I said in one of my comments, I'm not sure what is going on in your test. Well that's a lie. I read your program, I know whats going on. But someone who has never used your program before will look at that code and not be sure. They could infer much, but not everything. I for one, have not found where $field is used after it is declared, and I HAVE read your code. Maybe I am missing something? My suggestion about associative arrays will clear up your clarity issue somewhat.

Finally, I would change the TAB constant to \t rather than the escaped equivalent to keep consistent with all those \n's, and then I would make the \n a constant as well, but that is just me. Actually I wouldn't, I would just remove the TAB constant and start using \t. There's nothing wrong with repeatedly using escaped characters. There is no need to use variables or constants to define them, unless of course they are system specific, such as directory separators, but that is not the case here, at least not if all you are using is \n, \r would have to be added as well.

Good luck!