We all have came across to a point when we used to say who reviewed this piece of code and though of why this wasn’t pointed. Well, we all learn from our mistakes but what about someone who just started working or is just started climbing up the ladder, its hard to define how code reviews are done and what a good code looks like. Reviewing someone’s code and making it better takes a lot of experience and efforts but we can define a starting point to that long road in few pointers.
This blog deals with helping out everyone from the developer to the one who is reviewing code, this document will help with a basic to high-level standard that should be followed but the best decision/responsibility still resides with the one reviewing as to see the practicality of the implementation and use.
General Checks :
-
Check the naming of functions, PHP classes, files, services, constants, etc. The name should be concise and clear. The name should clearly define what it does or contains or used for.
-
Duplicate or very similar, code parts are problematic and should either be made re-usable or removed from the file.
-
Code comments should be checked, that it’s not copied & paste with irrelevant pieces and that it is meaningful and provides value for the reader of the source code.
-
Check for whitespaces in every file, if un-required should be removed to increase code visibility and cleanliness.
-
Commented out code should be removed in almost every case, Git history is there to look for the previous state if needed.
-
Check for unused variables and functions, rules, files, and classes.
-
Check for hard-coded parts and validate if they need to be configurable or not.
-
Errors of any kind should be handled properly and logged.
-
Validate the base and destination branch of the Pull request as per the defined workflow.
-
Check the parent of the Pull request, it should always be one of the base branches and not sub-branched from another Pull request unless dependent.
-
Constants should always be all-uppercase, with underscores to separate words. (This includes pre-defined PHP constants like
TRUE,FALSE, andNULL.). Module-defined constant names should also be prefixed by an uppercase spelling of the module that defines them. -
In case of global variables, their name should start with a single underscore followed by the module/theme name and another underscore.
-
Always use
<?php ?>to delimit PHP code, not the shorthand,<? ?>. -
Check to have a single space after a comma
,is used. -
Always use a space between the dot and the concatenated parts to improve readability
$string = $bar . 'foo';
-
Check that arrays should always be formatted as, one property per line :
$form['title'] = [
'#type' => 'textfield',
'#title' => t('Title'),
'#size' => 60,
'#maxlength' => 128,
'#description' => t('The title of your node.'),
];
-
Anonymous functions should have a space between "function" and its parameters, as :
array_map(function ($item) use ($id) {
return $item[$id];
}, $items);
2. Drupal Code Standards :
-
Indentation & Whitespace :
-
Use an indent of
two spacesand no tab. -
Files should be formatted with
\nor a new line as the line ending. -
Add a line before and after of
namespaceandusestatements. -
For a non-class file add
@fileannotation as a first element after opening php tag and add a new line and before and after the comment block.<?php
/**
* @file
* Provides example functionality.
*/
use Drupal\foo\Bar;
-
Put a space between the (type) and the $variable in a cast:
(int) $mynumber -
Control statements should have one space between the control keyword and opening parenthesis, to distinguish them from function calls, also add an empty line or comment before starting the control statements and at the end of control statements.
.../............some line of code.....
if (condition1 || condition2) {
action1;
}
elseif (condition3 && condition4) {
action;
}
else {
defaultaction;
}
...../............some line of code....
OR
.../..........some line of code.....
// this is a comment desc the if statement.
if (condition1 || condition2) {
action1;
}
elseif (condition3 && condition4) {
action2;
}
else {
defaultaction;
}
...../............some line of code....
The conditional statements should be split as :
if (condition) {
//....
}
-
-
Operators :
-
All binary operators (operators that come between two values), such as
+,-,=,!=,==,>, etc. should have a space before and after the operator, for readability. -
Checks for weak-typed inequality MUST use the
!=operator. The<>operator MUST NOT be used in PHP code.
-
-
Line Length & Wrapping :
-
The number of characters in a line of code should not be longer than 80 characters.
-
Conditions should not be wrapped into multiple lines instead of putting the complete condition in a single line, this will even be application for function arguments while defining functions or using them.
-
-
Class/Method Visibility :
-
Always define a visibility property when creating a new method, class or interface.
-
Only define “public” if you are sure of it to be re-used at other places but try to limit its use and try to use private or protected more often.
-
While calling a defined function in the same class, try calling it using the $this property, or make the property static if that makes sense.
-
If a property or field_name is being used multiple time in the same file, define it as a constant and use constants to help in increasing overall code coverage.
-
-
Contributed Modules :
-
When using a contributed module, install the module using compose and add .json and .lock file
-
When creating a new functionality try to use Contributed module wherever possible as the first approach for solutionism.
-
When applying a patch to a module, it's preferred to download the patch and put it into the
Patchdirectory and define its path inside the compose.json file, along with the actual path of the patch in the patch description in composer.json -
For most cases try to define a specific version of the module needed instead of just relying on and updating to the latest version (avoid using ^), an un-noticed update to a module can break things up and hard to debug the source the issue.
-
-
Custom Modules/functionality :
-
Check for security issues, SQL injections and XSS. SQL injection can come from un-sanitised user input, directly appended to SQL query. XSS typically come from un-sanitised user input sent to the output without filtering
-
Check for hard-coded paths, hard-coded URLs and other hard-coded parameters and verify if that can be replaced with a configuration option for better coverage and ease.
-
Check for proper usage of the Drupal API. If there is an API function, class or service for the purpose, custom code should not be written, rather the code should rely on the existing solution.
-
Check that the use of alter hook as the last solution to achieving the end goal and its use should be limited and only for critical scenarios.
-
If your custom class or module file is using code that can be generalised, where ever possible move it to create it as a service, so as to make it easily reusable.
-
Create Drush script only if it is made generic and parameters are placed while calling the script otherwise for a one time use try to use hook_UPDATE_N for one-time configuration changes/updates or when the number of affected nodes is greater than 20k.
-
Check for validation of data forms. Validation should happen when accepting data from the user, typically handled by the Drupal API.
-
While creating a new functionality use subscriber pattern so other functionality can alter and use its functionality in longer run if required.
-
When creating a new module, the module should contains a readme.md file & Changelog.txt file and is updated on every change in that module.
-
Do performance-related checks, to determine if something seems to be a resource-heavy operation. If so, question it; check if it's cache-able.
-
Validate the code for deprecation and if its optimal for use, for example using entityTypeManager() instead of entityManager() to load any entity.
-
Validate if appropriate caching mechanism is used in its implementation.
-
Validate that appropriate annotation and doc blocks are placed where ever required. The parameters included and return type of every custom method is documented in its doc block.
-
-
JavaScript :
-
Check the use of the Javascript API: Behaviors, settings, context is used properly.
-
Check Javascript code format
-
Excessive amount of custom, project-specific JavaScript code is a “code smell,” for a typical Drupal project. Most functionality should be solved in PHP or using a JavaScript-based library.
-
Check that errors of any kind are handled properly (e.g., unsuccessful HTTP requests).
-