💾 Archived View for capsule.dotslashme.com › gemlog › posts › 2017-06-01-the-inner-beauty.gmi captured on 2024-02-05 at 09:34:22. Gemini links have been rewritten to link to archived content

-=-=-=-=-=-=-

The inner beauty

published on 2017-06-01

--//# best-practices # refactoring # java

All software projects suffer the same issue - change, but what does that really mean?

Changes are of course not inherently bad things, but with every change, the code debt will increase and without refactoring, this debt will over time become harder and harder to handle. In this post, I will highlight changes to the internal data structure and what this can cause if some refactoring was overlooked.

A change in the internal data model

Below is a snippet with a String class member called services. By looking at the unused methods below, it is quite obvious that at one time, this was just a string passed around, multiple services were most likely comma separated, but these two methods have been tagged with unused and a new getter and setter have been implemented. Now look at the the getServices() method, it returns a list of strings and the setServices() method takes a list of strings as an argument. So apparently someone decided that the string services should be exchanged for a list of string at some other place in the code, but in this class the internal data model was not changed.

private String services;

@SuppressWarnings("unused")
private String getServicesList() {
  return this.services;
}

@SuppressWarnings("unused")
private void setServicesList(String services) {
  this.services = services;
}

public List<String> getServices() {
  List<String> services = new ArrayList<String>();
  if (this.services != null) {
    for (String service : this.services.split(",")) {
      services.add(service);
    }
  }
  return services;
}

public void setServices(List<String> services) {
  if (services.size() > 0) {
    StringBuffer sb = new StringBuffer();
    for (String service : services) {
      sb.append(service);
      sb.append(",");
    }
    this.services = sb.substring(0, sb.length() - 1);
  } else {
    this.services = "";
  }
  this.services = services;
}

As you can see, the setter and the getter are not really good methods, mostly because we are doing a conversion between two different data models, but even if we're forced to maintain a conversion, it can be done a lot better. The setter and the getter are also not safe, and the data in the objects they return cannot be trusted.

I of course decided to refactor this method and since I could skip maintaining the dual data model, I updated the code to:

private List<String> services = new ArrayList<>();

public List<String> getServices() {
  return Collections.unmodifiableList(this.services);
}

public void setServices(List<String> services) {
  this.services = new ArrayList<>(services);
}

The code is more readable, it's shorter and more to the point since the data model now is consistent. The setter and the getter are also converted to safer alternatives, where the data in the services member can no longer be modified from outside this class.

Conclusion

Changes and refactoring goes hand in hand, because without refactoring the debt will increase and soon overwhelm you. Maintaining the correct internal data model within your objects will not only simplify the logic of your setters and getters, but will also allow for smoother and leaner code. Domain driven design can also be a very good alternative here, since it enforces more well defined objects.

licensed under cc by-sa 4.0