Coding guidelines: Difference between revisions
From LimeSurvey Manual
No edit summary |
m Text replacement - " enclose="div"" to "" |
||
(51 intermediate revisions by 10 users not shown) | |||
Line 1: | Line 1: | ||
__TOC__ | |||
NB: This will be replaced by the [[Code quality guide]]. | |||
=General= | =General= | ||
Line 14: | Line 16: | ||
*Keep a fluid dialog with the others coders, they have their own thoughts about what you are doing. | *Keep a fluid dialog with the others coders, they have their own thoughts about what you are doing. | ||
*Ask if you are in doubt. | *Ask if you are in doubt. | ||
=Documentation= | =Documentation= | ||
Please | Please document your source code - see this introduction: [[How to document your source code]]. | ||
=HTML= | =HTML= | ||
*As you will notice, a very customized HTML element can take a lot of lines (very expensive if you paid each one (:wink:) ) to avoid that, use 'CSS classes'. In general, use classes to aesthetic related stuff: color, borders, backgrounds, font faces, sizes, etc. | *As you will notice, a very customized HTML element can take a lot of lines (very expensive if you paid each one (:wink:) ) to avoid that, use 'CSS classes'. In general, use classes to aesthetic related stuff: color, borders, backgrounds, font faces, sizes, etc. | ||
*Respect W3C standards, and try to use the HTML understandable for all browsers. If one of them has a special feature, think if this feature is so special to break down the HTML in other browsers. | *Respect W3C standards, and try to use the HTML understandable for all browsers. If one of them has a special feature, think if this feature is so special to break down the HTML in other browsers. | ||
*Do not use PHP short tags <? ?>. Always use full PHP tags <?php ?> | *Do not use PHP short tags <? ?>. Always use full PHP tags <?php ?>. Excluded from this is the echo shorthand <?=. Which is allowed as per PSR standard. | ||
*Any rules of the mentioned before: about CSS, well closed HTML tags and JS respect XHTML standards. Give it a try!. | *Any rules of the mentioned before: about CSS, well closed HTML tags and JS respect XHTML standards. Give it a try!. | ||
=Localization= | =Localization= | ||
Moved to https://manual.limesurvey.org/Code_quality_guide#Localization | |||
=Bug tracker= | =Bug tracker= | ||
#First consider if the reported issue is really a bug. If it is a feature request please | #First consider if the reported issue is really a bug. If it is a feature request please move it to the feature tracker. | ||
#If you would like to work on a bug assign it to yourself. | #If you would like to work on a bug assign it to yourself. | ||
#Try to reproduce the issue. | #Try to reproduce the issue. | ||
Line 204: | Line 38: | ||
##If the user does not responds for more than a week ask once again for feedback by adding a comment. After a further week without feedback close the issue. | ##If the user does not responds for more than a week ask once again for feedback by adding a comment. After a further week without feedback close the issue. | ||
#After you resolved the issue set the status to resolved AND provide the 'Fixed in version' information. | #After you resolved the issue set the status to resolved AND provide the 'Fixed in version' information. | ||
#After you committed | #After you committed a fix and use the proper GIt commit message naming the commit will be automatically assigned to your bug report. | ||
#After a release the release technician will set all issues to 'Closed' that were fixed for this release. That signals to the reporter that a fix is available in the version and it was just released. | #After a release the release technician will set all issues to 'Closed' that were fixed for this release. That signals to the reporter that a fix is available in the version and it was just released. The release technicial will also add a comment to the close issue: 'Fixed in <Version> <Build number>' | ||
=Cross-DB compatibility= | =Cross-DB compatibility= | ||
Line 213: | Line 47: | ||
==Parameter binding== | ==Parameter binding== | ||
Yii/PDO supports parameter binding - use it. Don't inject variables directly into the query or into conditions. | Yii/PDO supports parameter binding - use it. Don't inject variables directly into the query or into conditions because this might create a security issue. | ||
Example: | Example: | ||
<syntaxhighlight lang="php | <syntaxhighlight lang="php"><?php | ||
$oResult = Yii::app()->db | $oResult = Yii::app()->db | ||
->createCommand() | ->createCommand() | ||
->select('somefield') | |||
->select(' | |||
->where("sid = :sid") | ->where("sid = :sid") | ||
->from('{{sometable}}') | |||
->from('{{ | |||
->bindParam(":sid", $surveyid, PDO::PARAM_INT); | ->bindParam(":sid", $surveyid, PDO::PARAM_INT); | ||
?></syntaxhighlight> | ?></syntaxhighlight> | ||
Line 237: | Line 64: | ||
WRONG: | WRONG: | ||
<syntaxhighlight lang="php | <syntaxhighlight lang="php"><?php | ||
$oResult = Yii::app()->db | $oResult = Yii::app()->db | ||
->createCommand() | ->createCommand() | ||
->select('somefield') | |||
->select(' | |||
->where("sid = :sid and parent_sid= :sid ") | ->where("sid = :sid and parent_sid= :sid ") | ||
->from('{{sometable}}') | |||
->from('{{ | |||
->bindParam(":sid", $surveyid, PDO::PARAM_INT); | ->bindParam(":sid", $surveyid, PDO::PARAM_INT); | ||
?></syntaxhighlight> | ?></syntaxhighlight> | ||
RIGHT: | RIGHT: | ||
<syntaxhighlight lang="php | <syntaxhighlight lang="php"><?php | ||
$oResult = Yii::app()->db | $oResult = Yii::app()->db | ||
->createCommand() | ->createCommand() | ||
->select('somefield') | |||
->where("sid = :sid1 and parent_sid= :sid2 ") | |||
->from('{{sometable}}') | |||
->bindParam(":sid1", $surveyid, PDO::PARAM_INT) | |||
->bindParam(":sid2", $surveyid, PDO::PARAM_INT); | |||
?></syntaxhighlight> | |||
==Quote column name== | |||
Some column name must be quoted for DB compatibility. You can use [https://www.yiiframework.com/doc/api/1.1/CDbConnection#quoteColumnName-detail App()->db->quoteColumnName]. | |||
Know column needed to be quoted : | |||
* Column name statring with number( column in survey database) | |||
* Reserved <code>key</code> word column name | |||
= | =Code complexity= | ||
*If using an auxiliary variable makes clear the code or process, just do it. If you are coding a very complex expression, calling to functions with parameters that are other functions: | *If using an auxiliary variable makes clear the code or process, just do it. If you are coding a very complex expression, calling to functions with parameters that are other functions: | ||
<syntaxhighlight lang="php | <syntaxhighlight lang="php"> | ||
WRONG: | WRONG: | ||
Line 286: | Line 107: | ||
iAuxB = fb(p1,p2) | iAuxB = fb(p1,p2) | ||
iAuxD = fd(p3) | iAuxD = fd(p3) | ||
iAuxC = fc(iAuxD,p4) | iAuxC = fc(iAuxD,p4) | ||
iAResult = fa(iAuxB, iAuxC, p5) | iAResult = fa(iAuxB, iAuxC, p5) | ||
Line 296: | Line 114: | ||
Anyway, if you are in doubt about someone missing the point or you are afraid that your expression is obscure, take the chance of writing it in a more simple way, even if that involves the use of one or more auxiliary variables. | Anyway, if you are in doubt about someone missing the point or you are afraid that your expression is obscure, take the chance of writing it in a more simple way, even if that involves the use of one or more auxiliary variables. | ||
=Checklist for new features= | |||
If you implement a new feature please make sure that you checked the following things before you create a pull request/commit your new feature: | |||
*'''Localization''': Do all texts (that are usually visible to the user) use the localization/translation system (gT, eT, etc.)? | |||
*'''Permissions''': Does the feature obey the permission system (global permissions, survey permissions)? | |||
*'''Backward-compatibility''': Does the feature still work when updating from any old version? Does it maybe break the update? | |||
*'''Database compatibility''': Does it work on all database types (MySQL, MSSQL, Postgres)? | |||
*'''Scaling''': Does your feature scale? For example; It will work fine with 10 surveys, does it still work performant with 10000? | |||
*'''Performance''': Does your feature use the minimal possible number of SQL queries (see also Scaling). | |||
*'''QA''': Did you test your code for all scenarios? Did you let someone else test or read your code? | |||
*'''Acceptance tests''': Did you write automatic acceptance tests for your specification? | |||
*'''Discussions''': Did you discuss the new feature with the team for feedback? | |||
Also see [https://manual.limesurvey.org/How_to_contribute_new_features How_to_contribute_new_features]. | |||
[[Category:Development]] |
Latest revision as of 15:06, 16 February 2022
NB: This will be replaced by the Code quality guide.
General
This is a free development, everything you do can be useful for others. If the feature is very personal there is a way to turn it into a universal solution. This is the challenge!
Some general rules:
- Be efficient and save resources (memory and runtime). Things might work for a small survey but also try out your new feature with a survey that has 400 question, 5 languages and 20,000 responses (Yes, there are surveys like this out there).
- Keep a fluid dialog with the others coders, they have their own thoughts about what you are doing.
- Ask if you are in doubt.
Documentation
Please document your source code - see this introduction: How to document your source code.
HTML
- As you will notice, a very customized HTML element can take a lot of lines (very expensive if you paid each one (:wink:) ) to avoid that, use 'CSS classes'. In general, use classes to aesthetic related stuff: color, borders, backgrounds, font faces, sizes, etc.
- Respect W3C standards, and try to use the HTML understandable for all browsers. If one of them has a special feature, think if this feature is so special to break down the HTML in other browsers.
- Do not use PHP short tags <? ?>. Always use full PHP tags <?php ?>. Excluded from this is the echo shorthand <?=. Which is allowed as per PSR standard.
- Any rules of the mentioned before: about CSS, well closed HTML tags and JS respect XHTML standards. Give it a try!.
Localization
Moved to https://manual.limesurvey.org/Code_quality_guide#Localization
Bug tracker
- First consider if the reported issue is really a bug. If it is a feature request please move it to the feature tracker.
- If you would like to work on a bug assign it to yourself.
- Try to reproduce the issue.
- If you cannot reproduce ask the user to provide a small example survey to demonstrate the issue. Then set the issue to 'Feedback'. After the user added the necessary information it will automatically be set to 'Assigned' again.
- If the user does not responds for more than a week ask once again for feedback by adding a comment. After a further week without feedback close the issue.
- After you resolved the issue set the status to resolved AND provide the 'Fixed in version' information.
- After you committed a fix and use the proper GIt commit message naming the commit will be automatically assigned to your bug report.
- After a release the release technician will set all issues to 'Closed' that were fixed for this release. That signals to the reporter that a fix is available in the version and it was just released. The release technicial will also add a comment to the close issue: 'Fixed in <Version> <Build number>'
Cross-DB compatibility
LimeSurvey targets several database types it can run on: MySQL, Postgres and Microsoft SQL Server. This demands certain precautions when coding
Parameter binding
Yii/PDO supports parameter binding - use it. Don't inject variables directly into the query or into conditions because this might create a security issue.
Example:
<?php
$oResult = Yii::app()->db
->createCommand()
->select('somefield')
->where("sid = :sid")
->from('{{sometable}}')
->bindParam(":sid", $surveyid, PDO::PARAM_INT);
?>
If you bind the same value several times do NOT use the same parameter name, instead use a different named parameter for each bind:
WRONG:
<?php
$oResult = Yii::app()->db
->createCommand()
->select('somefield')
->where("sid = :sid and parent_sid= :sid ")
->from('{{sometable}}')
->bindParam(":sid", $surveyid, PDO::PARAM_INT);
?>
RIGHT:
<?php
$oResult = Yii::app()->db
->createCommand()
->select('somefield')
->where("sid = :sid1 and parent_sid= :sid2 ")
->from('{{sometable}}')
->bindParam(":sid1", $surveyid, PDO::PARAM_INT)
->bindParam(":sid2", $surveyid, PDO::PARAM_INT);
?>
Quote column name
Some column name must be quoted for DB compatibility. You can use App()->db->quoteColumnName.
Know column needed to be quoted :
- Column name statring with number( column in survey database)
- Reserved
key
word column name
Code complexity
- If using an auxiliary variable makes clear the code or process, just do it. If you are coding a very complex expression, calling to functions with parameters that are other functions:
WRONG:
iAResult= fa(fb(p1,p2),fc(fd(p3),p4),p5)
It will be more readable:
iAuxB = fb(p1,p2)
iAuxD = fd(p3)
iAuxC = fc(iAuxD,p4)
iAResult = fa(iAuxB, iAuxC, p5)
Anyway, if you are in doubt about someone missing the point or you are afraid that your expression is obscure, take the chance of writing it in a more simple way, even if that involves the use of one or more auxiliary variables.
Checklist for new features
If you implement a new feature please make sure that you checked the following things before you create a pull request/commit your new feature:
- Localization: Do all texts (that are usually visible to the user) use the localization/translation system (gT, eT, etc.)?
- Permissions: Does the feature obey the permission system (global permissions, survey permissions)?
- Backward-compatibility: Does the feature still work when updating from any old version? Does it maybe break the update?
- Database compatibility: Does it work on all database types (MySQL, MSSQL, Postgres)?
- Scaling: Does your feature scale? For example; It will work fine with 10 surveys, does it still work performant with 10000?
- Performance: Does your feature use the minimal possible number of SQL queries (see also Scaling).
- QA: Did you test your code for all scenarios? Did you let someone else test or read your code?
- Acceptance tests: Did you write automatic acceptance tests for your specification?
- Discussions: Did you discuss the new feature with the team for feedback?
Also see How_to_contribute_new_features.