How can this big method be refactored?

Question

I wrote a class for badge activation and it should be refactored now. Are there any good advices, how should I refactor it's trigger() method?

The source of the class is in github: https://github.com/heal25/ced/blob/master/models/BadgeActivator.php

The problematic method:

public function trigger($id, $data = []) {
    if (!$this->_uid) $this->setUid(Yii::app()->player->model->uid); //set default uid

    $activate = false;

    switch ($id) {
        //case 'login_1': $activate = true; break;
        case 'max_nrg_35': if ($data['energy_max'] >= 35) $activate = true; break;
        case 'max_nrg_100': if ($data['energy_max'] >= 100) $activate = true; break;
        case 'skill_35': if ($data['skill'] >= 35) $activate = true; break;
        case 'skill_100': if ($data['skill'] >= 100) $activate = true; break;
        case 'strength_35': if ($data['strength'] >= 35) $activate = true; break;
        case 'strength_100': if ($data['strength'] >= 100) $activate = true; break;
        case 'energy_drink': $activate = true; break;
        case 'level_10': if ($data['level'] >= 10) $activate = true; break;
        case 'level_100': if ($data['level'] >= 100) $activate = true; break;
        case 'dollar_50': if ($data['dollar'] >= 50) $activate = true; break;
        case 'dollar_5000': if ($data['dollar'] >= 5000) $activate = true; break;

        case 'travel_loc3': if ($data['water_id'] == 3) $activate = true; break;
        case 'travel_county2': if ($data['county_id'] == 2) $activate = true; break;
        case 'travel_county9': if ($data['county_id'] == 9) $activate = true; break;
        case 'routine_100': if ($data['routine'] >= 100) $activate = true; break;
        case 'loc_routine_4b': if ($data['water_id']==4 and $data['routine'] > 0) $activate = true; break;
        case 'loc_routine_13s': if ($data['water_id']==13 and $data['routine'] >= 3) $activate = true; break;
        case 'loc_routine_28s': if ($data['water_id']==28 and $data['routine'] >= 3) $activate = true; break;
        case 'loc_routine_37g': if ($data['water_id']==37 and $data['routine'] >= 9) $activate = true; break;
        case 'loc_routine_52b': if ($data['water_id']==52 and $data['routine'] > 0) $activate = true; break;
        case 'loc_routine_61s': if ($data['water_id']==61 and $data['routine'] >= 3) $activate = true; break;
        case 'loc_routine_71g': if ($data['water_id']==71 and $data['routine'] >= 9) $activate = true; break;
        case 'loc_routine_72e': if ($data['water_id']==72 and $data['routine'] >= 27) $activate = true; break;
        case 'loc_routine_46d': if ($data['water_id']==46 and $data['routine'] >= 81) $activate = true; break;
        case 'setpart_3': if ($data['cnt'] >= 3) $activate = true; break;
        case 'setpart_10': if ($data['cnt'] >= 10) $activate = true; break;
        case 'setpart_30': if ($data['cnt'] >= 30) $activate = true; break;
        case 'first_duel_win': if ($data['role'] == 'caller' and $data['winner'] == 'caller') $activate = true; break;
        case 'duel_success_100': if ($data['cnt'] >= 100) $activate = true; break;
        case 'duel_fail_100': if ($data['cnt'] >= 100) $activate = true; break;
        case 'duel_rate_40': if ($this->getSuccessRate(100, $data) <= 40) $activate = true; break;
        case 'duel_rate_25': if ($this->getSuccessRate(300, $data) <= 25) $activate = true; break;
        case 'duel_rate_10': if ($this->getSuccessRate(600, $data) <= 10) $activate = true; break;
        case 'duel_rate_60': if ($this->getSuccessRate(100, $data) >= 60) $activate = true; break;
        case 'duel_rate_75': if ($this->getSuccessRate(300, $data) >= 75) $activate = true; break;
        case 'duel_rate_90': if ($this->getSuccessRate(900, $data) >= 90) $activate = true; break;
        case 'duel_money_100': if ($data['dollar'] >= 100) $activate = true; break;
        case 'duel_money_1000': if ($data['dollar'] >= 1000) $activate = true; break;
        case 'duel_win_chance35': if ($data['winner'] and $data['chance'] <= 35) $activate = true; break;
        case 'duel_win_chance20': if ($data['winner'] and $data['chance'] <= 20) $activate = true; break;
        case 'duel_win_chance5': if ($data['winner'] and $data['chance'] <= 5) $activate = true; break;
        case 'duel_lose_chance65': if (!$data['winner'] and $data['chance'] >= 65) $activate = true; break;
        case 'duel_lose_chance80': if (!$data['winner'] and $data['chance'] >= 80) $activate = true; break;
        case 'duel_lose_chance95': if (!$data['winner'] and $data['chance'] >= 95) $activate = true; break;
        case 'duel_2h': if ($data['role'] == 'caller' and date('G')==2) $activate = true; break;
        case 'shop_item10': if (Yii::app()->player->model->owned_items >= 10) $activate = true; break;
        case 'shop_bait20': if (Yii::app()->player->model->owned_baits >= 20) $activate = true; break;
        case 'set_b': if ($data['id']==1) $activate = true; break;
        case 'set_s': if ($data['id']==2) $activate = true; break;
        case 'set_g': if ($data['id']==3) $activate = true; break;
        case 'set_sell_b': if ($data['id']==1) $activate = true; break;
        case 'set_sell_s': if ($data['id']==2) $activate = true; break;
        case 'set_sell_g': if ($data['id']==3) $activate = true; break;
        case 'club_join': $activate = true; break;
        case 'club_create': $activate = true; break;
        case 'club_members_8': if ($data['cnt'] >= 8) $activate = true; break;
        case 'login_days_7': if ($this->getLoginDays() >= 7) $activate = true; break;
        case 'login_days_30': if ($this->getLoginDays() >= 30) $activate = true; break;
        case 'login_days_60': if ($this->getLoginDays() >= 60) $activate = true; break;
        case 'win_contest': $activate = true; break;
    }

    if ($activate) {
       return $this->activate($id);
    }
    return false;
}

