Function rewrite 2

[jdevalk]

Example of a rewrite

As iterated in the first rewrite issue, more attention should be paid to the details. For example, this extremely simple function:

/**
 * Sanitize the text position value before being saved to database
 *
 * @param array $position Position value.
 * @return array
 */
function edac_sanitize_simplified_summary_position( $position ) {
	if ( in_array( $position, array( 'before', 'after', 'none' ), true ) ) {
		return $position;
	}
}

• Docblock error: $position is not an array, it's a string.
• Docblock error: the function doesn't return an array, it returns a string.
• Performance issue: in array() is slow.
• Return error: if $position is not one of the 3 values, the function returns null instead of a default value - which we already know is after.

Should be rewritten like this:

/**
 * Sanitize the text position value before being saved to database.
 * If the value is not one of the 3 allowed values, it will be set to "after".
 *
 * @param string $position The position value to sanitize.
 *
 * @return string Returns the sanitized position value, 
 *                or "after" if the value is not one of the 3 allowed values.
 */
function edac_sanitize_simplified_summary_position( $position ) {
    return ( 'before' === $position || 'after' === $position || 'none' === $position )
        ? $position
        : 'after';
}

or even like this if we want it to be easier to read:

/**
 * Sanitize the text position value before being saved to database.
 * If the value is not one of the 3 allowed values, it will be set to "after".
 *
 * @param string $position The position value to sanitize.
 *
 * @return string Returns the sanitized position value, 
 *                or "after" if the value is not one of the 3 allowed values.
 */
function edac_sanitize_simplified_summary_position( $position ) {
    switch ( $position ) {
        case 'before':
        case 'after':
        case 'none':
            return $position;

        default:
            return 'after';
    }
}

It might be a bit longer, but it's also easier to maintain long-term. If we had PHPUnit tests, we could easily test that function. As it is now, it would throw errors - because it's wrong.

The same thing applies to edac_sanitize_simplified_summary_prompt and dozens of others.

The use of switch

We don't use switch () anywhere in the code right now. As seen above, using switch () instead of if ()/elseif ()/else can make the code easier to read and maintain in some cases - especially when there are many conditions.