Blocks

  1. Do one thing.
  2. Do it well.
  3. Keep it concise.
  4. Use only what is given.
Today we continue from Comments and explore how to refactor out code from blocks.  In Comments, we learned to extract code out into it’s own method whenever we use a comment to describe a block of code.  Today, we are going to look at how to do the same thing with blocks.
A block of code is any code that exists within a if, a loop, or a switch statement.  A good rule of thumb is every time you indent your code.  Our goal is to keep indents inside your code from going beyond 2 levels.  Let’s look at a typical method where we break this rule.
public function createAction()
{
    $adminForm = new Form_User();
    $adminForm->removeElement('dealership_id');

    if ($this->getRequest()->isPost()) {
        if ($adminForm->isValid($_POST)) {
            $mAdmin = new Application_Model_DbTable_Users();
            $result = $mAdmin->createAdmin(
                $adminForm->getValue('name'),
                $adminForm->getValue('email'),
                $adminForm->getValue('password'),
                $adminForm->getValue('role')
            );

            if ($adminForm->getValue('notify') === 'yes') {
                if ($adminForm->getValue('notifyPhone') === 'yes') {
                    $notification = new Application_Model_DbTable_PhoneNotification($mAdmin);
                } else {
                    $notification = new Application_Model_DbTable_Notification($mAdmin);
                }
                $notification->sendNotification();
            }

            if ($result)
                $this->_forward ('confirm');
        }
    }

    $adminForm->setAction('/admin/create');
    $adminForm->setMethod('post');
    $this->view->form = $adminForm;
}

Before we start refactoring this, I want to cover a couple things.  First, the above is a small example, but it can still be used to explain our need.  Secondly, what we learn here can be applied to code, regardless of the size or complexity.

Complexity

Before we set out on this road, we need to define complexity.  Specifically, I’m referring to Cyclomatic Complexity.  The best definition of Cyclomatic Complexity that I’ve found is from the PHPMD page on Cyclomatic Complexity.

Complexity is determined by the number of decision points in a method plus one for the method entry. The decision points are ‘if’, ‘while’, ‘for’, and ‘case labels’. Generally, 1-4 is low complexity, 5-7 indicates moderate complexity, 8-10 is high complexity, and 11+ is very high complexity.

Our goal is to keep what we write in the low complexity category.  At this point, we should go through the sample code from above, and figure out how much complexity there is.

public function createAction()  // Complexity 1
{
    $adminForm = new Form_User();
    $adminForm->removeElement('dealership_id');

    if ($this->getRequest()->isPost()) {  // Complexity 2
        if ($adminForm->isValid($_POST)) {  // Complexity 3
            $mAdmin = new Application_Model_DbTable_Users();
            $result = $mAdmin->createAdmin(
                $adminForm->getValue('name'),
                $adminForm->getValue('email'),
                $adminForm->getValue('password'),
                $adminForm->getValue('role')
            );

            if ($adminForm->getValue('notify') === 'yes') {  // Complexity 4
                if ($adminForm->getValue('notifyPhone') === 'yes') {  // Complexity 5
                    $notification = new Application_Model_DbTable_PhoneNotification($mAdmin);
                } else {  // Complexity 6
                    $notification = new Application_Model_DbTable_Notification($mAdmin);
                }
                $notification->sendNotification();
            }

            if ($result)  // Complexity 7
                $this->_forward ('confirm');
        }
    }

    $adminForm->setAction('/admin/create');
    $adminForm->setMethod('post');
    $this->view->form = $adminForm;
}

A total of 7 complexity, on the high end of moderate complexity.  You might be looking at this code and wondering what makes it so complex.  It is simple enough.  It’s built to handle a form.  It checks input, creates a new user if the form was submitted in a valid format, and sets up notifications that were obviously a choice in the form.  It finally forwards off to the confirmation.

But, that is actually a lot of complexity.  To test this function, we’d need at least 7 test cases.  Even more so, this method violates some of our rules.  Do one thing.  It does many.  It’s not concise.  And it’s also not just using what is given.

We are going to focus on the complexity portion today.  We are going to bring down the size of this method step by step.

Indents are intents to modularize

Every time you indent your code, you should be thinking about modularizing.  In other words, if you indent your code into a block, you should consider moving that code into it’s own method.

Let’s start with the extremely large if in the middle there.  The one that checks to see if the request is a post.  When we indent that block of code inside, we should ask ourselves if that code inside can be better moved to it’s own method.  The answer is a resounding yes.  Within that block of code, we have 5 levels of complexity.  By moving all that complexity out of the createAction method, we will help decrease the complexity of that single method.  More so, we make that method easier to read, and we are able to compartmentalize the code even more.  We make our code even more reusable.

