Actions

Coding guidelines: Difference between revisions

From LimeSurvey Manual

No edit summary
(36 intermediate revisions by 8 users not shown)
Line 1: Line 1:


Contents:__TOC__
__TOC__


=General=
=General=
Line 41: Line 41:
function questionsRetrieve( $iQID, $iSurvey)
function questionsRetrieve( $iQID, $iSurvey)
*What about files? Unless Yii requires it in some way always name your files in lowercase - this prevents problems on systems using case-aware filesystems
*What about files? Unless Yii requires it in some way always name your files in lowercase - this prevents problems on systems using case-aware filesystems
Here a list of prefixes:
*o=Object
*a=Array
*i=Integer
*f=Float
*s=String
*ar=ActiveRecord


=Documentation=
=Documentation=


Please refer to this introduction: [[How to document your source code]].
Please document your source code - see this introduction: [[How to document your source code]].


=Structures=
=Structures=
*Indent using spaces instead of TAB, or be gentle to announce how many spaces are your TABs long. There is wide ranging debate about tabs vs. spaces, many people prefer using tabs, others prefer spaces, others are happy with a mix of tabs and spaces. Because LimeSurvey is developed collaboratively, we ask that you leave your preference and follow this standard.
* Apply [https://www.php-fig.org/psr/psr-2/ PSR-2], or better, [https://www.php-fig.org/psr/psr-12/ PSR-12].
*The opening curly brace must be below the structure opening:
*Indent using steps of 4 spaces instead of TAB.
 
* Feel free to use [https://github.com/squizlabs/PHP_CodeSniffer codesniffer] and/or [https://phpmd.org/ mess detector] to detect syntax violations and simple bugs (like unused or undefined variables).
<syntaxhighlight lang="php" enclose="div">
 
if (condition)
 
{
 
   ...
 
}
 
</syntaxhighlight>
*Using one line for a simple condition is very "fashionable", a taste of "hacker", but it is not clear at all:
 
<syntaxhighlight lang="php" enclose="div">
 
 WRONG: if (condition) {do true} else {do false}
 
 WRONG: if (condition) {do true}
 
        else {do false}
 
 RIGHT: if (condition)
 
        {
 
           do true
 
        } else
 
        {
 
           do false
 
        }
 
 RIGHT: swith $sSelector
 
        {
 
              case X:
 
                   ...
 
                   break;
 
              case Y:
 
                   ...
 
              default:
 
        }
 
</syntaxhighlight>
 
NOTE: It&acute;s true that sometimes the "WRONG" way can be useful, but if it makes the straight reading difficult, please use the "RIGHT" way instead. Be gentle with your fellow developers.
*SQL sentences must respect the same rule:
*SQL sentences must respect the same rule:


Line 111: Line 63:


 $sSQL = "SELECT field1, field2, field3 "
 $sSQL = "SELECT field1, field2, field3 "
        ."WHERE condition "
        ."WHERE condition "
        ."AND condition "
        ."AND condition "
        ."OR condition "
        ."OR condition "
        ."ORDER BY field1, field2";
        ."ORDER BY field1, field2";
</syntaxhighlight>
</syntaxhighlight>
*The same for HTML in views tags:
*The same for HTML in views tags:


<syntaxhighlight lang="php" enclose="div">
<syntaxhighlight lang="php" enclose="div">
<table>
<table>
<tr>
<tr>
 
 <td colspan='2' height='4'>
 <td colspan='2'
 
     height='4'>
 
  <strong>
  <strong>
   Some Text
   Some Text
  </strong>
  </strong>
 </td>
 </td>
</tr>
</tr>
</table>
</table>


Line 150: Line 86:
*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!.


Line 158: Line 94:


Since LimeSurvey is available in 60 languages it is very important that your original English string (which will be picked up automatically for translation by our translators) is done right from the start. Imagine if we have to correct only one string at a later time - then 60 strings become invalid across the project and need to be re-translated by the poor translator souls.
Since LimeSurvey is available in 60 languages it is very important that your original English string (which will be picked up automatically for translation by our translators) is done right from the start. Imagine if we have to correct only one string at a later time - then 60 strings become invalid across the project and need to be re-translated by the poor translator souls.
* gT() is the original translation function. It will return a translated version of the string in the currently selected language. ie: echo ''$clang->gT("Hello");''
* gT() is the original translation function. It will return a translated version of the string in the currently selected language. ie: echo ''gT("Hello");''
* ngT() returns multiple translations of a sentence or phrase for which alternate plural forms may apply. ie: ''echo sprintf($clang->ngT('Please select at least %s answer','Please select at least %s answers',iMinimumAnswers),$iMinimumAnswers)'';
* ngT() returns multiple translations of a sentence or phrase for which alternate plural forms may apply. ie: ''echo sprintf(ngT('Please select at least %s answer','Please select at least %s answers',iMinimumAnswers),$iMinimumAnswers)'';
* eT() echos the translation directly. (ie: instead of ''echo $clang->gT("Hello");'' you can use ''$clang->eT("Hello");''
* eT() echos the translation directly. (ie: instead of ''echo gT("Hello");'' you can use ''eT("Hello");''
* neT() echos the multiple translations of the sentence or phrase for which alternate plural forms may apply directly.
* neT() echos the multiple translations of the sentence or phrase for which alternate plural forms may apply directly.


Line 166: Line 102:
*'''Do not embed margin spaces''' in your translation. Instead use proper CSS formatting
*'''Do not embed margin spaces''' in your translation. Instead use proper CSS formatting


<span style='color:#F00'>Wrong</span>: $clang->eT('Visible? ');
<span style='color:#F00'>Wrong</span>: eT('Visible? ');


<span style='color:#060'>Right</span>: $clang->eT('Visible?');
<span style='color:#060'>Right</span>: eT('Visible?');
*'''Do not capitalize words''' except where grammatically correct (like at the beginning of a sentence or for a brand name). LimeSurvey is not a newspaper.
*'''Do not capitalize words''' except where grammatically correct (like at the beginning of a sentence or for a brand name). LimeSurvey is not a newspaper.


<span style='color:#F00'>Wrong</span>: $clang->eT('Create A New Label Set');
<span style='color:#F00'>Wrong</span>: eT('Create A New Label Set');


<span style='color:#060'>Right</span>: $clang->eT('Create a new label set');
<span style='color:#060'>Right</span>: eT('Create a new label set');


<span style='color:#F00'>Wrong</span>: $clang->eT('Google Maps API Key');
<span style='color:#F00'>Wrong</span>: eT('Google Maps API Key');


<span style='color:#060'>Right</span>: $clang->eT('Google Maps API key');
<span style='color:#060'>Right</span>: eT('Google Maps API key');
*'''Do not concatenate''' several translations to form a sentence or concatenate to an additional information (like a number or string). Instead use the sprint() or sprintf() function with placeholders.
*'''Do not concatenate''' several translations to form a sentence or concatenate to an additional information (like a number or string). Instead use the sprint() or sprintf() function with placeholders.


<span style='color:#F00'>Wrong</span>: echo $clang->gT('The sum must not be bigger than').$iMaxSum;
<span style='color:#F00'>Wrong</span>: echo gT('The sum must not be bigger than').$iMaxSum;


<span style='color:#060'>Right</span>: echo sprintf($clang->gT('The sum must not be bigger than %s'),$iMaxSum);
<span style='color:#060'>Right</span>: echo sprintf(gT('The sum must not be bigger than %s'),$iMaxSum);


This comes from the problem that in other languages the setting of a sentence can be much different and the information $iMaxSum from the example above might be needed in the middle of the sentence.
This comes from the problem that in other languages the setting of a sentence can be much different and the information $iMaxSum from the example above might be needed in the middle of the sentence.


<span style='color:#F00'>Wrong</span>: echo $clang->gT('The user').$sUsername.$clang->gT('was deleted.');
<span style='color:#F00'>Wrong</span>: echo gT('The user').$sUsername.gT('was deleted.');


<span style='color:#060'>Right</span>: echo sprintf($clang->gT('The user %s was deleted.'),$sUsername);
<span style='color:#060'>Right</span>: echo sprintf(gT('The user %s was deleted.'),$sUsername);
*'''Use the n*T functions where applicable'''. These functions show a different translation depending the number of items. Currently LimeSurvey only supports the numbers/situations 1 and >1.
*'''Use the n*T functions where applicable'''. These functions show a different translation depending the number of items. Currently LimeSurvey only supports the numbers/situations 1 and >1.


+Example:
+Example:


<span style='color:#F00'>Wrong</span>: echo sprintf($clang->gT('Please select at least %s answer(s).'),$iMinimumAnswers);
<span style='color:#F00'>Wrong</span>: echo sprintf(gT('Please select at least %s answer(s).'),$iMinimumAnswers);


<span style='color:#060'>Right</span>: echo sprintf($clang->ngT('Please select at least %s answer','Please select at least %s answers',iMinimumAnswers),$iMinimumAnswers);
<span style='color:#060'>Right</span>: echo sprintf(ngT('Please select at least %s answer','Please select at least %s answers',iMinimumAnswers),$iMinimumAnswers);


=Bug tracker=
=Bug tracker=
#First consider if the reported issue is really a bug. If it is a feature request please point the reporter to http://ideas.limesurvey.org and set the issue status to 'Closed'.
#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 140:
##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 your fix add a comment to the resolved issue 'Fixed in rev. <Subversion revision number>'
#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=
 
LimeSurvey targets several database types it can run on: MySQL, Postgres and Microsoft SQL Server. This demands certain precautions when coding


=Miscellaneous=
==Parameter binding==
*Do not use "global" to access to a declared variable in another process. Pass variables or arrays by reference, use the session or the configuration object.
 
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:
 
<syntaxhighlight lang="php" enclose="div"><?php
$oResult = Yii::app()->db
                    ->createCommand()
                    ->select('somefield')
                    ->where("sid = :sid")
                    ->from('{{sometable}}')
                    ->bindParam(":sid", $surveyid, PDO::PARAM_INT);
?></syntaxhighlight>
 
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:
 
<syntaxhighlight lang="php" enclose="div"><?php
$oResult = Yii::app()->db
                    ->createCommand()
                    ->select('somefield')
                    ->where("sid = :sid and parent_sid= :sid ")
                    ->from('{{sometable}}')
                    ->bindParam(":sid", $surveyid, PDO::PARAM_INT);
?></syntaxhighlight>
 
RIGHT:
 
<syntaxhighlight lang="php" enclose="div"><?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);
?></syntaxhighlight>
 
=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:


Line 220: Line 199:


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 230: Line 206:


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]]

Revision as of 17:10, 22 November 2019

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!

"The difficulty within a solution is to solve it in a simple way" (Lance Burton, magician)

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.

Naming convention

  • Use the Hungarian Rule for variables:
[t]NameOfVariable

 | <strike>-</strike><strike>+</strike>----

 |        +--> Descriptive name

 +--> Datatype stored (for example start with "a" if it is an array)

Examples:

aQuestionAttributes: Array to store the questions attributes, Mixed values

dSurveyEnds: Date when the survey ends

sContditionsOutput: String used to create the JS for conditions

  • In case a function is declared do not use the Hungarian rule for the function name, instead use "Lower CamelCase":

function questionsRetrieve( $iQID, $iSurvey)

  • What about files? Unless Yii requires it in some way always name your files in lowercase - this prevents problems on systems using case-aware filesystems

Here a list of prefixes:

  • o=Object
  • a=Array
  • i=Integer
  • f=Float
  • s=String
  • ar=ActiveRecord

Documentation

Please document your source code - see this introduction: How to document your source code.

Structures

  • Apply PSR-2, or better, PSR-12.
  • Indent using steps of 4 spaces instead of TAB.
  • Feel free to use codesniffer and/or mess detector to detect syntax violations and simple bugs (like unused or undefined variables).
  • SQL sentences must respect the same rule:
 $sSQL = "SELECT field1, field2, field3 "
        ."WHERE condition "
        ."AND condition "
        ."OR condition "
        ."ORDER BY field1, field2";
  • The same for HTML in views tags:
<table>
<tr>
 <td colspan='2' height='4'>
  <strong>
   Some Text
  </strong>
 </td>
</tr>
</table>

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

Currently we are using the gT(),eT(),ngT() and neT() functions of the limesurvey_lang object for translations.

Since LimeSurvey is available in 60 languages it is very important that your original English string (which will be picked up automatically for translation by our translators) is done right from the start. Imagine if we have to correct only one string at a later time - then 60 strings become invalid across the project and need to be re-translated by the poor translator souls.

  • gT() is the original translation function. It will return a translated version of the string in the currently selected language. ie: echo gT("Hello");
  • ngT() returns multiple translations of a sentence or phrase for which alternate plural forms may apply. ie: echo sprintf(ngT('Please select at least %s answer','Please select at least %s answers',iMinimumAnswers),$iMinimumAnswers);
  • eT() echos the translation directly. (ie: instead of echo gT("Hello"); you can use eT("Hello");
  • neT() echos the multiple translations of the sentence or phrase for which alternate plural forms may apply directly.

Please follow these important rules:

  • Do not embed margin spaces in your translation. Instead use proper CSS formatting

Wrong: eT('Visible? ');

Right: eT('Visible?');

  • Do not capitalize words except where grammatically correct (like at the beginning of a sentence or for a brand name). LimeSurvey is not a newspaper.

Wrong: eT('Create A New Label Set');

Right: eT('Create a new label set');

Wrong: eT('Google Maps API Key');

Right: eT('Google Maps API key');

  • Do not concatenate several translations to form a sentence or concatenate to an additional information (like a number or string). Instead use the sprint() or sprintf() function with placeholders.

Wrong: echo gT('The sum must not be bigger than').$iMaxSum;

Right: echo sprintf(gT('The sum must not be bigger than %s'),$iMaxSum);

This comes from the problem that in other languages the setting of a sentence can be much different and the information $iMaxSum from the example above might be needed in the middle of the sentence.

Wrong: echo gT('The user').$sUsername.gT('was deleted.');

Right: echo sprintf(gT('The user %s was deleted.'),$sUsername);

  • Use the n*T functions where applicable. These functions show a different translation depending the number of items. Currently LimeSurvey only supports the numbers/situations 1 and >1.

+Example:

Wrong: echo sprintf(gT('Please select at least %s answer(s).'),$iMinimumAnswers);

Right: echo sprintf(ngT('Please select at least %s answer','Please select at least %s answers',iMinimumAnswers),$iMinimumAnswers);

Bug tracker

  1. First consider if the reported issue is really a bug. If it is a feature request please move it to the feature tracker.
  2. If you would like to work on a bug assign it to yourself.
  3. Try to reproduce the issue.
    1. 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.
    2. 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.
  4. After you resolved the issue set the status to resolved AND provide the 'Fixed in version' information.
  5. After you committed a fix and use the proper GIt commit message naming the commit will be automatically assigned to your bug report.
  6. 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);
?>

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.