Let's say we have a function that takes the name of a city and returns the city's current weather. More details about the implementation:
- The user might misspell the city name. The function will use a spell checker to correct any spelling errors we may find.
- Even with spell correction, we may not find the city. We will throw an exception if we can't find any city.
- There can be multiple cities with the same name. For example there is a Hyderabad in India, and one in Pakistan. In such cases, we will arbitrarily pick one. If we get the wrong city, user has to refine the query.
- We may not be able to get weather data. If the weather service we are relying on fails, we return
null. (You would probably throw an exception here as well, but this is just a fabricated example.)
- You don't always want real weather data. Most of the time, you are working on different parts of the app; you don't care if the weather you get is accurate or not. You want to use fake weather data if you are running the app for development/testing.
I am going to use Java in this post because that's what I have been writing these days. First, I present the dreaded conditionals version:
Weather getWeatherFor(String cityName, boolean mayCorrect) throws CityNotFoundException {
if (AppConfig.useFakeWeather()) {
return AppConfig.getFakeWeather();
} else {
try {
List<Location> locations = locationService.findCity(cityName);
if (locations.size() > 0) {
Location locn = locations.get(0);
return weatherService.getCurrentWeather(locn);
} else {
if (mayCorrect) {
for (String name : spellCheckService.getCorrectionsFor(cityName)) {
Weather weather = getWeatherFor(name, false);
if (weather != null) {
return weather;
}
}
throw new CityNotFoundException(cityName);
} else {
throw new CityNotFoundException(cityName);
}
}
} catch (WeatherException e) {
return null;
}
}
}
Sure, you can read this code and understand what's going on. But how easy is it to find a bug in this? And how easy it would be to fix that bug?
The problem with this code is that it uses way too much branching. An if-else is essentially one statement. What I mean by that is, when you change something in the middle of an if-else, you should keep the entire if-else in mind. The condition
(weather != null), for example, is 3 levels down a decision tree. Before you can touch that part of this function, you should have worked out all 3 paths of the decision tree in your mind; otherwise your change might introduce a bug.
Now, the same code written in a way I like:
Weather getWeatherFor(String cityName, boolean mayCorrect) throws CityNotFoundException {
if (appConfig.useFakeWeather()) {
return appConfig.getFakeWeather();
}
List<String> candidates = new ArrayList<String>();
if (mayCorrect) {
candidates = spellCheckService.getCorrectionsFor(cityName);
}
// Actual input must be the first search we do; insert it
// as the first item in the list.
candidates.add(0, cityName);
Location locn = findCity(candidates);
try {
return weatherService.getCurrentWeather(locn);
} catch (WeatherException e) {
return null;
}
}
Location findCity(List<String> nameCandidates) throws CityNotFoundException {
for (String candidate : nameCandidates) {
List<Location> locations = locationService.findCity(cityName);
if (locations.size() > 0) {
return locations.get(0);
}
}
return new CityNotFoundException(nameCandidates.get(0));
}
In this version, the decision tree is only one level deep at any given point. (
for and
try are not necessarily "conditionals", but even if you include them, nesting is only two levels deep. Which of these two versions would you change confidently?
If you have been writing code for a while, you will know that what starts like the second version would eventually end up like the first version over time: by changes that happen one line at a time. As you make code changes, check how deep your decision trees get; if it's anything more than two, it's time to restructure the code to simplify it.
