Petter Måhlén's Blog

Last summer, I read the excellent book Clean Code by Robert Martin and was so impressed that I made it mandatory reading for the coders in my team. One of the techniques that he describes is that rather than writing code like:

  if (a != null && a.getLimit() > current) {
    // ...
  }

you should replace the boolean conditions with a method call and make sure the method name is descriptive of what you think should be true to execute the body of the if statement:

  if (withinLimit(a, current)) {
    // ...
  }

  private boolean withinLimit(A a, int current) {
    return a != null && a.getLimit() > current;
  }

I think that is great advice, because I used to always say that unclear logical conditions is one of the things that really requires code comments to make them understandable. The scenario where you have an if-statement with 3-4 boolean statements AND-ed and OR-ed together is

1. not uncommon, and
2. frequently a source of bugs, or something that needs changing, and
3. is something that stops you dead in your tracks when trying to puzzle out what the code in front of you does.

So it is important to make your conditions easily understandable. Rather than documenting what you want the condition to mean, simply creating a tiny method whose name is exactly what you want the condition to mean makes the flow of the code so much smoother. I also think (but I’m not sure) that it is easier to keep method names in synch with code updates than it is to keep comments up to date.

So that’s advice I have taken onboard when I write code. Yesterday, I found myself writing this:

  public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) {

    // ...

    if (shouldPerformTracking()) {
      executor.submit(new TrackingCallable(servletRequest, trackingService, ctrContext));
    }

    // ...
  }

  private boolean shouldPerformTracking() {
    // only do tracking if the controller has added a page node to the context
    return trackingDeterminator.performTracking() && hasPageNode(ctrContext);
  }

  private boolean hasPageNode(CtrContext ctrContext) {
    return !ctrContext.getEventNode().getChildren().isEmpty();
  }

As I was about to check that in, it suddenly struck me that even if the name of the method describing what I meant by the second logical condition was correct (hasPageNode()), it was weird that I had just written a comment explaining just that – what I meant by the logical condition, namely that the tracking should happen if tracking was enabled and the controller had added tracking information to the context. I figured out what the problem was and changed it to this:

  private boolean shouldPerformTracking() {
    return trackingDeterminator.performTracking() && controllerAddedPageInfo(ctrContext);
  }

  private boolean controllerAddedPageInfo(CtrContext ctrContext) {
    return !ctrContext.getEventNode().getChildren().isEmpty();
  }

That was a bit of an eye-opener somehow. The name of the condition-method should describe the intent, not the mechanics of the condition. So the first take on the method name (hasPageNode()) described what the condition checked in a more readable way, but it didn’t describe why that was a useful thing to check. If this type of method names explain the why of the condition, I strongly think they increase code readability and therefore represent a great technique.

As always, naming is hard and worth spending a little extra effort on if you want your code to be readable.