Functions

How Functions Should be Defined

Lets consider the following code:

public static String testableHtml(
PageData pageData,
boolean includeSuiteSetup
) throws Exception {
  WikiPage wikiPage = pageData.getWikiPage();
  StringBuffer buffer = new StringBuffer();
  if (pageData.hasAttribute("Test")) {
    if (includeSuiteSetup) {
      WikiPage suiteSetup =
        PageCrawlerImpl.getInheritedPage(
        SuiteResponder.SUITE_SETUP_NAME, wikiPage
        );
      if (suiteSetup != null) {
        WikiPagePath pagePath =
        suiteSetup.getPageCrawler().getFullPath(suiteSetup);
        String pagePathName = PathParser.render(pagePath);
        buffer.append("!include -setup .")
          .append(pagePathName)
          .append("\n");
      }
    }
    WikiPage setup =
      PageCrawlerImpl.getInheritedPage("SetUp", wikiPage);
    if (setup != null) {
      WikiPagePath setupPath =
      wikiPage.getPageCrawler().getFullPath(setup);
      String setupPathName = PathParser.render(setupPath);
      buffer.append("!include -setup .")
        .append(setupPathName)
        .append("\n");
    }
  }
  buffer.append(pageData.getContent());
  if (pageData.hasAttribute("Test")) {
    WikiPage teardown =
    PageCrawlerImpl.getInheritedPage("TearDown", wikiPage);
    if (teardown != null) {
      WikiPagePath tearDownPath =
        wikiPage.getPageCrawler().getFullPath(teardown);
      String tearDownPathName = PathParser.render(tearDownPath);
      buffer.append("\n")
        .append("!include -teardown .")
        .append(tearDownPathName)
        .append("\n");
    }
    if (includeSuiteSetup) {
      WikiPage suiteTeardown =
        PageCrawlerImpl.getInheritedPage(
        SuiteResponder.SUITE_TEARDOWN_NAME,
        wikiPage
        );
      if (suiteTeardown != null) {
        WikiPagePath pagePath =
          suiteTeardown.getPageCrawler().getFullPath (suiteTeardown);
        String pagePathName = PathParser.render(pagePath);
        buffer.append("!include -teardown .")
          .append(pagePathName)
          .append("\n");
      }
    }
  }
  pageData.setContent(buffer.toString());
  return pageData.getHtml();
}

This is quite a convoluted function from which we can extract a few keypoints of things we should enforce when defining functions.

The first rule of functions is that they should be small.
The indent level should not be greater than 1 or 2.
A function should do one thing and one thing only. We say the function does one thing by abstracting all of the inner steps.
The statements within a function should be all on the same level of abstraction.
Every function should be defined so the program can be read from top to bottom. That is:

1 To print a string I have to create the string, show the string on the screen.
1.1 To create a string i have to create an array of characters and append each character
1.1.1 To create an array of characters i have to allocate memory
1.1.2 To append a character i have to find on memory the place i want to save it to and save its ascii
1.2. To show a string on the screen i have to show each character on the screen
1.2.1. To show a character on the screen i need to know its bitmap and print it on the screen
...

Use descriptive names that say exactly what the function does.
A function should have zero arguments. They take a lot of conceptual power as they are on a different level of abstraction and they also make testing harder.
Output arguments are harder to understand than input arguments as we usually do not expect arguments to be modified, so it is best to steer clear of them. If your function must change the state of something, have it change the state of its owning object.
Avoid flag arguments as these booleans make the function do one thing if they are true and another if they are false. That is now our function does two things.
Use the keyword form of a function name, where we encode the names of the arguments into the function name. For example instead of assertEquals we define assertExpectedEqualsActual.
Avoid having side effects, where the function seems to do one thing but is also does some other hidden thing (e.g. updating an instance variable).
Apply the Command Query Separation: functions should either change the state of an object, or it should return some information about that object. Doing both often leads to confusion.
Prefer returning exceptions to error codes: When you return an error code, you create the problem that the caller must deal with the error immediately.
Try/catch blocks confuse the structure of the code and mix error processing with normal processing. Functions should do one thing. Error handing is one thing. So it is better to extract the bodies of the try and catch blocks out into functions of their own.

public void delete(Page page) {
  try {
    deletePageAndAllReferences(page);
  }
  catch (Exception e) {
    logError(e);
  }
}
private void deletePageAndAllReferences(Page page) throws Exception {
  deletePage(page);
  registry.deleteReference(page.name);
  configKeys.deleteKey(page.name.makeKey());
}

Switch Statements

Switch statements it violates our previous rule about functions doing only one thing, most usually swith function do as many things as clauses it has. Also they may grow whenever a new case is added. In order to avoid this it is recommended swith statements be buried low on the abstraction hierarchy.

For example, instead of creating a function that computes the pay with regards of the type of employee:

public Money calculatePay(Employee e)
  throws InvalidEmployeeType {
    switch (e.type) {
    case COMMISSIONED:
      return calculateCommissionedPay(e);
    case HOURLY:
      return calculateHourlyPay(e);
    case SALARIED:
      return calculateSalariedPay(e);
    default:
      throw new InvalidEmployeeType(e.type);
  }
}

We, instead, define an abstract factory that creates one type of employee or another, and where each type of employee decides how calculatePay is computed with polymorphism.

public abstract class Employee {
  public abstract boolean isPayday();
  public abstract Money calculatePay();
  public abstract void deliverPay(Money pay);
}

public interface EmployeeFactory {
  public Employee makeEmployee(EmployeeRecord r) throws InvalidEmployeeType;
}
public class EmployeeFactoryImpl implements EmployeeFactory {

  public Employee makeEmployee(EmployeeRecord r) throws InvalidEmployeeType {
    switch (r.type) {
      case COMMISSIONED:
        return new CommissionedEmployee(r) ;
      case HOURLY:
        return new HourlyEmployee(r);
      case SALARIED:
        return new SalariedEmploye(r);
      default:
        throw new InvalidEmployeeType(r.type);
    }
  }
}

Meaningful Names Comments