An exercise in refactoring

The other day at work I was implementing a new feature and found myself in a nice little, self-contained bit of code. I was adding the ability to parse a pair of query parameters from an HTTP request into an optional Period (a class used to represent a period of time in the application). After TDDing my functionality into place I reached a fully functional version but cringed every time I looked at it. I normally try to refactor during the TDDing but I just couldn’t see what to do until I had a bit more meat to work with. Once it was all in place I had enough to start chopping away at it.

The code you see below is pretty much the code as it is in the application. I’ve changed a few of the types to remove references to internal structures, but I haven’t changed the logic. It makes use of a modified version of Nat Pryce’s Maybe class and uses Guava, which shows up in some of the interfaces that get implemented.

So this is where we start:


public static Maybe<Period> optionalPeriod(Maybe<String> from, final String fromParamName, Maybe<String> to, final String toParamName) {
  final Maybe<Date> fromDate = optionalDate(from, fromParamName);
  final Maybe<Date> toDate = optionalDate(to, toParamName);

  if (fromDate.isKnown() && toDate.isEmpty()) {
    throw badRequestError(invalidPeriodError(INCOMPLETE_STARTED_PERIOD_MESSAGE, fromParamName, toParamName, from.otherwise(MISSING_VALUE), to.otherwise(MISSING_VALUE)));
  }

  if (fromDate.isEmpty() && toDate.isKnown()) {
    throw badRequestError(invalidPeriodError(INCOMPLETE_ENDED_PERIOD_MESSAGE, fromParamName, toParamName, from.otherwise(MISSING_VALUE), to.otherwise(MISSING_VALUE)));
  }

  return fromDate.bind(new Function<Date, Maybe<? extends Period>>() {
    @Override public Maybe<Period> apply(final Date from) {
      return toDate.transform(new Function<Date, Period>() {
        @Override public Period apply(Date to) {
          Period period = new Period(from, to);
          assertPeriodInOrder(period, fromParamName, toParamName);
          return period;
        }
      });
    }
  });
}

The first few lines seemed alright. They fairly clearly state that the two parameters needed to be considered optional dates and followed up with some lines declaring that if we have one but not the other then there is an error. The last statement was a horrible monstrosity, though. That said, my first victim was something that can’t even be easily seen in the above code: if neither date is there then the function should return nothing(). After that I took a swipe at the little bit of duplication that I felt was showing up in the two if statements about one or the other date being missing:


public static Maybe<Period> optionalPeriod(Maybe<String> from, final String fromParamName, Maybe<String> to, final String toParamName) {
  final Maybe<Date> fromDate = optionalDate(from, fromParamName);
  final Maybe<Date> toDate = optionalDate(to, toParamName);

  if (fromDate.isEmpty() && toDate.isEmpty()) {
    return Maybe.nothing();
  }

  if (fromDate.isKnown() && toDate.isEmpty() || fromDate.isEmpty() && toDate.isKnown()) {
    String message = fromDate.isKnown() ? INCOMPLETE_STARTED_PERIOD_MESSAGE : INCOMPLETE_ENDED_PERIOD_MESSAGE;
    throw badRequestError(invalidPeriodError(message, fromParamName, toParamName, from.otherwise(MISSING_VALUE), to.otherwise(MISSING_VALUE)));
  }

  return fromDate.bind(new Function<Date, Maybe<? extends Period>>() {
    @Override public Maybe<Period> apply(final Date from) {
      return toDate.transform(new Function<Date, Period>() {
        @Override public Period apply(Date to) {
          Period period = new Period(from, to);
          assertPeriodInOrder(period, fromParamName, toParamName);
          return period;
        }
      });
    }
  });
}

At this point I was pretty happy with the first few lines of the function. But that error checking was now an even worse eyesore! Working through the logic I realized that the way it was now structured I should never reach the final, eye-bleed inducing final return statement unless both dates are specified. That bit of information means that all of that obscure bind() and transform() logic is completely unnecessary. The second breakthrough was in noticing that if either of fromDate or toDate end up being empty after the check that neither is known then it is an error.

With those two new realizations I could get rid of bind() and transform() and replace it with just getting the values out of the Maybes or throwing the appropriate exception.


public static Maybe<Period> optionalPeriod(final Maybe<String> from, final String fromParamName, final Maybe<String> to, final String toParamName) {
  final Maybe<Date> fromDate = optionalDate(from, fromParamName);
  final Maybe<Date> toDate = optionalDate(to, toParamName);

  if (fromDate.isEmpty() && toDate.isEmpty()) {
    return Maybe.nothing();
  }

  Supplier<WebApplicationException> missingStartDate = new Supplier<WebApplicationException>() {
    @Override public WebApplicationException get() {
      return badRequestError(invalidPeriodError(INCOMPLETE_ENDED_PERIOD_MESSAGE, fromParamName, toParamName, from.otherwise(MISSING_VALUE), to.otherwise(MISSING_VALUE)));
    }
  };

  Supplier<WebApplicationException> missingEndDate = new Supplier<WebApplicationException>() {
    @Override public WebApplicationException get() {
      return badRequestError(invalidPeriodError(INCOMPLETE_STARTED_PERIOD_MESSAGE, fromParamName, toParamName, from.otherwise(MISSING_VALUE), to.otherwise(MISSING_VALUE)));
    }
  };

  Period period = new Period(fromDate.otherwiseThrow(missingStartDate), toDate.otherwiseThrow(missingEndDate));
  assertPeriodInOrder(period, fromParamName, toParamName);
  return Maybe.definitely(period);
}

