Refactoring - A first example » Debuggable

Refactoring - A first example

Today we will look at a first php example of refactoring, to start creating a sense for it.

Think about the following scenario: You want to dynamically add workers to a department and then display that department's workers as an unordered list. Pretty easy eh? However, we will discover that there are some pitfalls and ways to improve from our initial code. We'll start off with the following:

class Department {
protected $name;
protected $workers = array();

public function addWorker($name,$isChief,$salary) {
$this->workers[] = array($name,$isChief,$salary);
}

public function listWorkers() {
echo '<ul>';
foreach($this->workers as $worker) {
echo "<li>{$worker[0]} - {$worker[1]} - {$worker[2]}";
}
echo '</li></ul>';
}
}

$dept = new Department;
$dept->addWorker('Felix Broda',1,2000);
$dept->addWorker('Stefan Muster',0,1400);
$dept->addWorker('Klaus Schmidt',0,1350);
$dept->listWorkers();

There are a couple of really bad Code Smellsin it, unfortunately.

Our first refactoring

The first thing that comes to mind is the very bad way of storing the workers in the Department class. I mean an array is by all means fine. However, look at the 13th line: We will always depend on our implementation and have to go to the addWorker function to remember the order in which we stored the data. How can we fix this? Introduce a class for a Worker:

class Worker {
protected $name;
protected $isChief = false;
protected $salary = 0;
}

Now we can change the addWorker method of the Department class:

public function addWorker($worker) {
$this->workers[] = $worker;
}

Okay, so we can change the client code now in the listWorkers() method:

public function listWorkers() {
echo '<ul>';
foreach($this->workers as $worker) {
echo "<li>{$worker->name} - {$worker->isChief} - {$worker->salary}";
}
echo '</li></ul>';
}

Okay looking good now. :)

Our second refactoring

When executing the code we see that the attributes of the Worker class must be public in order to work. A quick refactoring, let's fix it up:

class Worker {
public $name;
public $isChief = false;
public $salary = 0;
}

Making our list standards-compliant

There are actually two smaller problems now with our list generation: We are missing the closing list-tags:

echo "<li>{$worker->name} - {$worker->isChief} - {$worker->salary}</li>";

Okay this took us 5 seconds, but it was worth it. Now we notice that when we are outputting a department-list before adding workers to it, we get an unordered list that has starting ul-tags, but no li-tags. This is not standards-compliant, so we fix it:

public function listWorkers() {
if(count($this->workers) > 0) {
echo '<ul>';
foreach($this->workers as $worker) {
echo "<li>{$worker->name} - {$worker->isChief} - {$worker->salary}";
}
echo '</li></ul>';
} else {
echo 'Sorry, there are no workers to display.';
}
}

Switching hats again - adding a feature

Switching hats now, because our customer told us, that he wants the number of department workers displayed at the end of each list. We are wearing our feature-hat now. This feature is a quick thing, so we implement it right away:

public function listWorkers() {
if(count($this->workers) > 0) {
echo '<ul>';
foreach($this->workers as $worker) {
echo "<li>{$worker->name} - {$worker->isChief} - {$worker->salary}";
}
echo '</li></ul>';
echo "There are {count($this->workers} workers in this department.";
} else {
echo 'Sorry, there are no workers to display.';
}
}

More refactoring

Looking at our code now we use the count() function twice to receive the same bit of information. Now we could call count() once and store its result in a variable. However, that would tie our interface to the implementation. Let's add a new method instead that counts the number of workers for us:

public function numWorkers() {
return count($this->workers);
}

Our altered listWorkers() method looks as follows:

public function listWorkers() {
$numWorkers = $this->numWorkers();

if($numWorkers > 0) {
echo '<ul>';
foreach($this->workers as $worker) {
echo "<li>{$worker->name} - {$worker->isChief} - {$worker->salary}";
}
echo '</li></ul>';
echo "There are {$numWorkers} workers in this department.";
} else {
echo 'Sorry, there are no workers to display.';
}
}

Cool stuff. :) Let's see what we currently have:

class Department {
protected $name;
protected $workers = array();

public function addWorker($worker) {
$this->workers[] = $worker;
}

public function listWorkers() {
$numWorkers = $this->numWorkers();

if($numWorkers > 0) {
echo '<ul>';
foreach($this->workers as $worker) {
echo "<li>{$worker->name} - {$worker->isChief} - {$worker->salary}";
}
echo '</li></ul>';
echo "There are {$numWorkers} workers in this department.";
} else {
echo 'Sorry, there are no workers to display.';
}
}

public function numWorkers() {
return count($this->workers);
}
}

class Worker {
public $name;
public $isChief = false;
public $salary = 0;
}

Adding some client code into the mix

$felix = new Worker;
$felix->name = 'Felix Broda';
$felix->isChief = true;
$felix->salary = 2000;

