Code quality guide
From LimeSurvey Manual
DRAFT
Prologue
- Be risk aware. Too perfect code can be a business risk (slow to write, maybe over-designed). Too sloppy code can also be a business risk (hard to maintain and understand). You have to find a balance that is adapted to the current situation and risk analysis, in which code quality becomes a risk mitigation technique.
- Be humble. LimeSurvey was made by developers from all over the world, with different age, education and experience. Your code might be read by a completely different team ten years from now, in the same way you are now reading code by developers that no longer work with us, but which work pays our rent.
- Performance matters sometimes, and shouldn't be disregarded completely. In particular, database queries using the ORM and ActiveRecord can be problematic. Some surveys have thousands of questions and hundreds of thousands of responses. Fast response time is also important for a fluid user experience.
- It's harder to read code than to write code. Don't choose patterns that are easy or fast to write, but that are easy to read.
- It's easy to forget cross-cutting concerns like translation and security. Keep a mental note.
- Stress affects code quality. If your stress level is too high to write code with proper quality, take a step back and discuss it with your boss.
Quality
What is quality? Which aspects of quality can be measured?
It's usually easier to get along what is "bad" code than what's "best" code. Blacklist instead of whitelist?
Quality attributes related to code quality:
- Readability
- Testability
- Maintainability
- Performance
- Security
It's better to avoid emotional language when describing code quality, like "clean" or "dirty". Be precise.
Idiomatic code is more readable than non-idiomatic code. What's idiomatic depends on which context or domain you work in. We work in PHP and web application development, and have other idioms than in, say, functional or hardware-close programming.
Some code are "hot spots" - changed often. Those parts should require higher quality than other code.
Risk
todo
Modules
There's no inherit module functionality in PHP, but Yii has a module system.
Classes
todo
Different categories of classes:
- Builders
- Fetchers
- Data-transfer object
- ...
Functions
Functions are one of the fundamental building blocks of programming, so it's important to get it right.
Functions that are part of a class are called "methods", if you want to be formal.
Different types of functions in PHP:
- Functions
- Methods
- Anonymous functions
- Short functions
Size
- A function should be short enough to be completely readable on one screen
- A function should not have too many arguments; if you have five or more, consider make it a class instead
- If a function is growing too big, it might be better to create a class instead
Contract
- A function has a contract with its surrounding environment:
- Pre-condition: What needs to be true before the function is executed
- Post-condition: What will be true after the function has returned
- The function contract can be checked with assertions (internal invariants) or exceptions (outside world); see #assertions below
- The first line in the docblock can be one sentence describing the relation between function arguments and its output
Example/** * Creates a random password of length $length (defaults to 12) * @param int $length * @return string */ function createPassword($lenght = 12)
Function name
- The function name should tell you what it does; docs should tell you why. How is described by the code itself (hopefully, but can in some cases be relevant to document).
- Function name can often be verb + noun, like "createFieldmap" or "quoteText"
todo: difference between create, get, fetch
Assertions
Assertions can be used for mental checks. They are only used for internal logic - failed assertions are not supposed to be seen by the end user. For interaction with the outside world, like database or browser, exceptions should be used instead.
Assertions can be disabled in production mode and have then no runtime cost.
Example:
/**
* Translation helper function
*
* @param string $sToTranslate
* @param string $sEscapeMode Valid values are html (this is the default, js and unescaped)
* @param string $sLanguage
* @return string
*/
function gT($sToTranslate, $sEscapeMode = 'html', $sLanguage = null)
{
assert(is_string($sToTranslate));
assert($sEscapeMode === 'html' || $sEscapeMode === 'js' || $sEscapeMode === 'unescaped');
assert(strlen($sLanguage) === 2 || $sLanguage === null);
return quoteText(Yii::t('', $sToTranslate, array(), null, $sLanguage), $sEscapeMode);
}
If-statements
If-statements should not be too long or hard to understand. If they are, consider factor out the checks into separate functions.
Variables
Descriptive naming.
- Variables or ActiveRecord classes can have same name as the class, like "$survey" or "$question"
- Add -s for plural when it's an array: "$surveys", "$questions"
Not so good:
$result = LabelSet::model()->findAll();
Better with model name and -s for plural:
$labelSets = LabelSet::model()->findAll();
Looping
The bodies of foreach-loops are often a good candidate to factor out into separate functions.
Example:
foreach ($questions as $question) {
processQuestion($question);
}
Loop indexing should be named i, j, k etc.
Example:
for ($i = 0; $i < $length; $i++) {
for ($j = 0; $j < $otherLength; $j++) {
// ...
}
}
Cases
/**
* This function generates an array containing the fieldcode, and matching data in the same order as the activate script
*
* @param Survey $survey Survey ActiveRecord model
* @param string $style 'short' (default) or 'full' - full creates extra information like default values
* @param boolean $force_refresh - Forces to really refresh the array, not just take the session copy
* @param bool|int $questionid Limit to a certain qid only (for question preview) - default is false
* @param string $sLanguage The language to use
* @param array $aDuplicateQIDs
* @return array
*/
function createFieldMap($survey, $style = 'short', $force_refresh = false, $questionid = false, $sLanguage = '', &$aDuplicateQIDs = array())
Good:
- Name is verb and noun
- Each argument is documented (almost)
Bad:
- Returns "array", which can be anything. A separate class for "Fieldmap" would be better.
- Not clear if it should be "createFieldMap" or "createFieldmap" - is "fieldmap" one word or two?
- Very many boolean arguments. Should probably be a class with methods like
$fieldmapCreator->setForceRefresh(true);
instead. - Too long.
- Doesn't elaborate on what "fieldmap" really is, and does not link to any documentation about it.
- Access super-global, which makes it hard to test. Should instead return a data-transfer object, like:
$_SESSION['fieldmap'] = createFieldMap(...);
- Has no test
- Mixes Hungarian notation with non-Hungarian notation in the argument list
Defensive programming and error handling
todo
Documentation
todo
OOP design
todo
Object reification.
Hierarchy depth.
The PHP of yesterday
Old idioms and habits that should be abandoned.
Assoc arrays vs DTO. No performance overhead of objects compared to arrays.
Objects are already passed by reference; arrays are value types
Security
todo
XSS
Database injection
Permissions and roles
Performance
todo
Testing
todo
QA
PHPUnit
TDD
Mocks, can't mock functions, static methods, mocking as a DSL, PHPUnit mocking tools
Pure functions, referential transparency, side-effects
Unit, functional, integration
Don't test the framework; don't test PHP
Static analysis
php -l
, CodeSniffer, Mess Detector, Psalm
Resources
todo
Books, links, videos