Skip to main content
added 8 characters in body
Source Link
mickmackusa
  • 8.8k
  • 1
  • 17
  • 31
  1. Study PSR-12 coding standards and obey every rule. This will make your code easier for you and others to read. Specifically pay attention to when and where to write curly braces.

  2. I see A LOT of misuses/abuses of empty(). The call is most appropriate when needing to determine two things at the same time:

    1. When a variable is not declared AND
    2. when the value is falsey (which includes a zero).

    If a variable is unconditionally declared in a method's signature, then empty() can be replaced with a function-less truthy check or strlen() if desired.

    Calling empty() after isset() or array_key_exists() or key_exists() is an antipattern for the same reason.

    Checking count($data) < 1 after !empty() will never be satified. An empty array is "falsey" data.

  3. Use return type declarations like void and string where appropriate.

  4. I endorse the use of the sprintf() or vsprintf() to print variables to strings. This will make it easier to use double quotes in your generated HTML markup.

  5. I personally never use and or or in PHP code. This helps to avoid precedence issues and assures code consistency.

  6. Endeavor to never use magic numbers. Even checks against 1 or 2 should be avoided. If these numbers have a logical meaning, declare class level constants with indicative names which logically explain their use.

  7. If your class variables (private properties) are not expected to change, write them as constants.

  8. While much of the PHP applications running on the web are building HTML markup from PHP files, there is a growing trend to use client-side languages to generate and populate HTML documents.

  9. When possible, try to avoid using vague variable names like $data, $output, etc. Indicative variable names like $attr, $tag, $form, and $html add meaning. As a general cue, when you find a variable declaration in a method signature that is identical to its neighboring type declaration, this is a symptom that an opportunity to convey meaning is being missed.

  10. I never declare a variable as null if it is intended to contain array-type data (see $attributes). It will be cleaner to define it as an empty array, populate it if appropriate, not check if is is an array later, and simply access it.

  1. Study PSR-12 coding standards and obey every rule. This will make your code easier for you and others to read. Specifically pay attention to when and where to write curly braces.

  2. I see A LOT of misuses/abuses of empty(). The call is most appropriate when needing to determine two things at the same time:

    1. When a variable is not declared AND
    2. when the value is falsey (which includes a zero).

    If a variable is unconditionally declared in a method's signature, then empty() can be replaced with a function-less truthy check or strlen() if desired.

    Calling empty() after isset() or array_key_exists() or key_exists() is an antipattern for the same reason.

    Checking count($data) < 1 after !empty() will never be satified. An empty array is "falsey" data.

  3. Use return type declarations like void and string where appropriate.

  4. I endorse the use of the sprintf() or vsprintf() to print variables to strings. This will make it easier to use double quotes in your generated HTML markup.

  5. I personally never use and or or in PHP code. This helps to avoid precedence issues and assures code consistency.

  6. Endeavor to never use magic. Even checks against 1 or 2 should be avoided. If these numbers have a logical meaning, declare class level constants with indicative names which logically explain their use.

  7. If your class variables (private properties) are not expected to change, write them as constants.

  8. While much of the PHP applications running on the web are building HTML markup from PHP files, there is a growing trend to use client-side languages to generate and populate HTML documents.

  9. When possible, try to avoid using vague variable names like $data, $output, etc. Indicative variable names like $attr, $tag, $form, and $html add meaning. As a general cue, when you find a variable declaration in a method signature that is identical to its neighboring type declaration, this is a symptom that an opportunity to convey meaning is being missed.

  10. I never declare a variable as null if it is intended to contain array-type data (see $attributes). It will be cleaner to define it as an empty array, populate it if appropriate, not check if is is an array later, and simply access it.

  1. Study PSR-12 coding standards and obey every rule. This will make your code easier for you and others to read. Specifically pay attention to when and where to write curly braces.

  2. I see A LOT of misuses/abuses of empty(). The call is most appropriate when needing to determine two things at the same time:

    1. When a variable is not declared AND
    2. when the value is falsey (which includes a zero).

    If a variable is unconditionally declared in a method's signature, then empty() can be replaced with a function-less truthy check or strlen() if desired.

    Calling empty() after isset() or array_key_exists() or key_exists() is an antipattern for the same reason.

    Checking count($data) < 1 after !empty() will never be satified. An empty array is "falsey" data.

  3. Use return type declarations like void and string where appropriate.

  4. I endorse the use of the sprintf() or vsprintf() to print variables to strings. This will make it easier to use double quotes in your generated HTML markup.

  5. I personally never use and or or in PHP code. This helps to avoid precedence issues and assures code consistency.

  6. Endeavor to never use magic numbers. Even checks against 1 or 2 should be avoided. If these numbers have a logical meaning, declare class level constants with indicative names which logically explain their use.

  7. If your class variables (private properties) are not expected to change, write them as constants.

  8. While much of the PHP applications running on the web are building HTML markup from PHP files, there is a growing trend to use client-side languages to generate and populate HTML documents.

  9. When possible, try to avoid using vague variable names like $data, $output, etc. Indicative variable names like $attr, $tag, $form, and $html add meaning. As a general cue, when you find a variable declaration in a method signature that is identical to its neighboring type declaration, this is a symptom that an opportunity to convey meaning is being missed.

  10. I never declare a variable as null if it is intended to contain array-type data (see $attributes). It will be cleaner to define it as an empty array, populate it if appropriate, not check if is is an array later, and simply access it.

