#CGL-decisions
##Decision process
1. summing up pros/cons
* on notes.typo3.org
1. discussions
* on the core-list
* on chat
* on notes.typo3.org
1. decision-making
* leave it up to the team leader to decide a proper way (depending on the relevance of the decision)
* formal voting (maybe tool based)
##Some items for decision
Why?
* consistency
* ability to search, replace, ...
* needs from tools (like phpdoc)
* help from tools (like phpstorm; might be able to adjust certain default-settings though)i
###Some rules of thumb
Whenever possible, use operators or language constructs instead of "real" functions to gain both, speed and memory.
***This example with the cast or strcmp is not clear, reading the rule I understand we should use cast but with other examples it looks like strcmp is preferred***
* Can you give examples, when strcmp would be the preferred solution? Actually it is meant to be used to sort strings, which is the ONLY case it might be preferable for.
* Examples:
* **(string)$foo === 'bar'**
* vs.
* **!strcmp($foo, 'bar')**
*
* **!empty($foo)**
* vs.
* **strlen($foo) !== 0**
* **attention this is not the same for '0', consider using !== ''**
* just an example that should of course be used in the right context :-)
* this is just meant to show: If you want to know if something is empty, you should use the right method
*
* **$string{0} (not good, PHP Warning if string is empty)**
* vs.
* **strlen($string) > 0**
* vs.
* $string !== '' (the one to go for)
*
**When you need read or write access to a single character from a string for reading or writing, use array access**
* Characters within strings may be accessed and modified by specifying the zero-based offset of the desired character after the string using square array brackets, as in $str{42}. Think of a string as an array of characters for this purpose. The functions substr() and substr\_replace() can be used when you want to extract or replace more than 1 character.
*
* Example:
* **strpos($string, ’n’) === 4**
* vs.
* **$string{4} === 'n'**
**Use pre increment operator when the result is not directly used**
* You can pre-increment a variable (++$count) or post increment it ($count++). The first one will return the value after adding one; the second will add the one after returning the current value.
* int x; int y;
* *// Increment operators*
* x = 1;
* y = ++x; *// x is now 2, y is also 2*
* y = x++; *// x is now 3, y is 2*
* *// Decrement operators*
* x = 3;
* y = x--; *// x is now 2, y is 3*
* y = --x; *// x is now 1, y is also 1*
*
*
* As long as you are not writing something like:
* **$something = $count++;**
* But are using the incrementor in a loop or on a single line, then use pre-increment:
* ++$count;
*
**Use TYPO3 defined constants instead of function calls wherever possible**
* Bootstrap defines the following:
* define('TAB', chr(9));
* define('LF', chr(10));
* define('CR', chr(13));
* define('CRLF', CR . LF);
* Find more in: \TYPO3\CMS\Core\Core\SystemEnvironmentBuilder::defineBaseConstants()
*
Avoid searching for or replacing strings whenever possible, especially when you actually don't want to use a string but a boolean or just check if a value is available or not.
**Avoid trimming values**
* I have seen some places where the match or the result of a regex result itself was trimmed: trim(preg\_replace()). If this is seen, the regex should be updated to include whitespace outside of the match brackets to avoid the trim operation.
**Avoid executing expensive functions or methods using a language construct check**
* The TypoScript parser uses the following line to check each line of TypoScript with an expensive regex!
*
* Example:
* **if (preg\_match('/^:=([^\\(]+)\\((.*)\\).*/', $line, $match)) {}**
*
* This call may be tuned in this case by first checking if the line starts with a colon before executing the regex:
* **if ($line{0} === ':' \&\& preg\_match('/^:=([^\\(]+)\\((.*)\\).*/', $line, $match)) {}**
**Move Language constructs to the front of comparison operator chain**
* If there are language constructs that decide if some action is going to get executed and the outcome of a function call is also used in deciding if something is going to happen, then move the language construct to the beginning of the chain. This increases the chance of the test returning false early without calling a function.
*
* Example:
* **if (is\_array($dirs) \&\& $recursivityLevels > 0) {}**
*
* This may be written as:
* **if ($recursivityLevels > 0 \&\& is\_array($dirs)) {}**
*
Avoid superfluous operators whenever possible
*
* Examples:
* **$foo = (string) $bar === (string) $foobar ? TRUE : FALSE**
* vs.
* **$foo = (string) $bar === (string) $foobar**
*
Avoid costly array functions when the same result can be achieved with less overhead
* Examples:
* **if (array\_key\_exists('key', $myArray))**
* vs.
* **if (isset($myArray['key'])**
* as long as you don't care about array keys with NULL values, which is true for most of the cases in the TYPO3 core
*
* **array\_unique($myOneDimensionalArray)**
* vs.
* **array\_flip(array\_flip($myOneDimensionalArray))**
* as long as you can be sure that the array is one dimensional only, otherwise array\_flip will throw a warning for values being an array themselves
*
* **in\_array($val, $yourArr)** // This will only work if the value key contains the value you want to test against. Code using in\_array should be checked for that
* vs.
* **isset( $yourArr[$val] )**
*
**Use the following methods to check if a string begins with another string:**
*
* This one for any needle that IS based on a variable
* **strpos($string, $part) === 0 // **This will throw a warning if $part is empty!
* **$part !== '' \&\& strpos($string, $part) === 0** // This will nog throw a warning if $part is empty
*
* This one for any needle that is NOT based on a variable
* **substr($string, 0, 3) === '123'**
*
* very easy to write and read in this example.
* but consider refactoring **GeneralUtility::*isFirstPartOfStr*($filename, 'RTEmagicC\_')** to what is proposed here without making off by one errors
* And is **substr($filename, 0, 9) === 'RTEmagicC\_' **really easier to read than **GeneralUtility::*isFirstPartOfStr*($filename, 'RTEmagicC\_')**
*
* As long as we are going to discuss "readability over performance" issues here, I guess the whole performance initiative is quite useless.
* It's been the "let's make things readable" behaviour that led us to the current state of the core.
*
* We simply can not refactor isFirstPartOfString to the two different methods, since this will implement another function call and a parameter, since we will have to decide if we "know" the needle is a variable or not. And besides that calling isFirstPartOfStr, would be like calling a function that calls another function. What is the overhead of that?
*
* In my tests, the readable strpos() comes out on top. If an empty string can be expected, an extra check is needed to avoid triggering a PHP warning.
* \url{http://pastebin.com/NtcxkCcv}
*
* If you know the key length you can even do:
* $string{0} === '1' \&\& $string{1} === '2' \&\& $string{2} === '3'
* But that's next to unreadable.
###Using count() / strlen() directly inside if
Alternative 1: if (count($a)) {
Alternative 2: if (count($a) > 0) { (always a boolean result for if-check)
(same with strlen())
* pros (for allowing count without additional comparison)
* short
* logic bound together in a spot where it belongs, easier to understand, even though more logic in packed into a single line.
* cons
* no boolean result for if-check
For performance reasons strlen() should be replaced with === '' unless a specific length is to be checked.
###Using long typenames instead of short ones
To be used in phpdoc, typecasts, ...
Alternative 1: integer, boolean, ...
Alternative 2: int, bool, ...
Currently both forms are used, which is inconsistent and should be cleaned up
* pros (for having long types)
* explicit
* more readable
* seems to be "native" type inside PHP (e.g. result of gettype())
* needed for tools like phdoc? --- needs to be checked, I only heard that once as a rumour
* cons
* shortnames are shorter
* in FLOW more usage of "int" than "integer" (according to Ernesto)
* var\_dump((integer) 1); returns "int"
* PHPdoc comments suggest int, bool and string \url{http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutorial\_tags.param.pkg.html}
* IDE support out-of-the-box
* less prone to typos
Since we have to rework intval() to (int) anway, it would be no big deal to take care of (integer) as well
* \url{http://forge.typo3.org/issues/54265}
**Currently we are working with (integer) for this patch as long as no one objects.**
**We should go for better readability just as we do at other places as well (variable names, file names, class names et al) and use more readable, long names here as well.**
The goal must be to have just THE ONE method for type casting, either (int) or (integer) but not both. Long names for improved readability sounds good.
(intval() should be used only for those cases, that make use of the "base" parameter of that function.)
See voting here: \url{http://doodle.com/b7ycp8y8g9zfxgfv} and \url{http://doodle.com/xm7wysupewg5gba9}
###Use short version ?: in ternary operator possible (Elvis operator)
available in PHP 5.3+
Example:
$result = ($value ?: $default);
* pros
* shorter
* easier to read
* less prone to typos
* cons
* risky code changes (logic changes)
* only works if condition part is the same as the true part
* ... any? ...
CGL rules here: \url{http://docs.typo3.org/TYPO3/CodingGuidelinesReference/PhpFileFormatting/PhpSyntaxFormatting/Index.html#conditions}
To be used only, when there are *exactly two *and not more possible outcomes.
\url{https://review.typo3.org/19007}
**Needs reviews**
Furthermore the example for the ternary operator has superfluous paratheses. Remove them.
$result =sout (sout$useComma ? ',' : '.'sout)sout;
$result = $useComma ? ',' : '.';
###Allow dots for multi-line string concatinations at the beginning of the new line as well.
Currently only dots in the end of the previous line is allowed. In conditions || and \&\& at the beginning of a new line is already accepted, hence allow it for dots as well.
###soutNo closing PHP-tags in files anymoresout
soutWas already decided, has accordingly been added in the CGL-document and implemented in the Core.sout
soutSee sout\url{southttp://forge.typo3.org/issues/50145sout}sout and sout\url{southttp://forge.typo3.org/issues/52360sout}
**soutDone.sout**
###soutCompare strings with === instead of strcmpsout
###soutsout
* soutprossout
* soutfastersout
* soutshortersout
* soutclear, because return value of strcmp is less obvioussout
* soutconssoutsout...sout
soutReworking has already been done - see sout\url{southttps://review.typo3.org/25843sout}
**soutDone.sout**