I can extract the activations for 'loc_routine_*' cases, but it isn't much.

Answer 1

I'm not familiar with Yii, but from glancing over their docs, you could probably refactor the trigger method to make use of Yii Validators and the DI container. Something along these lines:

class BadgeActivator
…
    public function trigger($id, array $data)
    {
        $badgeValidator = $this->badgeValidators->findById($id);

        if ($badgeValidator->validate($data)) {
            $this->activate($id);
        }
    }
}

The BadgeValidators object is a simple collection of objects implementing the BadgeValidator interface. You need to inject this to your BadgeActivator.

class BadgeValidators
{
    private $validators = [];

    public function __construct(array $badgeValidators)
    {
        foreach ($badgeValidators as $badgeValidator) {
            $this->addBadgeValidator($badgeValidator);
        }
    }

    public function addBadgeValidator(BadgeValidator $badgeValidator)
    {
        $this->validators[$badgeValidator->getId()] = $badgeValidator;
    }

    public function findById($badgeId)
    {
        if (isset($this->validators[$badgeId])) {
            return $this->validators[$badgeId];
        }

        throw new BadgeValidatorNotFoundException(
            "No BadgeValidator found for badge id [$badgeId]"
        );
    }
}

The BadgeValidator interface is the contract all concrete validators need to obey. Their purpose is to map from $data to Yii's build in validators or to encapsulate additional/unique validation logic.:

interface BadgeValidator
{
    public function getId();
    public function validate($value = null);
}

A concrete Validator would then look like this:

class MaxEnergy35 implements BadgeValidator
{
    public function getId()
    {
        return 'max_nrg_35';
    }

    public function validate($value = null)
    {
        $validator = new yii\validators\IntegerValidator();
        $validator->min = 35;

        return $validator->validate($value['max_energy']);
    }
}

You could move the configuration for this class to the DI config as well, e.g. you could inject maxEnergy, validatorId and the Yii Validator. If you do that, you'll only have one class MaxEnergy instead of MaxEnergy35 and MaxEnergy100. For sake of simplicity, I'll keep it this way now.

Here is one with custom logic:

class DuelRate40 implements BadgeValidator
{
    public function getId()
    {
        return 'duel_rate_40';
    }

    public function validate()
    {
        return $this->getSuccessRate(100, $data) <= 40;
    }

    private function getSuccessRate($limit, array $data)
    {
        // moved from BadgeActivator
    }    
}

As you can see, it's trivial to make your BadgeValidators with custom logic and without using the Yii validators. In this particular case, I just moved the getSuccessRate rate method to the validator. You could easily see that it's misplaced on the BadgeActivator because it didn't have any cohesion to any class properties. Obviously, if you need this code in multiple validations, you could make it configurable again, so you have only one validator of that kind, instead of duplicating that method in each concrete one.

Now, for each case in your switch/case, simply add another concrete BadgeValidator and inject it to the BadgeValidators in the DIConfig.

In your ced/config/main.php:

// more config  …
components => [
    'badgeValidator' => [
        'class'=> 'application.components.BadgeValidator',
        'badgeValidators' => [
            ['class'=>'MaxEnergy35'],
            ['class'=>'MaxEnergy100'],
            ['class'=>'DuelRate40'],
            // more validators …
        ]
    ],
    // other components …
]

^{Note: I have no clue if this is the correct way to use Yii's DI config. I am just assuming from your file on GitHub. But I guess, you'll know how to change it if necessary.}

And whenever you need to add a new Badge, you simply write the Validator and wire it up in the config. You will never need to touch the BadgeActivator again. This will also significantly lower the Cyclomatic Complexity of the BadgeActivator. And it's trivial to add Unit Tests for each of the components above.