Streamline implementation details for performance and readability #8
Conversation
|
On random note then: If you say this is not about performance, why not say in the commit message/whatnot this is about readability etc? |
thiemowmde
changed the title
|
Better? |
| @@ -11,10 +11,10 @@ class BasicJsExpressionParser implements JsExpressionParser { | |||
| */ | |||
| public function parse( $expression ) { | |||
| $expression = $this->normalizeExpression( $expression ); | |||
| if ( strpos( $expression, '!' ) === 0 ) { // ! operator application | |||
| if ( strncmp( $expression, '!', 1 ) === 0 ) { | |||
There was a problem hiding this comment.
Oh, by the way, an other perfectly valid and equally fast possibility is substr( $expression, 0, 1 ) === '!'. Just recently we discussed this in the MediaWiki CodeSniffer project, and preferred substr over strncmp, which might be a little obscure.
|
Thanks for the contribution. I am not going to have time to review this PR this week (most likely neither will I the week after this one), as there are more time-pressing things on my table now. Same most likely applies to other Wikidata developers currently. |
|
Sure. Since this is technically part of the Lexeme project, I would love to see this reviewed and merged before the project ends. But since what is done in this patch is more code cleanup than anything, there is no reason to rush. |
| @@ -171,7 +171,7 @@ private function replaceMustacheVariables( DOMNode $node, array $data ) { | |||
| private function handleAttributeBinding( DOMElement $node, array $data ) { | |||
| /** @var DOMAttr $attribute */ | |||
| foreach ( iterator_to_array( $node->attributes ) as $attribute ) { | |||
| if ( !preg_match( '/^:[\-\_\w]+$/', $attribute->name ) ) { | |||
There was a problem hiding this comment.
Can you explain why you removed _ here?
There was a problem hiding this comment.
The \w character class already contains the underscore.
|
Looks fine, but one thing I don't understand (see inline comments). |
I did not actually benchmarked these. The actual performance gain might be negligible. However, I still believe it is worth applying these optimizations. The motivation is not only to make this code (potentially) faster, but also simpler and more easy to read. For example, the goal of the comparison
count() === 0is not to actually count the number of elements, but to check if it's empty. This becomes much more obvious withempty()or=== [].