HiveBrain v1.2.0
Get Started
← Back to all entries
patternphpMinor

First WordPress template

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
firsttemplatewordpress

Problem

I wrote my first template, but didn't include any styling. What do you think? What's good what's bad?


" id="productForm" method="post">

There has been an error.";
?>
    
        
            Address*:
            " required="required" placeholder="Enter place">
                
            
        

        
            Email*:
            " required="required" placeholder="Enter email">
                
            
        

        
            Size*:
            ">
                ">
                ">cm
                
            
        
   
    Submit

Solution

It's quite fine. Something I don't like though is the mild overuse / abuse of isset and strlen.
Take this code for example:

$allFormPosts = array("address", "email", "xsize", "ysize", "zsize");
foreach($allFormPosts as $formPost){
    if(isset($_POST[$formPost])){
        ${$formPost} = trim($_POST[$formPost]);
    }else{
        $formNotValid = true;
        break;
    }
}

if(!isset($formNotValid)){


The main purpose of isset is to check if something was set at all,
and is best used to check things not within your control.
isset($_POST[$formPost]) is an appropriate use case:
indeed, you have no control over what was sent to your script,
whether from a proper user input form, or possibly from a curl script.

On the other hand, you do have control over $formNotValid;
it's your own variable you defined yourself, and you should use it better.
This variable should be a boolean: it should be either true or false,
there is no point in permitting "not set".

It would be better to initialize this variable yourself,
and eliminate the possibility of it not being set.
Initialize it to false,
check your form with the loop as before,
updating $formNotValid as needed,
and then do a boolean check instead of an isset:

$formNotValid = false;
foreach($allFormPosts as $formPost){
    // ...
}

if(!$formNotValid){


You cannot know what scripts get included before your script is called.
Some other script could possibly set a value to $formNotValid,
breaking your script.
By initializing it explicitly to a meaningful value yourself,
you eliminate an ambiguity, and with it a bug waiting to happen.

I recommend the same treatment for $hasError as well.
Initialize it first to false, and check it with if ($hasError) { ... } instead of resorting to isset.

Although it's true that strlen should always return 0 or higher,
it would be more natural to write conditions on strlen like this:

// this is kind of "clever"
//if(!strlen($address)){

// this is more natural
if (strlen($address) == 0) {


This may be a matter of taste though.

Borrowing from the standard in other languages,
I recommend using more spaces in loops and conditionals, like this, notice the difference from the original:

foreach ($allFormPosts as $formPost) {
    if (isset($_POST[$formPost])) {
        // ...
    } else {
        // ...
    }
}

Code Snippets

$allFormPosts = array("address", "email", "xsize", "ysize", "zsize");
foreach($allFormPosts as $formPost){
    if(isset($_POST[$formPost])){
        ${$formPost} = trim($_POST[$formPost]);
    }else{
        $formNotValid = true;
        break;
    }
}

if(!isset($formNotValid)){
$formNotValid = false;
foreach($allFormPosts as $formPost){
    // ...
}

if(!$formNotValid){
// this is kind of "clever"
//if(!strlen($address)){

// this is more natural
if (strlen($address) == 0) {
foreach ($allFormPosts as $formPost) {
    if (isset($_POST[$formPost])) {
        // ...
    } else {
        // ...
    }
}

Context

StackExchange Code Review Q#68030, answer score: 2

Revisions (0)

No revisions yet.