dseguy / clearphp Goto Github PK

Maybe mention you don't want to put sleep in web facing code specifically, but it's ok in CLI stuff.

https://github.com/dseguy/clearPHP/blob/master/rules/no-sleep.md

Sleep can be pretty handy in big cron jobs when you're doing batch imports and dont want to utterly flood your database.

Maybe add the uniqid function without entropy ?

for($i = 0; $i < 500; $i++) uniqid();
// 0.29069590568542

for($i = 0; $i < 500; $i++) uniqid('', true);
// 0.0015909671783447

We shouldn't encourage people to include a closing php by including it in the examples. This is because if someone writes a php class and then puts a new line character after the closing tag, php will send the response - not the desired behaviour at all!

Not entirely sure what the point of this is.

https://github.com/dseguy/clearPHP/blob/master/rules/no-eval.md#rule-details

It doesn't really explain what is happening, and if its good or bad.

Also you say:

Such code has to be systematically sanitized before it is used.

It's impossible to sanitize eval from user input securely.

http://stackoverflow.com/questions/5922762/eval-base64-decode-php-virus

http://stackoverflow.com/a/3697776

Best to just NEVER put user input anywhere near eval.

Just before the first code sample we have (https://github.com/dseguy/clearPHP/blob/master/rules/avoid-redefining-properties.md#avoid-redefining-properties)
Check the code below : $lock is defined in a class
But there's no $lock variable. And nothing about vehicle too,

Things like https://github.com/vlucas/phpdotenv are a great solution to this sort of problem, and it's a framework agnostic (or no framework) solution. Laravel builds on top of it, as do others no doubt.

Currently are all examples, pattern without space between brackets and var.
I think is also a good practice to write readable code.
Maybe I would check all examples for this and send a pull request with the changes - if you like it?

Example:

if (!$foo) {

much better

if ( ! $foo ) {

Also I would add a chapter about space, whitespace in code for better legibility, if you like?

Is there one tool (maybe tools) that detects all your /rules?

This is just bad advice.

1.) Indentation here is messed up.

$array = array('a', 
                  'b',
                 );
$array2 = ['c', 
             'd',
            ];

What is going on there? :)

2.) You have a copy paste error in The following pattern is considered legit:

$array = array('a', 
                  'b'
                 );

$array2 = ['c', 'd', ];

I guess that $array2 was supposed to have the , removed?

Either way, trailing commas on multi-line array definitions are awesome. They are also supported in pretty much every language I've used, but are banned in JSON.

They are a style guide choice for sure, but a) they don't hurt at all, and b) they reduce friction and potential conflicts in diffs. I've seen a LOT of conflicts caused by just this, and since asking developers on various teams to use trailing diffs those problems have gone away.

I'd like to see this one deleted entirely.

The No Unused Property rule is clearly copied from the No Unused Variables rule, but both the text and the examples are still the same.

In other words, the No Unused Property rule is invalid at this moment in time.

On the Commented Fallthrough page, the "warnings" and "legit" examples should be reversed.

In https://github.com/dseguy/clearPHP/blob/master/rules/imported-collision.md#rule-details block
last code sample with $a,$b,$c,$d,$e,$f is obviously not considered warnings

Hello. The page "No Uninitialized Variables"
https://github.com/dseguy/clearPHP/blob/master/rules/no-uninitialized-variable.md
is very similar to "No Recalculate" page
https://github.com/dseguy/clearPHP/blob/master/rules/no-recalculate.md
Probably, copy-paste error?

How should someone e.g. remove in_array() with isset() when the first function operates on values, while the other could only check for a key. You might advice someone trying to use the isset(), but it's not really a possible replacement. Same is true of diff/intersect on arrays.

Replacing array_walk() and array_map() with a foreach-by-reference could cause some side effects as mentioned in the PHP Pitfalls document you linked to. Although the foreach-by-reference is a lot faster, the usage of the other functions is necessary if you use clean and readable functional programming techniques.

I'd recommend to alter the recommendation to always sort by key rather than an array_reverse.

Example: https://3v4l.org/scmZ8

When going through everything, I noticed that these two are essentially the same. If they are not (supposed to be), then maybe the difference between the two rules should be made clearer.
If they are, I suggest merging them into one rule.

https://github.com/dseguy/clearPHP/blob/master/rules/no-buried-assignation.md
https://github.com/dseguy/clearPHP/blob/master/rules/no-implied-if.md

The title in the document is wrong, however, the rule is missing one important point: Interfaces

Members of an interface are per language specification always public and any other visibility makes no sense. Hence, requiring the visibility to be defined is redundant and (imho) should actually be avoided for clarity.

Scrutinizer

avoid-those-slow-functions.md
array_search function duplicate in table

While you have linked to Nikita's article Disproving the Single Quotes Performance Myth, your advice is still incredibly generalized and not entirely accurate.

I wrote this up on PHP The Right Way: Strings

Scroll down to this bit:

Which is quicker?
There is a myth floating around that single quote strings are fractionally quicker than double quote strings. This is fundamentally not true.

If you are defining a single string and not trying to concatenate values or anything complicated, then either a single or double quoted string will be entirely identical. Neither are quicker.

If you are concatenating multiple strings of any type, or interpolate values into a double quoted string, then the results can vary. If you are working with a small number of values, concatenation is minutely faster. With a lot of values, interpolating is minutely faster.

Regardless of what you are doing with strings, none of the types will ever have any noticeable impact on your application. Trying to rewrite code to use one or the other is always an exercise in futility, so avoid this micro- optimization unless you really understand the meaning and impact of the differences.

In the result $e is false, $f is true.
But it's the opposite:

$ php -r '$e = false || true; var_dump($e);'
bool(true)
$ php -r '$f = false or true; var_dump($f);'
bool(false)

In https://github.com/dseguy/clearPHP/blob/master/rules/use-smart-autoload.md example:

function my_autoloader($class) {
include 'classes/' . $class . '.class.php';
}

For every include that fail, will generate a warning and increase your log. If you have 4 functions (methods,..) registered using include directly, will create 4 warnings for every request.

It is worse with require, that create a fatal error and stop the execution.

It 's a shame than spl_autoload_register don't work like include_path, that try every path and show the error if all fail.