Boy-scout refactoring

I recently had a puzzling discussion with another  developer regarding refactoring; more specifically the use of private methods – he didn’t like them, favouring giant public methods which presumably require a whole load of inline comments to explain what they’re doing at various (probably deeply nested) stages.

Anyway, I thought I’d post a trivial example of the kind of small refactoring exercises that one can do along the way as they touch related code – leaving the code slightly prettier than they found it, as a boy scout might leave a campsite.

Here’s an original method I came across:

protected boolean validate() {
  boolean ok = true;

  if (JaasUserUtil.hasPermission("Use New Profile Pages")
      && JaasUserUtil.hasPermission("Change redirect SECRET")) {

    EnvironmentPropertiesDataBean data = (EnvironmentPropertiesDataBean)retrieveBean();

    // verify if a newly entered SECRET is secure enough
    if (!data.getDisableRedirectSecret() && data.getRedirectSecret()!=null) {
      ok = data.getRedirectSecret().length() <= 20
          && PasswordUtil.validatePassword(data.getRedirectSecret());
      if (!ok) {
        data.addFeedbackLine(new FeedbackItem(FeedbackItem.Type.WARNING,
        new BundleKey("account_profile.environment.account_secret_invalid",
        new String[]{data.getAccountCode()})));
        // SECRET value entered by the user is invalid -- now, if the SECRET *WAS* empty (disabled), then make sure
        // that it remains that way. Otherwise, the user would be told 'SECRET is set' while that's not actually true!
        String originalValue = AccountCache.get().getAccount(data.getAccountKey()).getRedirectSecret();
        if (originalValue==null) {      // It was disabled;
          data.setDisableaRedirectSecret(true);    // so, make sure it remains disabled.
        }
        data.setResult(FAILURE);
      }
    }
  }
  return ok;
}

 

And here’s how I left it:

protected boolean validate() {
  EnvironmentPropertiesDataBean data = (EnvironmentPropertiesDataBean) retrieveBean();
  return validationNotApplicable(data) || validateSecretStrength(data);
}

private boolean validationNotApplicable(EnvironmentPropertiesDataBean data ) {
  return (data.getRedirectSecret() == null || !userHasPermissionToChangeSecret() || data.getDisableRedirectSecret());
}

private boolean userHasPermissionToChangeSecret() {
  return JaasUserUtil.hasPermission("Use New Profile Pages") && JaasUserUtil.hasPermission("Change redirect SECRET");
}

private boolean validateSecretStrength(EnvironmentPropertiesDataBean data) {
  boolean result = PasswordUtil.validateSecretStrength(data.getRedirectSecret());
  if(!result) {
    handleInvalidSecret(data);
  }
  return result;
}

private void handleInvalidSecret(EnvironmentPropertiesDataBean data) {
  BundleKey bundleKey =  new BundleKey("account_profile.environment.account_secret_invalid", new String[]{data.getAccountCode()});
  data.addFeedbackLine(new FeedbackItem(FeedbackItem.Type.WARNING, bundleKey));
  resetDisabledRedirectSecretToOriginalState(data);
  data.setResult(FAILURE);
}

private void resetDisabledRedirectSecretToOriginalState(EnvironmentPropertiesDataBean data) {
  String originalValue = AccountCache.get().getAccount(data.getAccountKey()).getRedirectSecret();
  if (originalValue == null) {
    data.setDisableRedirectSecret(true);
  }
}

To start with the very first comment on line 9 in the first example would be entirely redundant if only the method had a self-describing name (such as ‘validatePasswordStrength’ rather than plain old ‘validatePassword’). Wham! more self-describing code & removal of a comment in one single method extraction (2 or 3 IDE clicks).

In this particular instance I didn’t change the method name in order to minimise my refactoring footprint, but you will see in the refactored example that the static method on PasswordUtil now has a different name which makes it’s purpose clearer, and in turn the purpose of it’s invoking code is clearer.

You should also see in the original that the latter comments are definitely necessary in this snippet if one wants to get a quick overview of the functionality without having to think about the code. The problem with code comments is that there is no mechanism to force them to stay up to date (i.e. we often find comments in code which say that it does something entirely different that it in actual fact does), and apart from looking untidy a reasonable assumption to make is that the code which they describe is almost certainly more complex than it needs to be (otherwise why write an explanation of what it does?). So comments in code tend to indicate a ‘code smell’ – they hint that something probably isn’t as good as it could be, or typically, much worse.

Smell aside, though, what if you don’t want to actually have to read the code to know what it does? This will probaby be the case more often than not.

The refactored version does away with the comments as they aren’t necessary anymore, and you can get an understanding of what the method does now from a quick glance – without having to actually read the implementation. We just saved future readers some effort.

The refactored version of the same method isn’t perfect, and it’s not meant to be – it is however significantly better than it was when I found it. The purpose of taking a ‘boy-scout’ (although I prefer ‘common sense’)  approach is not to make the codebase perfect – which typically costs more money than it saves – but to encourage it’s maintainability.

TL;DR?

The second one is better than the first. If you disagree then you are insane.

This entry was posted in coding, java, 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