Where with comments we used the comment as an indicator of a new method, here we use the ideated block of code as an indicator.

public function createAction() // Complexity 1
{
    $adminForm = new Form_User();
    $adminForm->removeElement('dealership_id');

    if ($this->getRequest()->isPost()) { // Complexity 2
        $this->letFormHandleInput($adminForm);
    }

    $adminForm->setAction('/admin/create');
    $adminForm->setMethod('post');
    $this->view->form = $adminForm;
}

protected function letFormHandleInput ($adminForm) // Complexity 1
{
    if ($adminForm->isValid($_POST)) { // Complexity 2

        $mAdmin = new Application_Model_DbTable_Users();
        $result = $mAdmin->createAdmin(
            $adminForm->getValue('name'),
            $adminForm->getValue('email'),
            $adminForm->getValue('password'),
            $adminForm->getValue('role')
        );

        if ($adminForm->getValue('notify') === 'yes') { // Complexity 3
            if ($adminForm->getValue('notifyPhone') === 'yes') { // Complexity 4
                $notification = new Application_Model_DbTable_PhoneNotification($mAdmin);
            } else { // Complexity 5
                $notification = new Application_Model_DbTable_Notification($mAdmin);
            }

            $notification->sendNotification();
        }

        if ($result) // Complexity 6
            $this->_forward('confirm');
    }
}

Referring back to createAction, we see that the complexity has dropped to a mere 2 points.  Granted, letFormHandleInput is 6 points, but we aren’t done yet.  Each method should be easy to understand, and easy to test.  createAction is now incredibly easy to understand.  It creates a new form for display, and if it receives a post request, it let’s the form handle the input.

I’ll admit, I’m not too enamoured with the name of the new method, but it’s simple, and defines exactly what the method does.  More importantly, we’re also passing the form in.  This means that we can pass this method any form.  If something fails, we come out of the method, and continue with the display of the form.  Normally, I’d want to also define letFormhandleInput as

protected function letFormHandleInput(Zend_Validate_Interface $adminForm, array $data)

Here, $adminForm would be explicit in what it is, and I’d pass in the $_POST data, rather than calling it.  In the method itself, I’d swap out the calls to $adminForm->getValue, and instead call directly from $data.  However, this goes beyond our discussion here.  However, I make mention of this because those simple changes are not only easy to make, but value in terms of maintainability, testing, and longevity.

Because letFormHandleInput is still complex, we need to extract some methods out from it as well.  letFormHandleInput is actually wrapped directly around a giant if.  A simple check to see if the $_POST data is valid.  After that point, everything is indented. It also happens that the form doesn’t do much after that.  We grab data from it, but as I noted above, that’s easily removed. Regardless, the sign is fairly clear: indented blocks are what we are looking for.

public function createAction() // Complexity 1
{
    $adminForm = new Form_User();

    $adminForm->removeElement('dealership_id');

    if ($this->getRequest()->isPost()) { // Complexity 2
      $this->validateNewUserDataInput($adminForm, $_POST);
    }

    $adminForm->setAction('/admin/create');
    $adminForm->setMethod('post');
    $this->view->form = $adminForm;
}

protected function validateNewUserDataInput (Zend_Validate_Interface $adminForm, $data)  // Complexity 1
{
    if ($adminForm->isValid($data)) { // Complexity 2
        $this->addNewUser($data);
    }
}

protected function addNewUser ($data)  // Complexity 1
{
    $mAdmin = new Application_Model_DbTable_Users();
    $result = $mAdmin->createAdmin(
        $data['name'],
        $data['email'],
        $data['password'],
        $data['role']
    );

    if ($data['notify'] === 'yes') {  // Complexity 2
        if ($data['notifyPhone'] === 'yes') {  // Complexity 3
            $notification = new Application_Model_DbTable_PhoneNotification($mAdmin);
        } else { // Complexity 4
            $notification = new Application_Model_DbTable_Notification($mAdmin);
        }

        $notification->sendNotification();
    }

    if ($result) // Complexity 5
        $this->_forward('confirm');
}

Once again, we’ve reduced complexity, and in our resulting method, our complexity has gone down.  As well, we’ve also remove the task of creating a new user to another method.  addNewUser does exactly that, it adds a new user and all it’s associate baggage.  I also took this time to better define what we needed, and what we didn’t.  You’ll see I’m pulling directly from $data now.  No need to pass $adminForm around.  Also, letFormHandleInput only requires a Zend_Validate_Interface object now, further decoupling ourselves.  I also took the liberty of renaming letFormHandleInput to validateNewUserDataInput.  This name better describes exactly what it does.  Granted, it also goes directly to creating a new user, but that’s something for another day.

Back on topic, lets take the next step, and refactor addNewUser in the same manner we’ve been.  Let’s see, we have another block of code.  This block sets up notifications.

