G
G
Gogeo2018-02-14 00:15:02
Refactoring
Gogeo, 2018-02-14 00:15:02

Array vs Switch case, code style best practice?

Good day.
I got something like this code in the project

public funciton getModelByTypeAndId($type, $id) {
switch ($name) {
            case 'type0':
                $offer = $this->em->getRepository(Type0::class)->find($id);
                break;
            case 'type1':
                $offer = $this->em->getRepository(Type1::class)->find($id);
                break;
            case 'type2':
                $offer = $this->em->getRepository(Type2::class)->find($id);
                break;
            case 'newType':
                $offer = $this->em->getRepository(NewType::class)->find($id);
                break;
            case 'newType2':
                $offer = $this->em->getRepository(NewType2::class)->find($id);
                break;
            case 'model':
                $offer = $this->em->getRepository(Model::class)->find($id);
                break;
            default:
                $offer = null;
        }
return $offer;
}

And there are a lot of such projects. And I don't like it, it's cumbersome and duplicative. And so I decided to
refactor
public funciton getModelByTypeAndId($type, $id) {
$classes = [
            'type0' => Type0::class,
            'type1' => Type1::class,
            'type2' => Type2::class,
            'newType' => NewType::class,
            'newType2' => NewType2::class,
            'NewType3' => NewType3::class,
        ];

        if (empty($classes[$name])) {
            return null;
        }

        return $this->em->getRepository($classes[$name])->find($id);
}

Intuition tells you that something is not right)
This option seems to be easier to read, but the one that I gave above is the most concise
public funciton getModelByTypeAndId($type, $id) {
        switch ($type) {
            case 'auto' : $entity = Type0::class; break;
            case 'type1' : $entity = Type1::class; break;
            case 'type2' : $entity = Type2::class; break;
            case 'newType' : $entity = NewType::class; break;
            case 'newType2': $entity = NewType2::class; break;
            case 'newType3': $entity = NewType3::class; break;
            default : return null;
        }
        return $this->em->getRepository($entity)->find($id);
}

Perhaps in general the very logic of this method is not optimal from the point of view of OOP. Due to the fact that the frontend (where this request comes from) is not designed in the best way. In general, please advise how it is more beautiful and semantically correct to act in such cases?
Maybe instead of default : return null; throw an Exception, and put the wired strings ('type1','newType' ...) into the classes themselves, so that they are class aliases, and, for example, make all of them a static getAlias ​​method and some kind of interface (IEntityWithAlias ​​for example), i.e. e. Something like this
public funciton getModelByTypeAndId($type, $id) : IEntityWithAlias {
switch ($type) {
            case Type0::getAlias() : $entity = Type0::class; break;
            case Type1::getAlias() : $entity = Type1::class; break;
            //и т.д.
            default : throw new InvalidArgumentException('Unknown category type');
        }
return $this->em->getRepository($entity)->find($id);

Or another such option, but it has a triple nesting
$classes = [
            Type1::class,
            Type2::class,
            Type3::class,
            NewType1::class,
            ///и т.д.
        ];

        foreach ($classes as $class) {
            if ($class::getAlias() === $name) {
                return $this->em->getRepository($class)->find($id);
            }
        }
        throw new InvalidArgumentException('Unknown category type');

Well, the last question. Where do I get information from when I ask similar questions, until it comes with experience itself, and what should I do to get it?)

Answer the question

In order to leave comments, you need to log in

1 answer(s)
O
OnYourLips, 2018-03-21
@OnYourLips

https://en.wikipedia.org/wiki/Open_Principle/z...
The second option is close to what you want, but it has an explicit enumeration, which you need to get rid of.
Make a setter (type, class) in this method, and in your service configuration, inject classes with the corresponding types there.

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question