Another look at the evolution of the ugly duckling or spaghetti refactoring

I am very enthusiastic about any attempts to improve the overall development culture in such an ambiguous community as the PHP developer community. But seeing some initiatives, I want to kill myself against the wall a little to fix them, so that God forbid you don’t have to work with a code that is very different from my ideas about the perfect code.

Today, for some reason, I could not resist seeing the topic From the ugly duckling to the swan, or how to fix the curved code and decided to fix it in my own way. Moreover, the author asked for references to other solutions.


Let me remind you that the task is to comb the code

<h1></h1> <? $DB = new DBConnector; $DB->query('SELECT * FROM users LIMIT 10'); if($DB->get_num_rows()){ while($user = $DB->fetch_row()){ echo '<p>'.$user['name'].'</p>'; } } ?> 

')
From what, in my opinion, refactoring should begin - with covering the code that we are going to refactor with tests. Without this, we cannot be sure that we are doing everything correctly and the code behavior does not change. As an example is a training one, it will not process special cases (there is no database, etc.) and we will assume that the code displays the names of four users from the database: Username1, Username2, ...

We write a simple test directly in PHP without using frameworks like PHPUnit.

 <?php $expected = <<<'EOT' <h1></h1> <p>Username1</p><p>Username2</p><p>Username3</p><p>Username4</p> EOT; ob_start(); include 'index.php'; $actual = ob_get_clean(); echo $actual === $expected ? 'OK' : 'Failed', PHP_EOL; 

Run ... and get the error "compilation" due to the absence of the class DBConnector. Without further ado, we write a stub for it (on a “combat” refactoring, we would have to plug it in, create test tables, etc., but the post is not about testing methods). Something like this I present the work of the original DBConnector class :)

 <?php class DBConnector { private $users; public function query($query) { for ($i=1; $i<=4; $i++) { $this->users[] = array('name' => 'Username' . $i); } } public function get_num_rows() { return count($this->users); } public function fetch_row() { $each = each($this->users); return $each['value']; } } 

Again, we run the test - we get OK. We fixed the behavior of the script, now if we narefakorim something wrong, we immediately get Failed. Let me remind you that in real refactoring we will need to cover all possible execution streams with tests, say an exception or errors when working with a database. Immediately limited to one.

We look at our code. The first thing that personally catches my eye is that the title, the query to the database, the output are all mixed up. Resolved, divided into receiving records and output html.

 <?php include 'DBConnector.php'; $DB = new DBConnector; $DB->query('SELECT * FROM users LIMIT 10'); if($DB->get_num_rows()){ while($user = $DB->fetch_row()){ $users[] = $user; } } else { $users = array(); } ?> <h1></h1> <? foreach ($users as $user) { echo '<p>'.$user['name'].'</p>'; } ?> 

Run the test - OK, nothing is broken. For convenience, we divide into two files, in other words, we put everything related to html into a separate index.php.html file:

 <?php // index.php include 'DBConnector.php'; $DB = new DBConnector; $DB->query('SELECT * FROM users LIMIT 10'); if($DB->get_num_rows()){ while($user = $DB->fetch_row()){ $users[] = $user; } } else { $users = array(); } include 'index.php.html'; 

 <h1></h1> <? // index.php.html foreach ($users as $user) { echo '<p>'.$user['name'].'</p>'; } ?> 

Check - OK. Some smart people are what we have in the new file now called the view or view. But we forget about him. Although no, let's make it a bit prettier (in taste and color ...).

 <h1></h1> <?php foreach ($users as $user): ?> <p><?=$user['name']?></p> <?php endforeach; ?> 