$stefan = new Worker;
$stefan->name = 'Stefan Muster';
$stefan->salary = 1400;

$klaus = new Worker;
$klaus->name = 'Klaus Schmidt';
$klaus->salary = 1350;

$dept = new Department;
$dept->addWorker($felix);
$dept->addWorker($stefan);
$dept->addWorker($klaus);
$dept->listWorkers();

Now it's all working well, but darn that client code is so much for what it does. Let's refactor a bit more and add a handy constructor to the Worker class:

class Worker {
public $name;
public $isChief = false;
public $salary = 0;

public function __construct($name,$salary,$isChief=false) {
$this->name = $name;
$this->salary = $salary;
$this->isChief = $isChief;
}
}

Note that we put the isChief variable as an optional parameter with the default value of false. This is handy, because most workers will not be chiefs, leaving us without the need to explicitely tell every new worker object that it is not a chief.

Now our new client code:

$felix = new Worker('Felix Broda',2000,true);
$stefan = new Worker('Stefan Muster',1400);
$klaus = new Worker('Klaus Schmidt',1350);

$dept = new Department;
$dept->addWorker($felix);
$dept->addWorker($stefan);
$dept->addWorker($klaus);
$dept->listWorkers();

Ah many less lines - much better. :)

Shortening the code more

Let's add the ability to add an array of workers all at once. Our goal is to make the following line valid:

$dept->addWorker(array($felix,$stefan,$klaus));

Our altered addWorker() method:

public function addWorker($worker) {
if(is_array($worker)) {
foreach($worker as $w)
$this->workers[] = $w;
} else {
$this->workers[] = $worker;
}
}

Now was this a feature or a refactoring?

Adding tax rates

Our client wants us to display the actual salaries of all workers of a department. We add the feature in our listWorkers() method:

public function listWorkers() {
$numWorkers = $this->numWorkers();

if($numWorkers > 0) {
echo '<ul>';
foreach($this->workers as $worker) {
$salary *= 1.07;
echo "<li>{$worker->name} - {$worker->isChief} - {$salary}";
}
echo '</li></ul>';
echo "There are {$numWorkers} workers in this department.";
} else {
echo 'Sorry, there are no workers to display.';
}
}

However, we just introduced another Code Smell. We should not hard code the calculation of the sales tax into our client code. If the calculation changes we would have to adjust all client code that depends on it. Let's make the overall flow by introducing a method in the Worker class clearer:

public function calcTotalSalary() {
return round($this->salary * 1.07,2);
}

Now we can rewrite listWorkers() as follows:

public function listWorkers() {
$numWorkers = $this->numWorkers();

if($numWorkers > 0) {
echo '<ul>';
foreach($this->workers as $worker) {
echo "<li>{$worker->name} - {$worker->isChief} - {$worker->calcTotalSalary()}";
}
echo '</li></ul>';
echo "There are {$numWorkers} workers in this department.";
} else {
echo 'Sorry, there are no workers to display.';
}
}

Introducing a constant for the tax rate

Now tax rates change and our code should account for them. Let's introduce a constant for the tax rate to save us a lot of trouble changing client code later when the tax rate changes:

define('TAX_RATE','17');
class Worker {
public $name;
public $isChief = false;
public $salary = 0;

public function __construct($name,$salary,$isChief=false) {
$this->name = $name;
$this->salary = $salary;
$this->isChief = $isChief;
}

public function calcTotalSalary() {
return ($this->salary + TAX_RATE/100 * $this->salary);
}
}

Conclusion

As you see when you refactor you make little changes to code that is working already, but that can be improved to quite some extent. Hopefully you noticed how we switched hats often in order to
reach to some very cool and working code. It often takes only fundamental sense of architecture to get to clean code that works. Please keep in mind that you do not add a feature and refactor at the same time, as that will lead you to the road to hell.

Happy refactoring. :)

&nsbp;

You can skip to the end and add a comment.

John said on May 01, 2007:

Thanks for this nice tutorial. :)

Rick Jayne said on May 01, 2007:

Yeah, I especially like the throurough code examples.

Good tutorial.

Raphael Stolt said on May 03, 2007:

Finally somebody writing some thoughts and insights on refactoring in PHP. I think you should have mentioned at least to run the according developer test after every large refactoring step, to prove the behaviour preservation. Thanks for posting on this topic.

Tim Koschuetzki said on May 04, 2007:

Hi Raphael,

yes you are right. I could have highlighted this part of refactoring a bit more. Thanks for the tip.

Oscar said on May 11, 2007:

I like the tutorial. It is to bad that your website is not displaying correctly on a Mac using Safari. It makes it hard to read.

Tim Koschuetzki said on May 15, 2007:

Hi Oscar, thanks for your comment.

I will work on it. :) Promise!

This post is too old. We do not allow comments here anymore in order to fight spam. If you have real feedback or questions for the post, please contact us.

debuggable