Review of https://github.com/WebDevStudios/StartBox/blob/2.7/includes/admin/admin.php - a file picked at random in the 2.7 branch.
(Tip: If going through and fixing bits mentioned here, start at the bottom - it will keep line references intact.)
Line 1: No file-level documentation. An example for the top of each file would be:
/**
* StartBox Framework
*
* @package WebDevStudios\StartBox
* @author Brian Richards <[email protected]>
* @license GPL-2.0+
* @link http://wpstartbox.com/
*/
3 (and throughout): End short descriptions, long descriptions and descriptions for tags with a full stop. When these strings are pulled out by a documentation generator, it'll read better.
6 (and throughout): No documentation of @global $sb_admin Admin page hook suffix.. Other globals also not documented.
13: Coding standards whitespace (CSWS), unnecessary use of double-quotes for a string, string not internationalized, line extends past 120 characters.
Whitespace inconsistency means grepping through the codebase is trickier.
Double-quotes means the string is parsed twice by PHP to resolve any variables that might be included.
Options string is not made available for translation.
Line length greater than 120 isn't part of the WP coding standards, but a general good practice. Splitting up the arguments on to their own lines also means they can be documented easier:
$sb_admin = add_theme_page(
THEME_NAME . __( ' Options', 'startbox' ), // Page title
__( 'Theme Options', 'startbox' ), // Menu title
'edit_theme_options', // Capability
'sb_admin', // Menu slug
'sb_admin_page' // Callback function
);16 (and throughout): CSWS. Use PHP CodeSniffer with the WP sniffs to automatically generate a report of issues to be fixed. You may like to try something like WP-PhpTidy that can help fix some coding standards issues automatically too.
23: Hooked in functions don't need to be defined before the corresponding add_*() call - that means you can move the add_*() calls to just above the documentation - if you've got a particularly long function, then all of the important bits (where the function is being hooked into, and documentation for the function) are all together, which makes it easier for folks not familiar with the code to follow.
28: All version numbers should contain three digits, so 2.4.0 and not 2.4. Apart from being a WP coding standard, it also means you can grep the code for functions that were added at exactly 2.4, and not have 2.4.1 et al included. @since tags should be on each and every function.
32: When passing in an array with more than one or two values, consider assigning it to a variable first. While the creation of a variable does use a little more memory (neglible compared to the existing amount of memory used for a request), it does make the code easier to read (avoids long line lengths, can be documented) and makes it easier to debug (can var_dump() the array to check what it is populated with. Also, include a trailing comma on the last array item, so then extra items can be added without having to remember to add the comma, or lines can be re-arranged without having to fix commas as well. Lining up the => with spaces does make the code easier to read as well. This, in this case, make it easy to see that there's some CSWS issues for the title value`.
$args = array(
'id' => 'theme-options',
'parent' => 'appearance',
'title' => __('Theme Options', 'startbox'),
'href' => admin_url( 'themes.php?page=sb_admin' )
);
$wp_admin_bar->add_menu( $args );Note that since WP 3.3, the Admin Bar is replaced with the toolbar and the preferred way to add items to the toolbar is with add_node().
45: sb_admin_help() has a lot of unnecessary nesting. If this codebase is still supporting pre WP 3.3, and as this function does nothing for earlier versions, consider moving the version check up to around line 20 so that the function isn't even hooked in.
49: Inconsistent coding style, which has lead to incorrect indentation for everything below in the function. While optional braces are indeed optional within WPCS, I'm currently prefering them since adding extra lines into the body of the conditional doesn't mean odd behaviour if the braces are not added, and it also stays in line with PSR-2:
if ( $screen->id != $sb_admin ) {
return;
}53: A database call via get_option() is assigned to $defaults and $theme_options, yet these variables don't appear to be then used.
58-60: Lines aligned with tabs not spaces. Since tabs can vary in width, this will make alignment in diffs look wrong, or on other systems. Just add the usual one space after the longest key, then line all other => up with that.
60: While it's usually OK to include basic markup within internationalized strings, here, you've got a string at the beginning of line 60 which is almost the same as the one on line 59 - this creates two entries which translators must add translations for. Try to minimize un-needed variations.
Line 60 has a line length of over 470 columns. Since you're using sprintf(), split the arguments on to individual lines:
'content' => '<h3>' . __( 'Additional Resources', 'startbox' ) . '</h3><p>' . sprintf(
__( 'For more information, try the %s or %s.', 'startbox' ),
'<a href="' . apply_filters( 'sb_theme_docs', 'http://docs.wpstartbox.com' ) . '" target="_blank">' . __( 'Theme Documentation', 'startbox') . '</a>',
'<a href="' . apply_filters( 'sb_theme_support', 'http://wpstartbox.com/support/' ) . '" target="_blank" >' . __( 'Support Forum', 'startbox' ) . '</a>'
) . '</p>',This still leaves lines of over 160 columns, so I'd then look at refactorising the link markup. You may want the links (with filtered URLs and internationalized link content) to be used elsewhere, so consider putting these into handy little functions - smaller code blocks also make it easy to see where values have not been escaped too:
/**
* Get markup for theme documentation link.
*
* The URL is filtered with 'sb_theme_docs' to allow pointing to non-English versions.
*
* @since 2.7.0
*
* @return string Anchor markup.
*/
function sb_get_theme_documentation_link() {
$url = apply_filters( 'sb_theme_docs', 'http://docs.wpstartbox.com/' )
return '<a href="' . esc_url( $url ) . '" target="_blank">' . __( 'Theme Documentation', 'startbox') . '</a>';
}
/**
* Get markup for theme support link.
*
* The URL is filtered with 'sb_theme_support' to allow pointing to non-English versions.
*
* @since 2.7.0
*
* @return string Anchor markup.
*/
function sb_get_theme_support_link() {
$url = apply_filters( 'sb_theme_support', 'http://wpstartbox.com/support/' )
return '<a href="' . esc_url( $url ) . '" target="_blank">' . __( 'Support Forum', 'startbox') . '</a>';
}Then:
'content' => '<h3>' . __( 'Additional Resources', 'startbox' ) . '</h3><p>' . sprintf(
__( 'For more information, try the %s or %s.', 'startbox' ),
sb_get_theme_documentation_link(),
sb_get_theme_support_link()
) . '</p>',...which I think is far easier to read.
67: Instead of nesting all of a loop within a conditional, consider checking for the opposite and using continue;:
foreach ( $settings as $setting ) {
if ( ! isset( $setting->description ) ) {
continue;
}
// ... now anything below here is known to have a description set, without having to be nested and indented.
}68: No need to assign an empty string when the next line immediately concats another string to it.
76 (and throughout): Avoid the style attribute at all costs. You're already enqueuing admin style sheet, so set a class here and move the CSS to .css. Developers can then enqueue their own style sheet which cascades over your styles, without having to resort to !important.
85-87: With the version compared removed / moved elsewhere, incorrect indentation fixed from line 49, and early return from line 67, there shouldn't be a need to comment } since there shouldn't be a collection of them on several consecutive lines. Adding comments like this doesn't actually make things clearer, since most developers would use an IDE than can highlight matching braces anyway, and it's really a code smell that there's something wrong with how the code is written.
91: No function-level documentation. As well as helping with a documentation generator (e.g. phpDocumentor), function-level documentation can also be used by IDEs - In NetBeans for instance, you can hold Ctrl and hover over a function call and it will show the documention as a tooltip without having to go find it in some obscure file.
104-105: Include version numbers on enqueued scripts, so that caches can be busted when a new version of SB / the script is released. Also, be explicit about dependencies. Presumably jquery-colorpicker depends on jquery, but this isn't given. Don't presume that just because other WP scripts have been enqueued just before, that someone hasn't dequeued them which means that jQuery might not have been referenced by the time it gets to your custom JS.
118: Another line with an inconsistent coding style, including multiple statements on one line. Consider using add_query_arg() to formulate the URL.
122: The first line of a DocBlock is a short description used to summarise what the function does, so leave credit and so on to a long description, or a @link tag.
133: Inconsistent spacing between functions.
140: The { should be the last character on a function foobar() { line - certainly not pulling global variables into scope then closing PHP.
145: Use get_admin_page_title() (uses the value from line 13) instead of redefining what the page title should be.
148: No need to compare something about being == true, since being true is what the conditional is all about. If you wanted to be specific about the value, then compare against a string, not a boolean.
153: Pull the PHP part of this line out to stand alone, and assign to a variable, then add that variable as part of the markup, to avoid multiple statements and logic on one line of what is basically a HTML view.
171: Comment suggests the postbox-container is closed here, but the div doesn't have a class of postbox-container.
186: More styles to be moved into the style sheet.
187-188: Use the submit_button() function for creating submit / reset buttons.
202: Use yoda conditions to avoid accidentally assigning a string to a variable instead of comparing against it.
210: Comment lines can be split into two or more lines to avoid long lines.
211: Only checking against a type of text may miss some of the HTML5 input types. Instead of checking against each string, create an array of types and use in_array() to check if this part of the conditional should be true.
212: Don't have ternary comparisons or other conditional logic as function call arguments as it makes the code harder to read. Do the logic, assign to a variable, then use that variable.
217: In an edge case, where there are no settings, or certain conditions fail, this line will throw a notice / warning as $inputs will not be defined.
219: Add a single blank line at the end of the file - it's what WP core files do, and PSR-2 coding style says to do.
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent
