Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Small performance optimization #555

Conversation

laurieplo
Copy link
Contributor

Q A
Branch grumpy-seventies
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Documented? no
Fixed tickets #482

A lot of very small changes that will affect performance and maintanability (fully-qualified function calls, ===/!==, return optimization, ...)

@veewee veewee requested a review from Landerstraeten October 25, 2018 05:57
@veewee veewee added this to the grumpy-seventies milestone Oct 25, 2018
@@ -156,7 +156,7 @@ private function enforceTextWidth(ContextInterface $context): TaskResult
}
}

if (count($errors) > 0) {
if (\count($errors) > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to if (\count($errors) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -58,12 +58,12 @@ public function run(ContextInterface $context): TaskResultInterface
$extensions = $config['triggered_by'];

$files = $context->getFiles();
if (0 !== count($whitelistPatterns)) {
if (0 !== \count($whitelistPatterns)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to if (\count($whitelistPatterns)) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -54,10 +54,10 @@ public function run(ContextInterface $context): TaskResultInterface

/** @var FilesCollection $files */
$files = $context->getFiles()->name($extensions);
if (count($whitelistPatterns) >= 1) {
if (\count($whitelistPatterns) >= 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to if (\count($whitelistPatterns)) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -70,12 +70,12 @@ public function run(ContextInterface $context): TaskResultInterface

/** @var \GrumPHP\Collection\FilesCollection $files */
$files = $context->getFiles();
if (0 !== count($whitelistPatterns)) {
if (0 !== \count($whitelistPatterns)) {
Copy link
Contributor

@Landerstraeten Landerstraeten Oct 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to if (\count($whitelistPatterns)) {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@laurieplo laurieplo force-pushed the small-performance-optimization branch from fc9bf08 to 359738a Compare October 25, 2018 18:38
@@ -33,7 +33,7 @@ public function __construct(string $string)
private function isRegex(string $string): bool
{
if (preg_match('/^(.{3,}?)['.self::ALLOWED_MODIFIERS.']*$/', $string, $m)) {
$start = substr($m[1], 0, 1);
$start = $m[1][0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use a string function here to avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -45,7 +45,7 @@ private function formatOutput(string $output): string
{
$result = static::RESET_COLOR;
foreach (array_filter(explode("\n", $output)) as $lineNumber => $line) {
$result .= preg_match('/^[0-9]+/', $line) ? $this->trimOutputLine($line, (int) $lineNumber) : $line;
$result .= preg_match('/^\d+/', $line) ? $this->trimOutputLine($line, (int) $lineNumber) : $line;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer [0-9] here since it is easier to folow then the \d alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@laurieplo laurieplo force-pushed the small-performance-optimization branch 2 times, most recently from 1952e7e to 76c5b93 Compare October 28, 2018 21:33
@Landerstraeten
Copy link
Contributor

Hi @laurieplo, Thanks for your effort. Can you fix the failing tests/build?

@laurieplo laurieplo force-pushed the small-performance-optimization branch from 76c5b93 to 170fb07 Compare October 30, 2018 10:59
@laurieplo
Copy link
Contributor Author

laurieplo commented Oct 30, 2018

Hi @laurieplo, Thanks for your effort. Can you fix the failing tests/build?

Yes, sorry @Landerstraeten ! It is fixed.

Copy link
Contributor

@Landerstraeten Landerstraeten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @laurieplo

@Landerstraeten Landerstraeten force-pushed the small-performance-optimization branch from 170fb07 to f43f215 Compare November 30, 2018 12:22
@Landerstraeten Landerstraeten merged commit 92ce312 into phpro:grumpy-seventies Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants