patternphpMinor
First WordPress template
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
Take this code for example:
The main purpose of
and is best used to check things not within your control.
indeed, you have no control over what was sent to your script,
whether from a proper user input form, or possibly from a
On the other hand, you do have control over
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
check your form with the loop as before,
updating
and then do a boolean check instead of an
You cannot know what scripts get included before your script is called.
Some other script could possibly set a value to
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
Initialize it first to
Although it's true that
it would be more natural to write conditions on
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:
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.