Skip to content

Improve getKey method performance #136

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Feb 15, 2021
Merged

Improve getKey method performance #136

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
86949d6
Add getKey() return type as string
simPod Aug 19, 2020
8d8c6ec
Merge branch 'master' into improve_getKey_performance
drealecs Feb 12, 2021
0ad4363
make assertValidValue private as it's not valuable to extend the API.…
drealecs Feb 14, 2021
edcc10d
cache the key value at construct changing from in_array to array_search
drealecs Feb 14, 2021
4f2c572
move key property initialization in __wakeup in case an old version i…
drealecs Feb 15, 2021
b072dc2
re-add the public static method assertValidValue
drealecs Feb 15, 2021
be02f8d
optimize the from named constructor to have only one array full scan …
drealecs Feb 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions src/Enum.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ abstract class Enum implements \JsonSerializable
*/
protected $value;

/**
* Enum key, the constant name
*
* @var string
*/
private $key;

/**
* Store existing constants in a static cache per object.
*
Expand Down Expand Up @@ -61,7 +68,7 @@ public function __construct($value)
$value = $value->getValue();
}

static::assertValidValue($value);
$this->key = static::assertValidValue($value);

/** @psalm-var T */
$this->value = $value;
Expand Down Expand Up @@ -93,11 +100,10 @@ public function getValue()
* Returns the enum key (i.e. the constant name).
*
* @psalm-pure
* @return mixed
*/
public function getKey()
{
return static::search($this->value);
return $this->key ?? ($this->key = static::search($this->value));
}

/**
Expand Down Expand Up @@ -199,11 +205,13 @@ public static function isValid($value)
* @psalm-pure
* @psalm-assert T $value
*/
public static function assertValidValue($value): void
private static function assertValidValue($value): string
Copy link
Contributor Author

@drealecs drealecs Feb 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I propose to make private the newly public static method added in #135 as there was no new release so far.
I believe there is no need for have another public method as:

  • If a user wants to programmatically test that a value is valid, it can do this by using isValid() static method.
  • During instantiation, from() can be used to have an exception thrown for invalid values.

I want to easily re-use the method to return the key when a value is valid for further storing in constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelpetri, let me know if this makes sense and if you envision any other use-case that I missed

Copy link
Contributor Author

@drealecs drealecs Feb 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I worked around it for now but did a change in from to optimize the construction.

Copy link

Choose a reason for hiding this comment

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

Sorry for my late response. imo no need to have it public since i just can call from($mixed) and get an enum or exception.

Copy link

Choose a reason for hiding this comment

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

Sorry missed the return string part, imo the assert should not return anything. If no exception was thrown you know you have a valid key, or do i miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current version of the PR is with an additional private method called assertValidValueReturningKey() that returns string and having the current assertValidValue() still there calling it.

The operation does a full array scan and I would like to reuse the same array scan for retrieving the key in one operation, changing in_array with array_search.

assertValidValueReturningKey() would be called in ctor and in from() method as well.

Copy link

Choose a reason for hiding this comment

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

Okay, now I understand what you want to achieve. From my side it's ok, even if I think it makes the codebase a bit more confusing. Would another static map (value => key) be an option here as well? Then you could lookup keys by self::$otherMap[$value] ?? null ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would not work as values can be anything, not just int or string.

Copy link

Choose a reason for hiding this comment

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

Okay, so nothing more from my side. 🤐

{
if (!static::isValid($value)) {
if (false === ($key = static::search($value))) {
throw new \UnexpectedValueException("Value '$value' is not part of the enum " . static::class);
}

return $key;
}

/**
Expand All @@ -224,11 +232,11 @@ public static function isValidKey($key)
/**
* Return key for value
*
* @param $value
* @param mixed $value
*
* @psalm-param mixed $value
* @psalm-pure
* @return mixed
* @return string|false
*/
public static function search($value)
{
Expand Down
26 changes: 10 additions & 16 deletions tests/EnumTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@ public function testFailToCreateEnumWithInvalidValueThroughNamedConstructor($val
EnumFixture::from($value);
}

public function testFailToCreateEnumWithEnumItselfThroughNamedConstructor(): void
{
$this->expectException(\UnexpectedValueException::class);
$this->expectExceptionMessage("Value 'foo' is not part of the enum " . EnumFixture::class);

EnumFixture::from(EnumFixture::FOO());
Copy link
Contributor Author

@drealecs drealecs Feb 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I added one more test here to clearly mention from() should not work with the enum object itself, even if the exception text is unclear that 'foo' there is the actual object casted to string.

}

/**
* Contains values not existing in EnumFixture
* @return array
Expand Down Expand Up @@ -316,7 +324,8 @@ public function testSerialize()
{
// split string for Pretty CI: "Line exceeds 120 characters"
$bin = '4f3a33303a224d79434c6162735c54657374735c456e756d5c456e756d4669787'.
'4757265223a313a7b733a383a22002a0076616c7565223b733a333a22666f6f223b7d';
'4757265223a323a7b733a383a22002a0076616c7565223b733a333a22666f6f223b73'.
'3a32323a22004d79434c6162735c456e756d5c456e756d006b6579223b733a333a22464f4f223b7d';

$this->assertEquals($bin, bin2hex(serialize(EnumFixture::FOO())));
}
Expand Down Expand Up @@ -344,19 +353,4 @@ public function testEnumValuesInheritance()
$inheritedEnumFixture = InheritedEnumFixture::VALUE();
new EnumFixture($inheritedEnumFixture);
}

/**
* @dataProvider isValidProvider
*/
public function testAssertValidValue($value, $isValid): void
{
if (!$isValid) {
$this->expectException(\UnexpectedValueException::class);
$this->expectExceptionMessage("Value '$value' is not part of the enum " . EnumFixture::class);
}

EnumFixture::assertValidValue($value);

self::assertTrue(EnumFixture::isValid($value));
}
}