Change the Wrong Way

Many factors can force software to change, such as new business requirements or new technology requirements. As software developers, we have to handle the constant stream of new demands in a maintainable way. But sometimes we don't have the time, or we don't have the resources. We just need to make it work because they need it like yesterday! When we allow ourselves to get caught up in the frenzy, we let quality slip. Other times, we just didn't know. Or we had bad practices that we thought were fine. Quality slips all the same. Can we see these issues just by looking at the code?

public List getPersons(String name, String firstName, int contacts, boolean sort,
    boolean dir) {
  ResultSet rs;
  List persons = new ArrayList();
  if (name != null) {
    rs = db.query("SELECT * FROM People");
    addAllPersons(rs, persons);
    if (sort) Collections.sort(persons);
    if (dir)
      return persons;
  } else {
    rs = db.query("SELECT * FROM People WHERE last_name = ? " +
      (firstName != null ? " AND first_name = ?" : ""),
      Arrays.asList(name, firstName).stream().filter(Objects::nonNull)
        .collect(Collectors.toList()).toArray());

    addAllPersons(rs, persons);

    if (contacts > 0) {
      rs = db.query("SELECT People.* FROM People JOIN Contacts ON " +
        "Contacts.person_id = People.id WHERE Contacts.contact_id IN (" +
        String.join(",", getPersonsIds(persons)) + ")");
      addAllPersons(rs, persons);
    }

    if (sort) {
      Collections.sort(persons);
    }
  }

  if(!dir) {
    Collections.reverse(persons);
  }

  return persons;
}

This method exhibits change gone wrong. It's bad. We can see how it grew over time. We started with a method to get all the people in the database. Then one day, we just needed to filter by name, but also still get all the people if we wanted to. This is the first step down the wrong path. These should have been two separate methods, but we just needed this to work. Now we may have calls like the following sprinkled throughout the code:

people.getPersons(null, null, -1, false, true);
// ...
people.getPersons("Smith", null, 1, true, true);
// ...
people.getPersons("Spector", "Jim", 6, false, true);

Looking at the invocations, we cannot know the meanings of the different parameters. We might guess the first two are names based on the last call, but we couldn't know that in the first call. Our IDE can show us what the names of the parameters are, but sometimes even that doesn't help. What does the boolean dir mean without looking at the implementation? If we realize it means sort direction, is true ascending or descending? And what does it do if we aren't sorting?

A significant number of calls explicitly pass in a null for a parameter to completely switch behavior, telling us there is a lot going on. The name and firstName parameters of the method above have several explicit nulls passed in as their values to change the searching behavior. Their values also determine if the contacts parameter is used. Passing explicit booleans is also a smell. If we know the value of the boolean parameter, then we are able to call a more semantic method right there at compile time.

The issue at hand is not how to fix the above code. Any decent software developer would be able to imagine one of the many ways to accomplish that. The real question is why code ends up looking like this. The developers who make these changes think they are doing the right thing. They don't realize that they are making the code less flexible, not more. In the above method, searching, sorting, and finding contacts are tied together. Trying to use just one of those features requires knowing exactly the right combination of flags. But the way they see it, searching and sorting should go together like peanut butter and chocolate. After all, it reduces duplication! There are not a number of sort calls all over the place. Instead, it is just inside this one method. They fail to realize they've introduced a new duplication in many more places. We now have to duplicate the knowledge that the boolean parameter means "sort" to every call to the method, including the ones that don't sort.

Sometimes it isn't just a single developer. There may be an entire team or an entire organization unaware of (or complicit in) the quality issues being created. Even a developer who knows better may give in because the world is against them. But one day, it all comes crashing down. Suddenly all the business people want to know why it is so hard just to make it sort by first name in this one case. A mountain of technical debt comes due. The quality issues become apparent to everyone, rotting from the inside out. Had there been a focus on quality, it wouldn't have turned out that way.