added a code snippet for inspirational purposes
Source Link
mickmackusa
  • 8.8k
  • 1
  • 17
  • 31

LATE EDIT: here is an untested rewrite of the add_field() method implementing some of my advice above. Note: I don't personally like to use snake_case in my projects and I think that the logical segments of this method should be abstracted into small methods which can then be more easily managed and potentially overridden.

/**
 * @throws Exception
 */
public function add_field(string $tag_name, array $attributes = [], array $options = []): void
{
    // validate tag name
    if (!preg_match('/^[a-z][\w-]*$/i', $tag_name)) {  // this is not well researched
        throw new Exception('Name of form element is not valid');
    }

    $html = '';
    $type = null;

    // accommodate fringe case tag names
    if (!in_array($tag_name, $this->form_elements)) {
        $found = false;
        foreach ($this->form_elements_types as $sub_element => $sub_types) {
            if (in_array($tag_name, $sub_types)) {
                $type = $tag_name;
                $tag_name = $sub_element;
                $found = true;
                break;
            }
        }
        if (!$found) {
            throw new Exception($tag_name . ': illegal form (type) element');
        }
    }

    $html .= "<$tag_name";

    // required attributes
    foreach ($this->form_elements_required_attr[$tag_name] ?? [] as $attr) {
        if (!key_exists($attr, $attributes)) {
            throw new Exception(
                sprintf(
                    '"%s%s" attribute "%s" is mandatory',
                    $tag_name,
                    $type === null ? '' : " $type",
                    $attr
                )
            );
        }
        if ($attr !== 'value' && !strlen($attributes[$attr])) {
            throw new Exception(
                sprintf(
                    '"%s%s" attribute "%s" cannot be empty',
                    $tag_name,
                    $type === null ? '' : " $type",
                    $attr
                )
            );
        }

        // append valid attribute
        if ($attr === "type" && $type) {
            $html .= sprintf(
                ' %s="%s"',
                $attr,
                htmlentities($type ?? $attributes[$attr])
            );
        }
    }

    // optional attributes
    foreach ($this->form_elements_specific_attr[$tag_name] ?? [] as $attr) {
        if (key_exists($attr, $attributes)) {
            $html .= sprintf(
                ' %s%s',
                $attr,
                in_array($attr, ['required', 'autofocus']) ? '' : ' "' . htmlentities($attributes[$attr]) . '"'
            );
        }
    }

    // common attributes
    foreach ($this->form_elements_common_attr as $attr) {
        if (key_exists($attr, $attributes) && strlen($attributes[$attr])) {
            $html .= sprintf(
                ' %s="%s"',
                $attr,
                htmlentities($attributes[$attr])
            );
        }
    }

    $html .= ">";

    // build <select> options
    if ($tag_name == "select") {
        // todo: shouldn't <optgroup> tags be implemented here?
        // todo: how does a developer designate the selected option?
        // todo: what about multiselect fields?
        foreach ($options as $value => $text) {
            $html .= sprintf(
                "\n<option%s>%s</option>",
                $value === $text ? '' : ' value="' . htmlentities($text) . '"',
                $text
            );
        }

    // append pseudo-attribute value as element's text (e.g. textarea, label, etc.)
    } elseif ($tag_name !== 'input' && key_exists('value', $attributes)) {
        $html .= htmlentities($attributes['value']);
    }
    
    // append closing tag 
    if (in_array($tag_name, $this->form_tag_closing)) {
        $html .= "</$tag_name>";
    }
    
    $this->output_elements[] = $html;
}

