Clean Code – Function

Cost of Owning a Mess

If you have been a programmer for more than two or three years, you have probably been significantly slowed down by someone else’s messy code. If you have been a programmer for longer than two or three years, you have probably been slowed down by messy code. The degree of the slowdown can be significant. Over the span of a year or two, teams that were moving very fast at the beginning of a project can find themselves moving at a snail’s pace. Every change they make to the code breaks two or three other parts of the code. No change is trivial. Every addition or modification to the system requires that the tangles, twists, and knots be “understood” so that more tangles, twists, and knots can be added. Over time the mess becomes so big and so deep and so tall, they can not clean it up. There is no way at all.

What is Clean Code?

Functions

Sample Function

Read through this function and let me know what you understand.

Do you understand the function after five minutes of study? Probably not. There’s too much going on in there at too many different levels of abstraction.

However, with just a few simple method extractions, some renaming, and a little
restructuring the above will look like this.

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.

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. This also implies that functions should not be large enough to hold nested structures. Therefore, the indent level of a function should not be greater than one or two. This, of course, makes the functions easier to read and understand.

DO ONE THING

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

SWITCH STATEMENTS

It’s hard to make a small switch statement. Even a switch statement with only two cases is larger than I’d like a single block or function to be. 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.

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 Principle7 (SRP) because there is more than one reason for it to change. Fourth, it violates the Open-Closed Principle8 (OCP) 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. The factory will use the switch statement to create appropriate instances of the derivatives of Employee, and the various functions, such as calculatePay, isPayday, and deliverPay, will be dispatched polymorphically through the Employee interface.

OTHER THINGS TO BE TAKEN CARE OF

Extract Try/Catch Block

Try/catch blocks are ugly in their own right. They confuse the structure of the code and
mix error processing with normal processing. So it is better to extract the bodies of the try
and catch blocks out into functions of their own.

For Example:

public void delete(Page page) 
{
 try 
 {
    deletePageAndAllReferences(page);
 }
 catch(Exception e)
 {
   logError(e);
 }
}

DRY (Don’t Repeat Yourself)

Look at the code below and figure out if we are breaking the DRY principle?

class CsvValidation
{
    public function validateProduct(array product)
    {
        if (product['color']==null) {
            throw new Exception('Import fail: the product attribute color is missing');
        }

        if (product['size']==null) {
            throw new Exception('Import fail: the product attribute size is missing');
        }

        if (product['type']==null) {
            throw new Exception('Import fail: the product attribute type is missing');
        }
    }
}

Implementing DRY Principle

class CsvValidation
{
    private productAttributes = [
        'color',
        'size',
        'type',
    ];

    public function validateProduct(array product)
    {
        foreach (attributes in productAttributes) {
            if ((product[attribute]==null)) {
                throw new Exception(sprintf($'Import fail: the product attribute {attribute} is missing'));
            }
        }
    }
}

Naming Varibales

• Use Intention Revealing Names
  
  Wrong Naming : int eTD; // elapsed time in days
  Correct Naming: int elapsedTimeInDays;

• Use Searchables Names

LAST IMPORTANT RULE

The Boy Scout Rule

Leave the campground cleaner than you found it

References: Clean Code (Robert C. Martin Series)