Elao / PhpEnums

:nut_and_bolt: Extended PHP 8.1+ enums features & specific integrations with frameworks and libraries
MIT License
326 stars 27 forks source link

Design issue: Unnnecessary split of values() and readables() #46

Closed ostrolucky closed 6 years ago

ostrolucky commented 6 years ago
class myEnum {
  function values() {
    return ['value1', 'value2'];
  }

  function readables() {
    return ['value1' => 'label1', 'value2' => 'label2'];
  }
}

Do you see where is the problem? Why not just do

class myEnum {
  function values() {
    return ['label1' => 'value1', 'label2' => 'value2'];
  }
}

In our case, almost all of the enums are designed to be read by FormTypes and this unnecessarily complicates the work. Of course, I solved it by proceeding to use this proposed alternative format anyway and created own basetype which doest just this.

abstract class AbstractEnum extends ReadableEnum
{
    function readables()
    {
        return array_flip(static::values());
    }
}

But, this is just a workaround for design issue in this library, hence won't look good in eyes of other developers who are not so keen of OSS libs when they can create their own (reinvent the wheel). I'm pushing for more involvement in OSS. I spent way too much effort on searching for enum library capable enough to replace our internal implementation everlutionsk/enum-bundle. During this process, I got burned by several others. elao/phpenums seem closest. So, let's improve it?

To figure out how to proceed, I first need to know your view on this issue. I also need to know a reason for this split. Is there some good reason for this separation?

ogizanagi commented 6 years ago

Glad to have you here!

Actually, the reason is simple: not all enums need/are meant to be readable. What you've spotted as a design flaw would to others be interfaces segregation. Readable enums are a separated feature and the EnumInterface::values() method was never meant to return anything else than values. Indexes are not relevant here and despite PHP doesn't allow yet to document/typehint this properly, the actual return value of this method should (but not enforced) be a set, not a dict.

However, I admit this is a DX issue, as you have to keep synchronized two methods. So I'm all in for providing a solution for this. But I won't rely on values. I'm for keeping clear separation for the most "low-level" classes and allow to compose higher-level, more opinionated classes. The AutoDiscoveredValuesTrait was already a step in this way and I'm happy about it.

So, here is my suggestion for this issue. I hope you'll be satisfied this way :)

ostrolucky commented 6 years ago

Still not sure about value of enum which must not be readable, was hoping you will provide some use case. Yes, enum doesn't need to be readable, but if it is just as a bonus, I don't think it's harmful in any way. But ok, let's reverse the dependency in my example then. How about

abstract class AbstractEnum extends ReadableEnum
{
    function values()
    {
        return array_keys(static::readables());
    }
}

This way, you still can have non-readable enums, but if you implement ReadableEnum, you get values() automatically inferred from that