Clean code — Functions(p1)

Right now I’m reading for the second time, the great book Clean Code by Robert C. Martin aka Uncle Bob and I think it’s a good idea to write down the most important ideas that it tries to convey to us. Today, I will focus on the third chapter.

A function has to be small

The first rule of functions is that they should be small. The second rule of functions is that they should be smaller than that. Functions should hardly ever be 20 lines long.

public static String renderPageWithSetupsAndTeardowns(PageData pageData, boolean isSuite)throws Exception{
   if(isTestPage(pageData)) includeSetupAndTeardownPages(pageData, isSuite);
   return pageData.getHtml();
}

Blocks and Indenting

This implies that the blocks within if statements, else statements, while statements, and so on should be one line long. Probably that line should be a function call. Not only does this keep the enclosing function small, but it also adds documentary value because the function called within the block can have a nicely descriptive name.

Do one thing

Functions should do one thing. They should do it well. They should do it only.

The problem with this statement is that it is hard to know what “one thing” is. It's easy to make the case that it's doing three things:

1. Determining whether the page is a test page.
2. If so, including setups and teardowns.
3. Rendering the page in HTML.

So which is it? Is the function doing one thing or three things? Notice that the three steps of the function are one level of abstraction below the stated name of the function. We can describe the function by describing it as a brief TO paragraph.

After all, the reason we write functions is to decompose a larger concept (in other words, the name of the function) into a set of steps at the next level of abstraction.

So, another way to know that a function is doing more than “one thing” is if you can extract another function from it with a name that is not merely a restatement of its implementation.

One level of Abstraction per Function

Mixing levels of abstraction within a function is always confusing. Readers may not be able to tell whether a particular expression is an essential concept or a detail.

Switch Statements

It's also hard to make a switch statement that does one thing. By their nature, switch statements always do N things. Unfortunately, we can't always avoid switch statements, but we can make sure that each switch statement is buried in a low-level class and is never repeated. We do this, of course, with polymorphism.

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);
   }
}

There are several problems with this function. First, it's large, and when new employee types are added it will grow. Second, it very clearly does more than one thing. Third, it violates the Single Responsibility Principle because there is more than one reason for it to change. Fourth, it violates the Open-Closed Principle because it must change whenever new types are added. But possibly the worst problem with this function is that there is an unlimited number of other functions that will have the same structure. For example, we could have

isPayday(Employee e, Date date);

or

deliverPay(Employee e, Money pay);

The solution to this problem is to bury the switch statement in the basement of an Abstract Factory and never let anyone see it. My general rule for switch statements is that they can be tolerated if they appear only once, are used to create polymorphic objects.

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{

   @Override
   public Employee makeEmployee(EmployeeRecord e) throws invalidEmployeeType {
      switch (e.type){
         case COMMISSIONED:
            return new CommissionedEmployee(e);
         case HOURLY:
            return new HourlyEmployee(e);
         case SALARIED:
            return SalariedEmployee(e);
         default:
            throw new InvalidEmployeeType(e.type);
      }
   }
}

Conclusions

The concept behind this idea is to simply build easy-to-read software and for this to happen, we have to make code with descriptive names and too small. One concept per function. You should use auxiliary functions with different levels of abstractions. If you build this type of code, you don’t need to spend time creating documentation. That is the point.