protected function addNewUser ($data) // Complexity 1
{
    $mAdmin = new Application_Model_DbTable_Users();
    $result = $mAdmin->createAdmin(
        $data['name'],
        $data['email'],
        $data['password'],
        $data['role']
    );

    $this->setupNotificationsForUser($data, $mAdmin);

    if ($result) // Complexity 2
        $this->_forward('confirm');
}

protected function setupNotificationsForUser (array $data, Application_Model_DbTable_Users $mAdmin)  // Complexity 1
{
    if ($data['notify'] === 'yes') { // Complexity 2
        if ($data['notifyPhone'] === 'yes') { // Complexity 3
            $notification = new Application_Model_DbTable_PhoneNotification($mAdmin);
        } else { // Complexity 4
            $notification = new Application_Model_DbTable_Notification($mAdmin);
        }

        $notification->sendNotification();
    }
}

In this case, I just decided to encapsulate the entire notification code into it’s own method.  Here now, setupNotificaitonsForUser can be passed proper data, and from that, we can setup notifications for a users outside the creation of a new user.  This means when you write your code for editing a users notification preferences, you already have your notification setup.  Complexity is at 4, but that brings us to the low category of complexity.  There is one more thing we can do to simplify this code.  However, by this point, you should already see what we can do, and how it will simplify the code even further.

By extracting out the second if/else clause in setupNotificationsForUser, we abstract away the creation process.

All these methods can be applied to other forms of blocks of code.  Every case in a switch should probably have its own method.  Any loop should call out to a method.  At every level of complexity, you want to keep things as simple as possible.  When you read a block of code, it should be simple.  It should do something specific.  It should do is quickly, and it should do it well.

Comments

I once posted  “comments indicate future refactoring.” I want to reaffirm my belief in that, and clearly explain. In order to fully appreciate this, let’s lay down some fundamental beliefs. These are beliefs I have built on experience, and later, I’ll apply them to more general fundamental beliefs like SOLID, but for now, these are simple rules I feel are good to follow.

  1. Do one thing.
  2. Do it well.
  3. Keep it concise.
  4. Use only what is given.

These aren’t overly complex rules. In fact, they are pretty simple. We are going to aim to meet these rules in a rather simple manner: using comments.

Now, let’s define what type of comments I’m referring to. I’m not talking about DocBlock comments. These are comments that allow for parsing by specific tools and allow us to generate documents about the code.

I’m also not referring to comments that repeat the code.

// increment a by 1
$a++;

That is a poor comment, and should be removed. It does nothing except add to confusion.

The comments I’m referring to, however, are a bit more involved. But let’s start with an exercise.

Code should explain what it does. It doesn’t always do this. For example, what’s the purpose of the following code?

if ( preg_match(EMAIL_REGEX, $input) || 
     is_numeric($input) || 
     preg_match(USERNAME_REGEX, $input) ) { /* ... */ }

It’s easy to see what the code is doing, but not the purpose. If we add a comment, it will help matters.

// Check to see if the user input matches one of the searchable data sets
if ( preg_match(EMAIL_REGEX, $input) ||
     is_numeric($input) || 
     preg_match(USERNAME_REGEX, $input) ) { /* ... */ }

Hey! That helps. The code checks to see if the user input matches a pattern for searching a data set. Okay, that helps. But it’s still problematic. The comment isn’t clear. Is it referring to just the if-clause, or is it referring to the block inside the if-statement. Can you trust the comment? After all, as time progresses and code enters maintenance phase, things can and will change. Is it easy to read? It’s not difficult, but when you are read the comment, you then have to mentally associate that with the code. It takes extra effort. And for what? Saying something explicit.

Maybe it’s better that we ask a simple question: What is the comment trying to tell us?

If the input matches a searchable data set, run the blocked code.

Luckily, we can say that succinctly.

if ( $this->inputMatchesSearchableData( $input ) ) { /* ... */ }

// ...
function inputMatchesSearchableData ( $input )
{
    return preg_match(EMAIL_REGEX, $input) ||
           is_numeric($input) ||
           preg_match(USERNAME_REGEX, $input);
}

Using the extra method refactoring recipe, we’ve made our code much, much easier to read. inputMatchesSearchableData says exactly what the method does, and in our original block of code, it reads much easier than the comment did. We’ve also refactored the code that checks to see if the input is searchable. We’ve split responsibility, and suddenly, our original method does one less thing.

Every time I use a comment to explain what code is doing, I always ask myself if it’s code that should be removed. If what the code is doing isn’t directly related to the parent method, the answer is most likely yes.

Often times, the comment itself is a good indicator of the name of the function. Extract the method out, and keep each method small, precise, and simple.