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

Rewrote getOpenMageVersion() to be faster #3875

Merged
merged 4 commits into from
Apr 1, 2024
Merged

Rewrote getOpenMageVersion() to be faster #3875

merged 4 commits into from
Apr 1, 2024

Conversation

fballiano
Copy link
Contributor

The implode shouldn't be necessary, so I made a try rewriting the method in order to make it easier and a tiny bit faster

@github-actions github-actions bot added the Mage.php Relates to app/Mage.php label Mar 5, 2024
@kiatng
Copy link
Contributor

kiatng commented Mar 6, 2024

In my test, there is no significant diff. For one thousand iterations:

  1. original: ~ 250 microsec
  2. new: ~270 microsec; yes, it is worse

I suggest to close this PR.

app/Mage.php Outdated Show resolved Hide resolved
app/Mage.php Outdated Show resolved Hide resolved
app/Mage.php Outdated Show resolved Hide resolved
@fballiano
Copy link
Contributor Author

this new version doesn't use implode, doesn't use trim, has less variable assignations and from my measurements it's more stable (in terms speed measurements, while when there are functions calling the measurements bounce a bit faster/slower).

Copy link
Contributor

@Sdfendor Sdfendor left a comment

Choose a reason for hiding this comment

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

I like the cleaner structure and will approve. Just to mention it though, what do you think about a version with a switch instead:

public static function getOpenMageVersion(): string
{
    $info = self::getOpenMageVersionInfo();
    $versionString = "{$info['major']}.{$info['minor']}.{$info['patch']}";

    switch (true) {
        case ($info['stability'] && $info['number']):
            return "{$versionString}-{$info['stability']}.{$info['number']}";
        case ($info['stability']):
            return "{$versionString}-{$info['stability']}";
        case ($info['number']):
            return "{$versionString}-{$info['number']}";
        default:
            return $versionString;
    }
}

Does this improve readibility even further?

An additional benefit for the future (when - hopefully soon - PHP 8 becomes the minimum version) is, that we then can use match and transform the redundant returns into one return in front of the statement.

@fballiano
Copy link
Contributor Author

mmmmm I kinda don't like the "switch true" :-\\

@fballiano fballiano merged commit 3f07e73 into OpenMage:main Apr 1, 2024
17 checks passed
@fballiano fballiano deleted the fastergetinfo branch April 1, 2024 05:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mage.php Relates to app/Mage.php
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants