Technical Practices: Switch and Else always evil
We've covered why we don't use getters. This reason is the fundamental principle driving all of Object Oriented Programming.
Encapsulate Behavior.
Encapsulating behavior allows changes to that behavior with a guarantee it is changed across the system.
We need to encapsulate the behavior.
The switch
statement and the use of the else
statement are constructs which control behavior.
These make the decision of what to do based on some criteria. "What to do" is behavior. This should be encapsulated into an object. It should be encapsulated into the object which has the data.
Let's look at a super contrived example using the following interface:
public interface IToggle{
bool ShouldBeOn();
void ToggleOn();
void ToggleOff();
}
and now a use of this
public void ProcessToggle(IToggle toggle){
if(me.ShouldBeOn()){
me.ToggleOn();
} else {
me.ToggleOff();
}
}
Let's assume that there's No Getter in the class. ShouldBeOn
does something that's not returning a reference we hold.
The class abides by No Getters. This is why we need more constraints. No single practice gets us the amazing code.
The simplest form of getting rid of switch
and else
statements is when the switch
of if
and the action are off of the same object.
In the above example, we're using the methods of the ProcessToggle
interface to make a decision, then execute a behavior based on that.
The object should handle it. All of the information is internal to the object, the object is guaranteed to be able to handle all of the process.
This is where the challenge of this style Object Oriented Programming comes in - It's a vastly different way to think. We're used to looking at software at bits of data flying around and invoking or writing the behavior around that. Maybe it's our brain, maybe it's growing pains of our industry stemming from the early days where it was much more fundamentally all data.
This isn't where we are now. We can write good object oriented code. We can encapsulate all of our behavior to make code more maintainable.
What should our ProcessToggle
method look like? I'll always assume it made sense for the engineer(s) writing it at the time of writing. When I come across code, anything I need to think about to figure out what it does or what it means - That's an opportunity to refactor the name.
This may not seem like an important step in getting rid of switch
and else
, renaming a method we're talking about getting rid of. It's huge though - How do we move this functionality into the object? Will IToggleMe
get a ProcessToggle
method? In this case, ProcessToggle
doesnt' tell me what the method is doing. What would it mean as part of IToggle
, IToggle#ProcessToggle
. If we look at the code, it's clear what this does, we're abstract though. We shouldn't need the code to understand what the contract does.
We refactor.
What's the method doing? Simplified, and what I find more informative is that it's updating the state, IToggle#UpdateState
. How or why, we no longer need to know anything about the Toggle-ness of the object.
Our ProcessToggle
method ends up being a passthrough when we move the functionality into the object. This method represented behavior that belonged as part of the object. How many places would this snippet exist?
Our new contract and code looks like
public interface IToggle{
void UpdateState()
}
...
toggle.UpdateState();
...
There's method around toggle.UpdateState()
as our original passthrough is removed in favor of invoking the behavior on the object.
We still have the if-else
in our single object, which isn't good. It's better though. Our behavior is now encapsulated into an object. If we had multiple behavior decisions throughout the code, which databags REQUIRE, then we'll have that many methods with if
or switch
statements. We'll get to fixing that, they're always evil. Sometimes worse is better. Pull in all the evil to a single location, easier to exorcise.
No Enums
What if we had a multi-state toggle control in our original example? What if we had, High
, Low
, and Off
. We can't represent this as a bool
with an if-else
construct. We need to move to something more robust that can hold more than binary information. We can use ints:
public interface IToggle{
bool ShouldBe();
void ToggleHigh();
void ToggleLow();
void ToggleOff();
}
and now a use of this
public void ProcessToggle(IToggle toggle){
if(me.ShouldBe() == 1){
me.ToggleHigh();
} else if(me.ShouldBe() == 2){
me.ToggleLow();
} else {
me.ToggleOff();
}
}
Going straight int
makes it really easy to have magic numbers floating about. If we don't do magic numbers we end up with public constants. I'm sure I've written about it before, but we don't use public constants - find one of those posts for more details.
The remaining option for encoding this information while using the if-else
chain is an enum
.
public void ProcessToggle(IToggle toggle){
if(me.ShouldBe() == Toggle.High){
me.ToggleHigh();
} else if(me.ShouldBe() == Toggle.Low){
me.ToggleLow();
} else {
me.ToggleOff();
}
}
This is where the IDE probably suggests the swap to a switch
.
public void ProcessToggle(IToggle toggle){
switch(me.ShouldBe()){
case Toggle.High:
me.ToggleHigh();
break;
case Toggle.Low:
me.ToggleLow();
break;
default:
me.ToggleOff();
}
}
This is the same behavior decision. I don't address them distinctly much. They both make a decision about what behavior should be executed. This is bad OO code. It knows way too much. It's going to be repeated in many places in the code - No.
There's a very distinct reason we don't want if-else
chains and switch
in our code. This dual-state to tri-state starts to show it.
Ever Growing
One of the costs with if-else
and switch
statements is that they always grow.
As our system becomes more complex, more behaviors are introduced. When related behaviors are introduced into the system, they're new if-else
or case
statmetns. These constructs grow. They get all the new behavior decisions and never shrink. It's hard to tell when the code goes dead in these statments.
We don't want these to grow. We don't want them to exist. It's easier to avoid behavior decisions when you don't allow the constructs required for them.
How can we avoid an ever growing state decision cosntruct? Even if we have all of our logic in a single class, we'll have ever growing decision constructs there.
Yes - let's avoid that.
Make the decision once
Once the behaviors are all encapsulated into a single class, let's refactor! Extract each common branch into a new object.
Introducing a new example
public class User{
public bool NeedsTimeCard(){
return this.EmployeeType != EmployeeType.SALARY;
}
public void ClockInOut(){
switch(this.EmployeeType){
case HOURLY: this.clock.Hourly(this.Id);
case VOLUNTEER: this.clock.Volunteer(this.Id);
case VOLUNTOLD: this.clock.Voluntold(this.Id);
}
}
public IReviewResults Review(){
switch(this.EmployeeType){
case HOURLY: return new HourlyReview(this);
case VOLUNTEER: return new VolunteerReview(this);
case VOLUNTOLD: return new VoluntoldReview(this);
case SALARY: return new SalaryReview(this);
}
}
}
Here we have three behaviors. Each with a switch bigger than the last. How does each need to be modified if we get a new type - INTERN
?
What if we stop having VOLUNTEER
s?
This class changes for all of these reasons - it's a big scope of change. We should be striving for a narrow scope of change - else
and switch
force us to have a big scope of chagne.
It's harder to test, harder to ensure correct changes. These types of systems become chaotic. Especially if we need to make more behavior decisions, starting to nest switch statements... The test matrix becomes a nightmare.
Let's save ourselves from this hellscape.
The idea is to exact each different type into a new object. When we implement interfaces, this is a minimal change for the system - without interfaces... everywhere that User
is used would need to be updated. Interfaces allow us a narrow scope of change when we need to make these refactors.
Let's see what we can do to extract this into a sensible situations.
First the interface - just push all the methods into an interface.
public interface IUser{
bool NeedsTimeCard();
IReviewResults Review();
void ClockInOut()
}
At this stage I don't care about what the interface contract IS, just that it exists. If it existsed before, all the better.
Look to your smallest branching behavior to start extracting. In our case, the smallest is a single line and references EmployeeType.SALARY
so we'll start with that.
internal class SalaryUser:IUser{
bool NeedsTimeCard() => false;
IReviewResults Review() => new SalaryReview();
void ClockInOut() {/* no-op */}
}
We've pulled in the rest of the functionality as well.
How can we get the system using this new class? Assuming the interface update has been done?
We need to isolate instantiation. Simple refactor is a factory pattern. I don't agree with this most of the time, but it's well know and allows us to move on. There are different techniques I'd use to eventually get rid of the factory as well.
Replace all instantiation with the factory that should basically look like
...
IUser CreateUser(EmployeeType type, EmployeeData data){
switch(type){
case SALARY: return new SalaryUser(data);
default: return new User(data);
}
}
...
This allows us to ensure the system works with the new user type without doing all types up front.
Let's speed it up and extract out the rest of our users
internal class SalaryUser:IUser{
bool NeedsTimeCard() => false;
IReviewResults Review() => new SalaryReview();
void ClockInOut() {/* no-op */}
}
internal class HourlyUser:IUser{
bool NeedsTimeCard() => true;
IReviewResults Review() => new HourlyReview();
void ClockInOut() => this.clock.Hourly(this.Id);
}
internal class Volunteer:IUser{
bool NeedsTimeCard() => true;
IReviewResults Review() => new VolunteerReview();
void ClockInOut() => this.clock.Volunteer(this.Id);
}
internal class Voluntold:IUser{
bool NeedsTimeCard() => true;
IReviewResults Review() => new VolunteerReview();
void ClockInOut() => this.clock.Voluntold(this.Id);
}
and our factory looks like
...
IUser CreateUser(EmployeeType type, EmployeeData data){
switch(type){
case SALARY: return new SalaryUser(data);
case HOURLY: return new HourlyUser(data);
case VOLUNTEER: return new VolunteerUser(data);
case VOLUNTOLD: return new VoluntoldUser(data);
default: return new NullUser(data);
}
}
...
I've snuck in the NullUser
there. If it's an unknown type - don't break, handle it. :)
Now, to add in an INTERN
type, we create the class with the encapsulated behavior and add it to the factory. The scope of change for a new user type, a single existing class.
In the beginning, when we'd have a switch
and if-else
throughout our code, the scope of change would be huge, and hopefully done correctly.
Summary
This is a quick example of extracting into classes the behavior that the switch
and else
constructs scatter through our code. When we eliminate them, our code gets smaller. We get simple, clear, easy classes that are easy to maintain.
As we bring behavior into classes and interact by telling the object what we want, not asking them for information to make the decision ourself.
This is the "Tell, Don't Ask" principle. I don't like the phrasing on it, though the details make it clear. It conflicts with my approach of anthropomorphisim the object. Treat it with respect.
Don't TELL the object what to do, ask it to do the thing for you. These are the very same ideas, phrased exactly opposite.
The other quote I really like is, as far back as I've bothered to trace, from Sandi Metz, "I know what I want, you know how to do it".
It's all down to the same thing - Objects are all about the Behavior they provide.
Don't get from them, ask of them.