Encapsulation ...Don't Abuse The If
/**
* Destined to sink
*/
public class Titanic
{
private final WallStreet wallstreet = new WallStreet();
private final Pharaoh pharaoh = new Pharaoh();
private final Oilwell oilwell = new Oilwell();
private final Cowboy[] cowboys = new Cowboy[]{new Cowboy()};
public void sail(String direction)
{
if("NORTH".equals(direction))
{
wallstreet.moneyNeverSleeps();
}
else if("SOUTH".equals(direction))
{
pharaoh.curse();
}
else if("EAST".equals(direction))
{
oilwell.drill();
}
else if("WEST".equals(direction))
{
for (Cowboy cowboy : this.cowboys) {
cowboy.shoot();
}
}
else{
throw new UnknownDestination();
}
}
}
/**
* Smooth sailing
*/
public class CruiseLiner
{
private static final String UNKNOWN_DESTINATION =
"There is no destination in the direction '%s'. " +
"Did you specify it as such in CruiseLiner#newCruiseLiner " +
"when constructing the cruise liner?";
/**
* Create a new {@link CruiseLiner} which is able to sail to the specified
* destinations
*
* @param destinations the destinations that can set sail to
* @return a new {@link CruiseLiner}
* @see Destinations
*/
public static CruiseLiner newCruiseLiner(Destination... destinations)
{
EnumMap<Direction, Destination> map =
new EnumMap<Direction, Destination>(Direction.class);
for (Destination destination : destinations){
map.put(destination.direction(), destination);
}
return new CruiseLiner(map);
}
/*
* A map by direction for each destination
* No need to specify any other dependencies here. Great!
*/
private final EnumMap<Direction, Destination> map;
private CruiseLiner(EnumMap<Direction, Destination> map)
{
this.map = map;
}
public void sail(Direction direction)
{
//Use the map to get the destination you want for the given direction
Destination destination = this.map.get(direction);
if(destination == null){
throw new UnknownDestination(
String.format(UNKNOWN_DESTINATION, direction));
}
//let the destination specify what to do once the cruise liner sails there
destination.sail(this);
}
}
/**
* Get your directions together
*/
public enum Direction
{
NORTH,
SOUTH,
EAST,
WEST,
UKNOWN
}
/**
* Encapsulate your destinations by introducing a common interface
*/
public interface Destination
{
public Direction direction();
/**
* Implement this method with what you would do at each destination
*/
public void sail(CruiseLiner cruiseLiner);
}
/**
* Constant destinations with each specifying its required dependencies
*/
public class Destinations
{
public static final Destination NORTH_AMERICA = new Destination()
{
private final WallStreet wallstreet = new WallStreet();
@Override
public void sail(CruiseLiner cruiseLiner)
{
wallstreet.moneyNeverSleeps();
}
@Override
public Direction direction() {
return Direction.NORTH;
}
};
public static final Destination SOUTH_AFRICA = new Destination() {
private final Pharaoh pharaoh = new Pharaoh();
@Override
public void sail(CruiseLiner cruiseLiner)
{
pharaoh.curse();
}
@Override
public Direction direction() {
return Direction.SOUTH;
}
};
public static final Destination MIDDLE_EAST = new Destination() {
private final Oilwell oilwell = new Oilwell();
@Override
public void sail(CruiseLiner cruiseLiner)
{
oilwell.drill();
}
@Override
public Direction direction() {
return Direction.EAST;
}
};
public static final Destination GREAT_WEST = new Destination() {
private final Cowboy[] cowboys = new Cowboy[]{new Cowboy()};
@Override
public void sail(CruiseLiner cruiseLiner)
{
for (Cowboy cowboy : this.cowboys) {
cowboy.shoot();
}
}
@Override
public Direction direction() {
return Direction.WEST;
}
};
}
/**
* Let's cruise
*/
public class CruiseLinerTest
{
private final Destination[] DESTINATIONS =
new Destination[]
{
/*
* Use constants!
*/
Destinations.NORTH_AMERICA,
Destinations.SOUTH_AFRICA,
Destinations.MIDDLE_EAST,
Destinations.WILD_WEST
};
@Test
public void sail() throws Exception
{
CruiseLiner cruiseLiner =
CruiseLiner.newCruiseLiner( DESTINATIONS );
cruiseLiner.sail( Direction.NORTH );
cruiseLiner.sail( Direction.SOUTH );
cruiseLiner.sail( Direction.EAST );
cruiseLiner.sail( Direction.WEST );
}
@Test(expected=UnknownDestination.class)
public void sailUnknown() throws Exception
{
CruiseLiner cruiseLiner =
CruiseLiner.newCruiseLiner();
cruiseLiner.sail(Direction.UKNOWN);
}
}It is shocking to see this basic abuse of an if so frequently. In the @qnoid, over at github and bitbucket, on freshly baked projects. Especially when all it takes is to use a Map instead of an long if statement and take advantage of delegation.
It usually manifests in the controller where you have a single method to handle different operations and though it resembles polymorphism is less about it and more about composition.
Again you might say it is nothing serious and you can choose to ignore it but it does make your code less extensible, harder to maintain, less readable and destined to sink.
If (pun intended) you add all the dependencies related to the different actions for each block you end up with one monolithic class. That’s ugly. On a lighter note, using strings isn’t type safe and thus error prone. Think how many times in the past you’ve had to debug for hours because of a single typo. Not fun.
Assuming you’ve used an enum you shouldn’t have any troubling mapping it with
Direction direction = Direction.valueOf( name );
So next time you are tempted to use a long if block just use a map where each key is the name of your action and the corresponding value is an object of a class implementing a common interface with a single method.
PS. Get used to the habit of creating constants out of your classes.