Function rewrite for developer accessibility

[jdevalk]

A lot of the code could be much more accessible to other developers.

Take this random function for example:

/**
 * Simplified summary markup
 *
 * @param int $post Post ID.
 * @return string
 */
function edac_simplified_summary_markup( $post ) {
	$simplified_summary         = get_post_meta( $post, '_edac_simplified_summary', true ) ? get_post_meta( $post, '_edac_simplified_summary', true ) : '';
	$simplified_summary_heading = 'Simplified Summary';

	// filter title.
	if ( has_filter( 'edac_filter_simplified_summary_heading' ) ) {
		$simplified_summary_heading = apply_filters( 'edac_filter_simplified_summary_heading', $simplified_summary_heading );
	}

	if ( $simplified_summary ) {
		return '<div class="edac-simplified-summary"><h2>' . wp_kses_post( $simplified_summary_heading ) . '</h2><p>' . wp_kses_post( $simplified_summary ) . '</p></div>';
	} else {
		return;
	}
}

It's not terrible, but why not make it easier for developers to understand it and work on it?

/**
 * Get the simplified summary markup for a post.
 *
 * @param int $post Post ID.
 *
 * @return string Returns a string with a wrapper div, 
 *                heading and paragraph with the simplified summary.
 */
function edac_simplified_summary_markup( $post ) {
	$simplified_summary = get_post_meta( $post, '_edac_simplified_summary', true )
		? get_post_meta( $post, '_edac_simplified_summary', true ) 
		: '';

	$simplified_summary_heading = apply_filters(
		'edac_filter_simplified_summary_heading',
		esc_html__( 'Simplified Summary', 'accessibility-checker' )
	);

	if ( ! $simplified_summary ) {
		return '';
	}
	$markup  = '<div class="edac-simplified-summary">';
	$markup .= '<h2>' . wp_kses_post( $simplified_summary_heading ) . '</h2>';
	$markup .= '<p>' . wp_kses_post( $simplified_summary ) . '</p>';
	$markup .= '</div>';

	return $markup;
}

The changes were simple:

• Tweak the doc block to make it easier to understand that the function "gets" something; I don't need to read it to understand that it doesn't echo anything.
• Add documentation for the @return of the function.
• Split the long line into multiple lines.
• Make sure that text strings are properly escaped and translatable.
• Remove the has_filter check. It's unnecessary.
• If there is no summary, bail early. Important: return a string instead of null, which is what was happening before. The function is supposed to return a string, so it should return a string.
• Build the markup and return it in multiple lines instead of a single line. Easier to read and understand.

Refactoring small things like that doesn't take more than a few minutes, but can have a huge impact in time, as it decreases the time it takes for new developers to understand what happens. It makes maintenance and debugging a lot easier.

Conditions should make sense

We encounter conditions like this one:

if ( ( 'full-scan' === $mode && $pro )
        ||
        ( 'ui' === $mode || 'editor-scan' === $mode
        &&
        $post_id && current_user_can( 'edit_post', $post_id ) )
) {

may seem simple, but there is a lot of underlying complexity that requires analysis. It's easier to comprehend if we consider the ||/&& priorities and group things so that visually it's easier to comprehend. Remember that this is one of the simple examples, there are others more complex in the codebase. This is how I would write it:

if ( ( 'full-scan' === $mode && $pro ) 
    || 'ui' === $mode 
    || ( 'editor-scan' === $mode 
        && $post_id 
        && current_user_can( 'edit_post', $post_id ) 
    )
) {

Grouping things where they belong makes it easier to understand what happens where. In this case, the 2nd set of parentheses contained both the ui condition and the editor-scan conditions. However, the ||/&& prioritization was such that the logic was not obvious. By placing the ui condition outside the parentheses, the logic remained the same but it's now easier to parse visually.

Code simplifications

What is the point of this? (from enqueue-scripts.php)

if ( is_array( $post_types ) && in_array( $current_post_type, $post_types, true ) ) {
    $active = true;
} else {
    $active = false;
}

Why not just like this?

$active = ( is_array( $post_types ) && in_array( $current_post_type, $post_types, true ) );

Same thing with this (from options-page.php):

if ( $interset ) {
    return true;
} else {
    return false;
}

Why not just like this?

return (bool) $interset;

There are dozens of cases where I see this pattern!