LATE EDIT: here is an untested rewrite of the add_field() method implementing some of my advice above. Note: I don't personally like to use snake_case in my projects and I think that the logical segments of this method should be abstracted into small methods which can then be more easily managed and potentially overridden.

/**
 * @throws Exception
 */
public function add_field(string $tag_name, array $attributes = [], array $options = []): void
{
    // validate tag name
    if (!preg_match('/^[a-z][\w-]*$/i', $tag_name)) {  // this is not well researched
        throw new Exception('Name of form element is not valid');
    }

    $html = '';
    $type = null;

    // accommodate fringe case tag names
    if (!in_array($tag_name, $this->form_elements)) {
        $found = false;
        foreach ($this->form_elements_types as $sub_element => $sub_types) {
            if (in_array($tag_name, $sub_types)) {
                $type = $tag_name;
                $tag_name = $sub_element;
                $found = true;
                break;
            }
        }
        if (!$found) {
            throw new Exception($tag_name . ': illegal form (type) element');
        }
    }

    $html .= "<$tag_name";

    // required attributes
    foreach ($this->form_elements_required_attr[$tag_name] ?? [] as $attr) {
        if (!key_exists($attr, $attributes)) {
            throw new Exception(
                sprintf(
                    '"%s%s" attribute "%s" is mandatory',
                    $tag_name,
                    $type === null ? '' : " $type",
                    $attr
                )
            );
        }
        if ($attr !== 'value' && !strlen($attributes[$attr])) {
            throw new Exception(
                sprintf(
                    '"%s%s" attribute "%s" cannot be empty',
                    $tag_name,
                    $type === null ? '' : " $type",
                    $attr
                )
            );
        }

        // append valid attribute
        if ($attr === "type" && $type) {
            $html .= sprintf(
                ' %s="%s"',
                $attr,
                htmlentities($type ?? $attributes[$attr])
            );
        }
    }

    // optional attributes
    foreach ($this->form_elements_specific_attr[$tag_name] ?? [] as $attr) {
        if (key_exists($attr, $attributes)) {
            $html .= sprintf(
                ' %s%s',
                $attr,
                in_array($attr, ['required', 'autofocus']) ? '' : ' "' . htmlentities($attributes[$attr]) . '"'
            );
        }
    }

    // common attributes
    foreach ($this->form_elements_common_attr as $attr) {
        if (key_exists($attr, $attributes) && strlen($attributes[$attr])) {
            $html .= sprintf(
                ' %s="%s"',
                $attr,
                htmlentities($attributes[$attr])
            );
        }
    }

    $html .= ">";

    // build <select> options
    if ($tag_name == "select") {
        // todo: shouldn't <optgroup> tags be implemented here?
        // todo: how does a developer designate the selected option?
        // todo: what about multiselect fields?
        foreach ($options as $value => $text) {
            $html .= sprintf(
                "\n<option%s>%s</option>",
                $value === $text ? '' : ' value="' . htmlentities($text) . '"',
                $text
            );
        }

    // append pseudo-attribute value as element's text (e.g. textarea, label, etc.)
    } elseif ($tag_name !== 'input' && key_exists('value', $attributes)) {
        $html .= htmlentities($attributes['value']);
    }
    
    // append closing tag 
    if (in_array($tag_name, $this->form_tag_closing)) {
        $html .= "</$tag_name>";
    }
    
    $this->output_elements[] = $html;
}
Another point
Source Link
mickmackusa
  • 8.8k
  • 1
  • 17
  • 31

I have not conducted a deep review of your scripting, but I have a few pieces of superficial advice:

  1. Study PSR-12 coding standards and obey every rule. This will make your code easier for you and others to read. Specifically pay attention to when and where to write curly braces.

  2. I see A LOT of misuses/abuses of empty(). The call is most appropriate when needing to determine two things at the same time:

    1. When a variable is not declared AND
    2. when the value is falsey (which includes a zero).

    If a variable is unconditionally declared in a method's signature, then empty() can be replaced with a function-less truthy check or strlen() if desired.

    Calling empty() after isset() or array_key_exists() or key_exists() is an antipattern for the same reason.

    Checking count($data) < 1 after !empty() will never be satified. An empty array is "falsey" data.

  3. Use return type declarations like void and string where appropriate.

  4. I endorse the use of the sprintf() or vsprintf() to print variables to strings. This will make it easier to use double quotes in your generated HTML markup.

  5. I personally never use and or or in PHP code. This helps to avoid precedence issues and assures code consistency.

  6. Endeavor to never use magic. Even checks against 1 or 2 should be avoided. If these numbers have a logical meaning, declare class level constants with indicative names which logically explain their use.

  7. If your class variables (private properties) are not expected to change, write them as constants.

  8. While much of the PHP applications running on the web are building HTML markup from PHP files, there is a growing trend to use client-side languages to generate and populate HTML documents.

  9. When possible, try to avoid using vague variable names like $data, $output, etc. Indicative variable names like $attr, $tag, $form, and $html add meaning. As a general cue, when you find a variable declaration in a method signature that is identical to its neighboring type declaration, this is a symptom that an opportunity to convey meaning is being missed.

  10. I never declare a variable as null if it is intended to contain array-type data (see $attributes). It will be cleaner to define it as an empty array, populate it if appropriate, not check if is is an array later, and simply access it.

