6.17.2009

Refactoring - Compose Method

This refactoring, again from Refactoring to Patterns, reduces ambiguity in methods that have too much detail or conditional logic to quickly comprehend. By breaking a method down into smaller methods, the ambiguity can be removed.

The mechanics of this refactoring are rather subjective, but obviously lead to the use of Extract Method. Study the code and make sure you understand it, the longer this takes, the more you need to apply this refactoring! Clean up the code, look for unused or duplicated pieces. Then, extract detail to smaller methods. Reveal intention through naming of new variables and methods. Once you are done, make sure all the code in the Composed Method is of the same level of detail.

Pros

  1. Readability
  2. Uniform level of detail.
  3. Joshua lists difficulty in debugging as a drawback, however, I think a stack trace that points to the smaller, intentionally named method will increase the likelihood that I can figure out what is wrong before I even crack open the code!
  4. Another benefit to add to Joshua’s list: code re-use of the extracted methods to replace other duplicated code.
Cons
  1. Excessive quantity of small methods, though Joshua points out this could lead to applying Extract Class.

So I rummaged around in some code and stumbled upon a text box validation method that was a great candidate for this refactoring:

public virtual void Validate()
{
  IsValid = true;
  if (Visible && Enabled)
  {
    if (IsRequired)
    {
      if (String.IsNullOrEmpty(Text) || Text.Trim().Length == 0)
      {
        ErrorMessage = String.Format(Messages.TextBoxCannotBeEmpty, InvalidDisplayName);
        IsValid = false;
      }
    }

if (MaxLength != 0 && !String.IsNullOrEmpty(Text) && Text.Trim().Length > MaxLength)
{
  ErrorMessage = String.Format(Messages.TextBoxExceedsMaximumLength, InvalidDisplayName, MaxLength);
  IsValid = false;
}

if (MinLength > 0 && (String.IsNullOrEmpty(Text) || Text.Trim().Length < MinLength))
{
  ErrorMessage = String.Format(Messages.TextBoxUnderMinimumLength, InvalidDisplayName, MinLength);
  IsValid = false;
}

} }

The level of nested conditionals makes it rather hard to follow. First, I want to remove some of the nesting by returning immediately if the validation isn’t applicable:

public virtual void Validate()
{
  IsValid = true;
  if (!Visible || !Enabled)
  {
    return;
  }
  if (IsRequired)
  {
    if (String.IsNullOrEmpty(Text) || Text.Trim().Length == 0)
    {
      ErrorMessage = String.Format(Messages.TextBoxCannotBeEmpty, InvalidDisplayName);
      IsValid = false;
    }
  }

if (MaxLength != 0 && !String.IsNullOrEmpty(Text) && Text.Trim().Length > MaxLength) { ErrorMessage = String.Format(Messages.TextBoxExceedsMaximumLength, InvalidDisplayName, MaxLength); IsValid = false; }

if (MinLength > 0 && (String.IsNullOrEmpty(Text) || Text.Trim().Length < MinLength)) { ErrorMessage = String.Format(Messages.TextBoxUnderMinimumLength, InvalidDisplayName, MinLength); IsValid = false; } }

Next, I notice 3 different “sub validations” occurring. One validates if the field is required, the next validates the maximum length and the last validates the minimum length. Each of these can become their own methods:

public virtual void Validate()
{
  IsValid = true;
  if (!Visible || !Enabled)
  {
    return;
  }

ValidateRequired(); ValidateMaximumLength(); ValidateMinimumLength(); } private void ValidateRequired() { if (IsRequired) { if (String.IsNullOrEmpty(Text) || Text.Trim().Length == 0) { ErrorMessage = String.Format(Messages.TextBoxCannotBeEmpty, InvalidDisplayName); IsValid = false; } } } private void ValidateMaximumLength() { if (MaxLength != 0 && !String.IsNullOrEmpty(Text) && Text.Trim().Length > MaxLength) { ErrorMessage = String.Format(Messages.TextBoxExceedsMaximumLength, InvalidDisplayName, MaxLength); IsValid = false; } } private void ValidateMinimumLength() { if (MinLength > 0 && (String.IsNullOrEmpty(Text) || Text.Trim().Length < MinLength)) { ErrorMessage = String.Format(Messages.TextBoxUnderMinimumLength, InvalidDisplayName, MinLength); IsValid = false; } }

Finally, I realize the level of detail isn’t uniform, I should extract a new property to check if the control is in use. Also, even more troublesome, the intentions of the first line to set the IsValid to true are lost to future developers. This is needed to reset the validation process, otherwise if a control is validated and fails, subsequent calls may still return invalid. My new Composed Method finally becomes:

public virtual void Validate()
{
  ResetValidation();
  if (!InUse)
  {
    return;
  }

ValidateRequired(); ValidateMaximumLength(); ValidateMinimumLength(); } private void ResetValidation() { IsValid = true; } private bool InUse { get { return Visible && Enabled; } } ...

The new Composed Method is much easier to comprehend. Ironically enough, looking at other validation methods, the newly extract methods can be re-used either through inheritance or possibly by applying Extract Class. The new InUse property may eventually be exposed for public consumption.





comments powered by Disqus