Wednesday, September 3, 2008

Flag Object

I've worked with a lot of programmers that write if bugs, so I've come up with small simple solutions. The simplest solution I've come up with so far for "if bugs" is what I call the Flag Object.

The Algorithm

  1. Instead of making a boolean, make an object.

  2. Take the if clause from the client code and make it a method on the object.

  3. In the client code call the method on the object.
I said it was simple.

An Example

The Old Code
public class Employee {
private boolean manager;
public boolean isManager() {
  return manager;
}
...
}

...
Employee employee = new Employee();
if (employee.isManager()) {
// calculate manager pay
} else {
// calculate employee pay
}
...

This is a potential "if bug". The following is better and only slightly more 'work':
public class Employee {
 private PayScale payscale;
 public double calculatePay() {
   payscale.calculatePay();
 }
...
}

public interface PayScale {
 double calculatePay();
}

public EmployeePayScale implements PayScale {
 public double calculatePay() {
   // calculate employee pay
 }
}

public ManagerPayScale implements PayScale {
 public double calculatePay() {
   // calculate manager pay
 }
}

...
Employee employee = new Employee()
double pay = employee.calculatePay()
..


If you were doing the old boolean flag trick, you'd have places in the code to set the boolean true or false.
employee.setManager(true);

Now you call something like:
employee.setPayScale(new ManagerPayScale());

Now the details of how a manager's and employee's pay is calculated is encapsulated in the PayScale objects. How pay is calculated can change and not affect anyone who actually uses the code to calculate pay. You can also add new employee types and not have to change existing code. That means the code satisfies the Open-Closed Principle.

There a few things I don't like. One, the object that calls
employee.setPayScale()
depends on the low level objects in the PayScale hierarchy. This breaks the Dependency-Inversion Principle that says concrete objects should depend on abstractions. Preferably the employee object is obtained from some kind of factory that knows how to make Employee objects already configured with the appropriate PayScale object. However, it's better than the "if bug" problem so I can live with a little dependency issue. At least the dependency issue is localized and easily solved. If Bugs can be spread throughout the code and finding all of them very very difficult. I also prefer Flag Objects because they conform to Tell Don't Ask.

5 comments:

andrewsw said...

okay, that all makes sense, and I completely agree with what you're trying to do. The potential downfall is that you can't know in advance all the possible reasons that someone might want to access isManager() and every one of them potentially involves significant overhead like that exhibited in the payroll example. I'm not arguing against the principle, just pointing out that the amount of "extra" work could get horrendous in a really complicated system. But I'm pretty much a n00b at this...

Meanwhile, with the if_bug itself... hmmm... I wonder what the effect of powerful IDE's is on this issue. Right now, with Eclipse, it would be pretty easy to *find* all the instances of isManager() and work how to fix that code. So that's a good thing. But it also seems the ease of refactoring the code with Eclipse makes it super easy to just use the isManager() idiom without really thinking about it. And refactoring existing isManager() code becomes so easy that you *really* don't have to think about it and that *is* a problem for sure.

Curtis Cooley said...

In the example, all you know about managers is that their pay is calculated differently than regular employees. There are a lot of ways to implement it. I showed two, one bad and one slightly better.

As manager gets more and more 'different' from employee, you then make the decisions on how to best deal with each change.

The problem with the "if bug" is all the places that should have the if but don't. No IDE or tool can help you find those.

andrewsw said...

d'oh. My bad. I had inverted the meaning of the if bug. And of course you're right.

I look forward to more. ;)

mschober said...

Encapsulating the calculatePay() method using the interface allows additional pay formats to be added without changing existing code. Code is added that fulfills the interface for a new pay format. No switch statements! It is a good technique.

Bill said...

Why would you get a boolean at all, rather than have the knowledge built into the parent class?

One solution would be to have the two classes derive from one class, and each has a different salary calculation routine. This is probably not great unless there are a lot of code differences between managers and other employees.

A better bet might be to have the employee class have a getSalary() method that is delegated to a Salary class contained within the employee class.

This way you can replace the salary calculation routine easily. This is pretty much just encapsulating the Flag class inside your Employee class and only exposing the salary() method.