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

Today, for some reason, I could not restrain myself from seeing the topic From an ugly duckling into a swan, or how to fix a curved-handed code and decided to fix it in our own way. Moreover, the author asked for links to other solutions.


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

Пользователи
query(‘SELECT * FROM users LIMIT 10’);
if($DB->get_num_rows()){
    while($user = $DB->fetch_row()){
        echo ‘
’.$user[‘name’].’
’;
    }
}
?>

Where, in my opinion, refactoring should begin - with test coverage of the code that we are going to refactor. Without this, we cannot be sure that we are doing everything correctly and the behavior of the code does not change. Since the example is a training one, it will not handle special cases (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 to PHP without using frameworks such as PHPUnit.

Пользователи
Username1
Username2
Username3
Username4
EOT;
ob_start();
include 'index.php';
$actual = ob_get_clean();
echo $actual === $expected ? 'OK' : 'Failed', PHP_EOL;

We start ... and we receive an error of "compilation" due to the absence of the DBConnector class. Without further ado, we write a stub for it (in a “battle” refactoring I would have to connect it, create test tables, etc., but the post is not about testing methods). This is how I present the work of the original DBConnector class :)

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'];
    }
}

Run the test again - get OK. We recorded the behavior of the script, now if we refactor something wrong, we immediately get Failed. Let me remind you that in this refactoring we will need to cover with tests all possible execution threads, say an exception or errors when working with the database. Immediately limited to one.

We look at our code. The first thing that strikes me personally is that the header is mixed up, the database query, the output. Solved, share on receiving records and html output.

query('SELECT * FROM users LIMIT 10');
if($DB->get_num_rows()){
    while($user = $DB->fetch_row()){
        $users[] = $user;
    }
} else {
    $users = array();
}
?>
Пользователи
'.$user['name'].'
';
}
?>

We start the test - OK, we did not break anything. For convenience, we divide it into two files, in other words, we take out everything related to html in a separate index.php.html file:

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';

Пользователи
'.$user['name'].'
';
}
?>

Check - OK. Some smart people are what we now call a view or view in a new file. But forget about it for now. Although no, we’ll make it a little prettier (to taste and color ...).

Пользователи

Run the test - Failed! We changed the view and the test fails. But we know that in HTML, 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 selections are repeated often. Anyway, somehow all these $ DB flicker in your eyes and you need to understand what they are doing. Well, we follow DRY and put the work with the users table in a separate method of a separate class, we will pass the connection to the database through the constructor, and do not make the global variable. Let's name the class, say, UserRepository.

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 was ported almost unchanged, only the limit parameter was added. He added automatically :(, it was impossible in a good way. But the change is simple and we hope that nothing will break. But first we need to change our script

getAll();
include 'index.php.html';

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

Take another look at our repository. Nobody has a dissonance - did they create the “User Repository” class , whose objects return an array of associative arrays (aka hashes)? It arises for me. 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 simply the model class.

name = $properties['name'];
    }
    public function getName()
    {
        return $this->name;
    }
}

Change the view for working with an array of objects

Пользователи
getName() ?>

and add the declaration of the model class to the controller.

getAll();
include 'index.php.html';

Run the test - everything is OK! Perhaps this can stop for now. To summarize:

- the behavior of the script is almost guaranteed not changed. In addition to 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.

- the view we have is separate from everything else, all that he needs is for the array of users from objects with the getName () method to be in his scope. We can test it separately.

- complex (hehe) work with the database is encapsulated in the repository, which does not distract from studying the application logic, but the connection itself is created outside it, which makes it possible a) to substitute connections with different databases and b) to substitute fake (stubs and Moki) instances of compounds for testing and even) almost without changing anything use any other location - files, NoSQL databases and even faylopomoyki cloud faylohostingi.

- Model objects are essentially independent of the database in general, and indeed nothing, 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 everything else.

- the work of the main script is almost obvious, only the last include almost does not say anything, but I was already too lazy to allocate it to a function / method.

What else can I do with the code to improve the complexity of the architecture:

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

- ignore the type of presentation (our include) - it can be done so that it is rendered by some template engine without problems - Smarty, Twig, etc.

- make the controller also an object of the class

- use the FrontController pattern

- easily attach other third-party components ( ORM, routers, configs, etc.)

- transfer the code to 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 in a short cycle. Only at first it would not hurt to complicate the initial example of the govnokod to something more or less working and at least a little unobvious and shitty :) If there are anyone who wants to, then the rap on the github.com/VolCh/refactor-sample github