Not bad. But having those two Supplier creations inline wasn’t so nice. Let’s extract those out.


public static Maybe<Period> optionalPeriod(final Maybe<String> from, final String fromParamName, final Maybe<String> to, final String toParamName) {
  final Maybe<Date> fromDate = optionalDate(from, fromParamName);
  final Maybe<Date> toDate = optionalDate(to, toParamName);

  if (fromDate.isEmpty() && toDate.isEmpty()) {
    return Maybe.nothing();
  }

  Supplier<WebApplicationException> missingStartDate = missingPeriodPortion(INCOMPLETE_ENDED_PERIOD_MESSAGE, from, fromParamName, to, toParamName);
  Supplier<WebApplicationException> missingEndDate = missingPeriodPortion(INCOMPLETE_STARTED_PERIOD_MESSAGE, from, fromParamName, to, toParamName);

  Period period = new Period(fromDate.otherwiseThrow(missingStartDate), toDate.otherwiseThrow(missingEndDate));
  assertPeriodInOrder(period, fromParamName, toParamName);
  return Maybe.definitely(period);
}

Much better. I find that the inlined fetching of the dates from the Maybes doesn’t lead to the clearest expression of what is going on. So let’s pull out the dates and inline the exception Suppliers. With that the dates can even be given names that give a better clue as to what is special about them at that point in the code.


public static Maybe<Period> optionalPeriod(final Maybe<String> from, final String fromParamName, final Maybe<String> to, final String toParamName) {
  final Maybe<Date> optionalFromDate = optionalDate(from, fromParamName);
  final Maybe<Date> optionalToDate = optionalDate(to, toParamName);

  if (optionalFromDate.isEmpty() && optionalToDate.isEmpty()) {
    return Maybe.nothing();
  }

  final Date requiredFromDate = optionalFromDate.otherwiseThrow(missingPeriodPortion(PERIOD_MISSING_FROM_DATE_MESSAGE, from, fromParamName, to, toParamName));
  final Date requiredToDate = optionalToDate.otherwiseThrow(missingPeriodPortion(PERIOD_MISSING_TO_DATE_MESSAGE, from, fromParamName, to, toParamName));

  Period period = new Period(requiredFromDate, requiredToDate);
  assertPeriodInOrder(period, fromParamName, toParamName);
  return Maybe.definitely(period);
}

Finally I decided to pull out the creation and checking of the Period itself. There were two reasons for this: I don’t like having a possibly “invalid” object floating around in a larger function, and there was another bit of code just above this function that was doing the same thing.


public static Maybe<Period> optionalPeriod(final Maybe<String> from, final String fromParamName, final Maybe<String> to, final String toParamName) {
  final Maybe<Date> optionalFromDate = optionalDate(from, fromParamName);
  final Maybe<Date> optionalToDate = optionalDate(to, toParamName);

  if (optionalFromDate.isEmpty() && optionalToDate.isEmpty()) {
    return Maybe.nothing();
  }

  final Date requiredFromDate = optionalFromDate.otherwiseThrow(missingPeriodPortion(PERIOD_MISSING_FROM_DATE_MESSAGE, from, fromParamName, to, toParamName));
  final Date requiredToDate = optionalToDate.otherwiseThrow(missingPeriodPortion(PERIOD_MISSING_TO_DATE_MESSAGE, from, fromParamName, to, toParamName));
  return Maybe.definitely(periodInOrder(requiredFromDate, requiredToDate, fromParamName, toParamName));
}

This is the point at which I stopped. Next time I go through this code I hope to find a few more things to clean up, though; this point is in no way the final destination. There are a few more possibilities that could be done here that I can see right now. The most obvious one to me was that the combination of parameter value and parameter name is just screaming out for a class. That would give a place to start moving a lot of this validation and transformation functionality that is right now just living in static methods (although the static methods are working pretty well right now).

I hope that this is a good illustration of refactoring in action on a real codebase. Something that I want everyone to get out of this is that not every step of the refactoring took me to a nice next step. This exercise was full of false starts and backslides in the readability of the code. I haven’t shown every dead end that I went down and had to undo out of, but rest assured it happened pretty often. What is important is that I kept trying to figure out how to better express what was happening. As I tried to make it clearer I ended up with a better understanding myself and could try to reflect that in the code.

Advertisements
This entry was posted in Software. Bookmark the permalink.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s