Run the test - Failed! We changed the presentation and the test fails. But we know that in HTML the spaces are not significant and add them to the test (never do that! :)). Now everything is OK. We return to our script. It is striking that it is highly dependent on the database. And it is possible that in other scripts of our application such samples are repeated frequently. And in general, somehow all these $ DB flicker in the eyes and you need to figure out what they are doing. Well, we follow DRY as it were and put the work with the users table into a separate method of a separate class; we will transfer the connection to the database through the constructor, and not the global variable. Let's call a class, well, let's say, a UserRepository.

 <?php class UserRepository { private $db_connector; public function __construct(DBConnector $db_connector) { $this->db_connector = $db_connector; } public function getAll($limit=10) { $this->db_connector->query("SELECT * FROM users LIMIT {$limit}"); if($this->db_connector->get_num_rows()){ while($user = $this->db_connector->fetch_row()){ $users[] = $user; } } else { $users = array(); } return $users; } } 

The code is moved almost unchanged, only the limit parameter is added. Automatically added: (in a good way it was impossible. But the change is simple and we hope that nothing will break. But first we need to change our script

 <?php require_once 'DBConnector.php'; require_once 'UserRepository.php'; $user_repository = new UserRepository(new DBConnector); $users = $user_repository->getAll(); include 'index.php.html'; 

Test? OK! (At each step of refactoring, it is necessary to run tests so that later it would not hurt for aimlessly lived years in search of "where did they screw it up"). If we discard the "service" require_once, then the file of our script has been reduced to three lines: create a repository, get all users from it, display them in a view. Hedgehog, probably it will be clear at first sight. Smart people call such a script controller, and thin. Well it is, for reference.

Take another look at our repository. No one has a dissonance - have they created the class “User Repository”, whose objects return an array of associative arrays (aka hashes)? I have arises. Let it return at least an array of objects with the original name User.

  while($user = $this->db_connector->fetch_row()){ $users[] = new User($user); } 

Well, the User class itself, which smart people call the domain model class or just the model class.

 <?php class User { private $name; public function __construct(array $properties) { $this->name = $properties['name']; } public function getName() { return $this->name; } } 

Change the view to work with an array of objects

 <h1></h1> <?php foreach ($users as $user): ?> <p><?= $user->getName() ?></p> <?php endforeach; ?> 

and add the model class declaration to the controller.

 <?php require_once 'DBConnector.php'; require_once 'User.php'; require_once 'UserRepository.php'; $user_repository = new UserRepository(new DBConnector); $users = $user_repository->getAll(); include 'index.php.html'; 

Run the test - everything is OK! Perhaps this can still stop. Let's sum up:

- The behavior of the script is almost guaranteed not changed. Apart from a few problems, the appearance of which we noticed and documented. Where? In the test! The test is also the documentation for the main code.

- our presentation is separated from everything else, all that it needs is for the array of users from objects with the getName () method to be in its scope. We can test it separately.

- complex (hehe) work with the database is encapsulated in the repository, which does not distract from the study of the application logic, but the connection itself is created outside of it, which makes it possible to a) substitute connections with different databases and b) substitute truly fake ones (stubs and moki) instances of connections for testing and even c) changing almost nothing to use any other storage - files, NoSQL DBMS and even cloud file hosting.

- model objects in fact do not depend on the database at all, and indeed from anything, these are the so-called POPO - Plain Old PHP Objects (flat old PHP objects). So far, in fact, there is no logic in them, but when it appears it can also be tested separately from the rest.

- the work of the main script is almost obvious, only the latter include almost nothing says, but I was already too lazy to select it as a function / method

What else can be done with the code to improve the complexity of the architecture:

- to abstract from the database even more, or even in general the type of storage.

- to abstract from the type of presentation (our include) - it can be done so that it can be rendered without any problems by some template maker - Smarty, Twig, etc.

- make the controller also an object of class

- use the FrontController pattern

- without any difficulty to screw other third-party components (ORM, routers, configs, etc.)

- translate the code into a framework, for example Symfony2 :)

- cover with normal tests, from modular (unit) to acceptance.

If there is interest, then this may be the first article of a small cycle. Only at first it would not hurt to complicate the initial example of a govnod to something more or less working and at least a little non-obvious and shit :) If you want one, then a rep on a github github.com/VolCh/refactor-sample