avoiding type switching

Question

If you're in a team and a programmer gives you an interface with create, read, update and delete methods, how do you avoid type switching?

Quoting Clean Code A Handbook of Agile Software Craftsmanship:

public Money calculatePay(Employee e)
    throws InvalidEmployeeType {
        switch (e.type) {
            case COMMISSIONED:
                return calculateCommissionedPay(e);
            case HOURLY:
                return calculateHourlyPay(e);
            case SALARIED:
                return calculateSalariedPay(e);
            default:
                throw new InvalidEmployeeType(e.type);
    }
}

There are several problems with this function. First, it’s large, and when new employee types are added, it will grow. Second, it very clearly does more than one thing. Third, it violates the Single Responsibility Principle7 (SRP) because there is more than one reason for it to change. Fourth, it violates the Open Closed Principle8 (OCP) because it must change whenever new types are added. But possibly the worst problem with this function is that there are an unlimited number of other functions that will have the same structure. For example we could have

isPayday(Employee e, Date date),

or

deliverPay(Employee e, Money pay),

or a host of others. All of which would have the same deleterious structure.

The book tells me to use the Factory Pattern, but in way that it makes me feel that I shouldn't really use it.

Quoting the book again:

The solution to this problem (see Listing 3-5) is to bury the switch statement in the basement of an ABSTRACT FACTORY,9 and never let anyone see it.

Is the switch statement ugly?

Answer 1

In reality, the employee object should have its own calculate pay function that will give you the pay. This calculate pay function would change based on what type of employee it was.

That way it is up to the object to define the implementation, not the user of the object.

abstract class Employee
{
     public abstract function calculatePay();
}

class HourlyEmployee extends Employee
{
     public function calculatePay()
     {
          return $this->hour * $this->pay_rate;
     }
}

class SalariedEmployee extends Employee
{
     public function calculatePay()
     {
          return $this->monthly_pay_rate;
     }
}

When you build the Factory, THEN you do the switch statement there, and only once, to build the employee.

Lets say Employee was in an array, and the type of employee was held in $array['Type']

public function buildEmployee($array)
{
    switch($array['Type']){
       case 'Hourly':
            return new HourlyEmployee($array);
            break;
       case 'Salaried':
            return new SalariedEmployee($array);
            break;
}

Finally, to calculate the pay

$employee->calculatePay();

Now, there is no need for more than one switch statement to calculate the pay of the employee based on what type of employee they are. It is just a part of the employee object.

Disclaimer, I'm a minor, so I'm not completely positive on how some of these pays are calculated. But the base of the argument is still valid. The pay should be calculated in the object.

Disclaimer 2, This is PHP Code. But once again, the argument should be valid for any language.

Answer 2

I read it somewhere, that if you're using a switch, then it's suspect that there's too much variation. And when we have too much variation, we should try to encapsulate the variation behind an interface, thereby decoupling the dependencies between objects. Having said that, I think that you should try to create an SalaryType lightweight base class object that will encapsulate this type of logic. Then you make it a member of class Employee and rid yourself of the switch construct. Here's what I mean in a nutshell:

abstract class SalaryType
{
   function calculatePay() {}
}

class CommissionedType extends SalaryType
{
   function calculatePay() {}    
}

class HourlyType extends SalaryType
{
   function calculatePay() {}    
}

class SalaryType extends SalaryType
{
   function calculatePay() {}    
}

class Employee
{
  private $salaryType;

  public function setType( SalaryType emp )
  {
     $this->salaryType = emp;
  }

  public function calculatePay()
  {
     $this->salaryType->calculatePay();
  }
}

Btw, a lot of your example code does not seem very "PHP-ish". There are no return types in PHP nor is there really any type safety. Keep in mind also that PHP is not truly polymorphic, so some of the polymorphic behavior found in typical type-safe languages may not work as expected here.

Answer 3

You can totally remove the switch by using a Map of some kind to map the type of an employee to it's corresponding pay calculator. This depends on reflection and is possible in all languages I know.

Assuming the pay calculation is not a responsibility of an employee, we have an interface PayCalculation:

interface PayCalculation {
    function calculatePay(Employee $employee);
}

There's an implementation for each category of employee:

class SalariedPayCalculator implements PayCalculation {
    public function calculatePay(SalariedEmployee $employee) {
        return $employee.getSalary();
    }
}

class HourlyPayCalculator implements PayCalculation {
    public function calculatePay(HourlyEmployee $employee) {
        return $employee.getHourlyRate() * e.getHoursWorked();
    }
}

class CommissionedPayCalculator implements PayCalculation {
    public function calculatePay(CommissionedEmployee $employee) {
        return $employee.getCommissionRate() * $employee.getUnits();
    }
}

And the pay calculation would work something like this. Reflection becomes important for this to look at an object and determine it's class at run-time. With this, the switch loop can be eliminated.

public class EmployeePayCalculator implements PayCalculation {

    private $map = array();

    public function __construct() {
        $this->map['SalariedEmployee'] = new SalariedPayCalculator();
        $this->map['HourlyEmployee'] = new HourlyPayCalculator();
        $this->map['CommissionedEmployee'] = new CommissionedPayCalculator();
    }

    public function calculatePay(Employee $employee) {
        $employeeType = get_class($employee);
        $calculator = $this->map[$employeeType];
        return $calculator->calculatePay($employee);
    }
}

Here we are initializing the map in the constructor, but it can easily be moved outside to an XML configuration file or some database:

<payCalculation>
    <category>
        <type>Hourly</type>
        <payCalculator>HourlyPayCalculator</payCalculator>
    </category>
    <category>
        <type>Salaried</type>
        <payCalculator>SalariedPayCalculator</payCalculator>
    </category>
    ...
</payCalculation>