I have not conducted a deep review of your scripting, but I have a few pieces of superficial advice:

  1. Study PSR-12 coding standards and obey every rule. This will make your code easier for you and others to read. Specifically pay attention to when and where to write curly braces.

  2. I see A LOT of misuses/abuses of empty(). The call is most appropriate when needing to determine two things at the same time:

    1. When a variable is not declared AND
    2. when the value is falsey (which includes a zero).

    If a variable is unconditionally declared in a method's signature, then empty() can be replaced with a function-less truthy check or strlen() if desired.

    Calling empty() after isset() or array_key_exists() or key_exists() is an antipattern for the same reason.

    Checking count($data) < 1 after !empty() will never be satified. An empty array is "falsey" data.

  3. Use return type declarations like void and string where appropriate.

  4. I endorse the use of the sprintf() or vsprintf() to print variables to strings. This will make it easier to use double quotes in your generated HTML markup.

  5. I personally never use and or or in PHP code. This helps to avoid precedence issues and assures code consistency.

  6. Endeavor to never use magic. Even checks against 1 or 2 should be avoided. If these numbers have a logical meaning, declare class level constants with indicative names which logically explain their use.

  7. If your class variables (private properties) are not expected to change, write them as constants.

  8. While much of the PHP applications running on the web are building HTML markup from PHP files, there is a growing trend to use client-side languages to generate and populate HTML documents.

  9. When possible, try to avoid using vague variable names like $data, $output, etc. Indicative variable names like $attr, $tag, $form, and $html add meaning. As a general cue, when you find a variable declaration in a method signature that is identical to its neighboring type declaration, this is a symptom that an opportunity to convey meaning is being missed.

I have not conducted a deep review of your scripting, but I have a few pieces of superficial advice:

  1. Study PSR-12 coding standards and obey every rule. This will make your code easier for you and others to read. Specifically pay attention to when and where to write curly braces.

  2. I see A LOT of misuses/abuses of empty(). The call is most appropriate when needing to determine two things at the same time:

    1. When a variable is not declared AND
    2. when the value is falsey (which includes a zero).

    If a variable is unconditionally declared in a method's signature, then empty() can be replaced with a function-less truthy check or strlen() if desired.

    Calling empty() after isset() or array_key_exists() or key_exists() is an antipattern for the same reason.

    Checking count($data) < 1 after !empty() will never be satified. An empty array is "falsey" data.

  3. Use return type declarations like void and string where appropriate.

  4. I endorse the use of the sprintf() or vsprintf() to print variables to strings. This will make it easier to use double quotes in your generated HTML markup.

  5. I personally never use and or or in PHP code. This helps to avoid precedence issues and assures code consistency.

  6. Endeavor to never use magic. Even checks against 1 or 2 should be avoided. If these numbers have a logical meaning, declare class level constants with indicative names which logically explain their use.

  7. If your class variables (private properties) are not expected to change, write them as constants.

  8. While much of the PHP applications running on the web are building HTML markup from PHP files, there is a growing trend to use client-side languages to generate and populate HTML documents.

  9. When possible, try to avoid using vague variable names like $data, $output, etc. Indicative variable names like $attr, $tag, $form, and $html add meaning. As a general cue, when you find a variable declaration in a method signature that is identical to its neighboring type declaration, this is a symptom that an opportunity to convey meaning is being missed.

  10. I never declare a variable as null if it is intended to contain array-type data (see $attributes). It will be cleaner to define it as an empty array, populate it if appropriate, not check if is is an array later, and simply access it.

Source Link
mickmackusa
  • 8.8k
  • 1
  • 17
  • 31
Loading