- Do one thing.
- Do it well.
- Keep it concise.
- 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.