How to use the Open/Closed Principle to replace switch block that modifies shared state

Question

I am working on a project that requires me to generate a report based on purchased rentals. I have to keep a count for each type of rental and sum the corresponding totals. Currently I am using a switch block to determine which action to take based on the current rental. However, it is my understanding that this violates the Open/Closed Principle, as I will have to modify the switch block every time a new rental is added. I would like to make this meet the OCP, but I am not sure how to go about. Below is my code:

public function generateReport($rentals)
{
    $type_1_count = 0;
    $type_2_count = 0;
    $type_3_count = 0;

    $type_1_sum = 0;
    $type_2_sum = 0;
    $type_3_sum = 0;

    foreach ($rentals as $rental) {
        switch ($rental->type) {
            case 'TYPE_1':
                $type_1_count++;
                $type_1_sum += $rental->price;
                break;
            case 'TYPE_2':
                $type_2_count++;
                $type_2_sum += $rental->price;
                break;
            case 'TYPE_3':
                // some of the rentals include other rentals which must be accounted for
                $type_1_count++;
                $type_1_sum += $rental->price / 2;

                $type_3_count++;
                $type_3_sum += $rental->price / 2;
                break;
            default:
                echo 'Rental Not Identified';
        }
    }
    return compact('type_1_count', 'type_2_count', 'type_3_count', 'type_1_sum', 'type_2_sum', 'type_3_sum');
}

I am modifying the shared state variables depending on the selected case. I reviewed many OCP examples but all of them show how to execute an action or return a value, but I need to modify the shared state instead. What would be the best way to refactor this code to be more inline with the OCP?

score 0 · Accepted Answer · answered Jan 29 '20 at 00:58

0

You can use an associative array. You just need to make sure the variable is set first. Something like this:

   $all_rentals = [];
   foreach ($rentals as $rental) {
       // make sure the variable has a default value
       if(!isset($all_rentals[$rental->type."_count"]) {
           $all_rentals[$rental->type."_count"] = 0;
           $all_rentals[$rental->type."_sum"] = 0;
       }
       $all_rentals[$rental->type."_count"]++;
       $all_rentals[$rental->type."_sum"] += $rental->price;
   }

...

This way you can add new values (rental types) without having to modify any existing code

answered Jan 29 '20 at 00:58

Michael Peng

806
9
11

Thank you for your response, it is very close to what I was looking for. My only remaining question is what to do if I need to perform different actions depending on the rental type? For example Type 3 rentals are handled slightly different from the others in the way they are counted and summed. Do I still need the switch statement or is there some way I can use polymorphism? – yCombinator Jan 29 '20 at 15:35
Are the calculations always different? If that's the case then a switch case is probably best unless you want multiple "IF" conditions. – Michael Peng Jan 29 '20 at 16:05

How to use the Open/Closed Principle to replace switch block that modifies shared state

1 Answers1