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

Most methods do not consider constants with same values. #65

Open
keithmifsud opened this issue Jan 29, 2018 · 6 comments
Open

Most methods do not consider constants with same values. #65

keithmifsud opened this issue Jan 29, 2018 · 6 comments

Comments

@keithmifsud
Copy link

keithmifsud commented Jan 29, 2018

Thanks for this library MyClabs :)

Not sure if this should be resolved or not. I find it problematic that when searching, the method does not consider that there may be several constants with same values. Example of an international dialling code enum:

const ITA = '39'; // this is fine and unique value.
const IMN = '44'; // UK and Isle of Man share the same code.
const GBR = '44'; // UK and Isle of Man share the same code.

Then when I instantiate the Enum like so:

        $testEnum = new CountryCallingCode(CountryCallingCode::GBR);
        var_dump($testEnum->getKey()); // return IMN not GBR!!
@keithmifsud keithmifsud changed the title Search() method does not consider constants with same values. Most methods do not consider constants with same values. Jan 29, 2018
@mnapoli
Copy link
Member

mnapoli commented Feb 12, 2018

👍, I think supporting multiple identical values in the same enum is problematic. I'm not sure we could enforce it strictly without loosing in performances though 🤔

@keithmifsud
Copy link
Author

If it was to be done, how would you go about it? I'm asking so I can understand what you mean in performance loss.

@keithmifsud
Copy link
Author

I encountered this issue in several domains including foreign exchange rates, foreign exchange currency codes and other more custom ones such as that a status can be different but the value the same. Example of "LeadStatus" enum:

const ACQUIRED = "Open";
const PENDING _QUALIFICATION= "Open";
const QUALIFIED = "Closed";
const NOT_QUALIFIED = "Closed";

@mnapoli
Copy link
Member

mnapoli commented Feb 12, 2018

Oh OK, maybe it's not so uncommon to have duplicate values… I did not think about it much.

I was thinking about ensuring that there are no duplicate values, but that's costly to do. I'm not sure there is much we can do here :/

@keithmifsud
Copy link
Author

I understand. Enums can have same value even in C language :)

But again this is a matter of how one uses Enums. The LeadStatus enum example can be changed so that the values do meet their name but cases like I had with the country calling codes cannot be refactored.

I also tried using another library from MabeEnum and it also produces the same unexpected result. Therefore, for a demo https://github.com/keithmifsud/laravel-ddd-demo/blob/master/src/Domain/Member/PhoneNumber/InternationalDialingCode.php I ended up querying by the value which is obviously very bad practice because I'm not really getting an Enum Constant but simply matching the first constant of similar value.

I guess we'll have to develop a new Enum library or ditch Enms in PHP. I any case, I would at least disallow instantiating Enums by value since this will break the dev's code when there are identical values.

What do you think? I think that if an Enum is allowed to have different constants with same value then wither disallow this or disallowing instantiating/querying by value since it will return the first constant and not the actual one.

@danielbeardsley
Copy link
Contributor

It seems quite odd to me to treat an Enum as a map. Generally, an enum is a human-readable and type-safe wrapper around some set of distinct values.

You situation seems better suited to using a map between two enums:

$map = [
  LeadStatus::AQUIRED => Status::OPEN,
  ...
]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants