By Benjamin Eberlei, first published at Tue, 21 Mar 2017 09:54:19 +0100
Download our free e-book "Crafting Quality Software" with a selection of the finest blog posts as PDF or EPub.
You can also buy a printed version of the book on Amazon or on epubli.
When you are refactoring in a legacy codebase, the goal is often to reduce complexity or separate concerns from classes, methods and functions that do too much work themselves. Primary candidates for refactoring are often controller classes or use-case oriented service classes (such as a UserService).
Extracting new service classes is one popular refactoring to separate concerns, but without tests it is dangerous because there are many ways to break your original code.
This post presents a list of steps and checklists to perform extract service when you don't have tests or only minimal test coverage. It is not 100% safe but it provides small baby-steps that can be applied and immediately verified.
The primary risk of failure is the temptation to do too many steps at the same time, delaying the re-execution and verification that the code still works for many minutes or even hours. Read more about this in How to Refactor Without Breaking Things.
This post builds upon the previous post on Extract Method, where we already moved a dedicated concern with low-level code to query a Solr database into a new method.
public function searchAction(Request $request)
{
if ($request->has('query') || $request->has('type')) {
$result = $this->search($request);
return ['result' => $result];
}
return [];
}
private function search(Request $request)
{
$select = $this->solarium->createSelect();
if ($request->has('type')) { // filter by type
$filterQueryTerm = sprintf(
'type:%s',
$select->getHelper()->escapeTerm($request->get('type'))
);
$filterQuery = $select->createFilterQuery('type')->setQuery($filterQueryTerm);
$select->addFilterQuery($filterQuery);
}
// more filtering logic here
$result = $solarium->select($query);
return $reesult;
}Our goal is to extract all the Solr code into a new SolrSearchService class. Before you start you should have already used Extract Method to create one or several methods that you want to move to a new class.
Similar to the extract method refactoring we start by copying code 1:1 without changing it for now.
class SolrSearchService
{
private function search(Request $request)
{
$select = $this->solarium->createSelect();
if ($request->has('type')) { // filter by type
$filterQueryTerm = sprintf(
'type:%s',
$select->getHelper()->escapeTerm($request->get('type'))
);
$filterQuery = $select->createFilterQuery('type')->setQuery($filterQueryTerm);
$select->addFilterQuery($filterQuery);
}
// more filtering logic here
$result = $solarium->select($query);
return $reesult;
}
}You can modify the private visibility to public as the first step, because we need to call this method from the original class it must be public.
You must also copy over all the use statements that are in the original class, because otherwise the new class could import a class from the wrong location.
use Symfony\Component\HttpFoundation\Request;
class SolrSearchService
{
public function search(Request $request)
{
// ...
}
}I usually copy all use statements if I extract a larger block of code, because then I don't miss a reference in the moved codeblock. If the block is smaller I only copy the ones that are actually used in the block. An IDE helps here because it can highlight the superfluous use statements and recommend missing ones.
As a last point in this section, verify that this new class is autoloadable, so that you can easily use it from tests and the original class.
As a next step you must look at the extracted code and find all references to instance variables, because our new class doesn't have those instance variables itself, they are still on the original class.
There are several cases of instance variable uses that need different handling:
You find an instance variable that is only used in the extracted method itself. Then you can copy (don't delete it just yet) the variable to the new service. You could perform a refactoring to convert the instance variable to a local variable if the state is not needed across multiple calls to the method.
The instance variable is another object that is injected into the originals class constructor or with setter injection. Copy the instance variable over to the new object and introduce a constructor that sets this dependency.
The instance variable is used for state that is only read in the new class. Convert the instance variable into a local variable on the new class and pass it as a new argument to the extracted method.
The instance variable is used for state that is read, changed or both in the new class. Convert the instance variable into a local variable on the new class and pass it to a setter on the new service before you call your news method, and retrieve it back with a getter after you called the service.
Method 4 works for all use cases, always use it when you are unsure, even if it requires the most code and does not look very clean.
It requires a bit of experience to quickly categorize instance variables into these groups and what the best course of action is. Don't be discouraged if the first attempts lead to nothing or weird APIs, remember that refactoring is a constant process and intermediate steps may actually make the code worse.
In our example we have one instance variable $this->solarium which is a service used in the parent class as well, point 2 in our list. We add the following code to SolrSearchService:
class SolrSearchService
{
private $solarium;
public function __construct(\Solarium_Client $solarium)
{
$this->solarium = $solarium;
}
}This class should now be usable independently of the original class. You should now write some tests for it, in this case integration tests, because we rely on third party APIs and a database.
Make sure to use dependency injection for the new class even if your dependencies are available as singletons or from a static registry. This way you at least this class is testable and you can make use of mock objects to write those tests.
The original class still uses the old extracted method. Comment the body of the method out and instantiate the new class inline, pass all the dependencies into the constructor or as arguments to the new method:
private function search(Request $request)
{
// commented out old code
$solrSearchService = new SolrSearchService($this->solarium);
$result = $solrSearchService->search($request);
return $result;
}This method looks more complex if you have instance variables mentioned as item 3, 4 or 5 in the previous section.
The original code is now runnable. It should work out of the box if you have followed all the steps correctly. Remove the commented out old code and remove instance variables that were entirely moved to the new class (type 1).
You should test the code manually (or with functional tests) and commit your current changes now.
The search method on the original class is not needed anymore, you can inline the method into the location where its called from:
public function searchAction(Request $request)
{
if ($request->has('query') || $request->has('type')) {
$solrSearchService = new SolrSearchService($this->solarium);
$result = $solrSearchService->search($request);
return ['result' => $result];
}
return [];
}This step is optional, but instantiating the SolrSearchService in your runtime code is not a good practice. You can create a new instance variable solrSearchService and move the new to where the solarium instance variable is already assigned:
private $solarium;
private $solrSearchService;
public function __construct(\Solarium_Client $solarium)
{
$this->solarium = $solarium;
$this->solrSearchService = new SolrSearchService($solarium);
}
public function searchAction(Request $request)
{
if ($request->has('query') || $request->has('type')) {
$result = $this->solrSearchService->search($request);
return ['result' => $result];
}
return [];
}This step makes sense if you continue extracting additional methods from the original class to the new SolrSearchService, because they can now reuse the same instance.
This step is optional and is only possible if you are already injecting dependencies using a factory or containers. After extracting all methods that work on the solarium instance variable to the new service we don't need the instance variable on the original class anymore.
At this point we can switch the injected dependency to be the SolrSearchService directly:
private $solrSearchService;
public function __construct(SolrSearchService $solrSearchService)
{
$this->solrSearchService = $solrSearchService;
}Qafoo experts can kick-start your team with a continuous refactoring process. We can show you how to improve your source code quality on the go and help you to get rid of the big quality chuckholes in your construction site.
Compared to the extract method refactoring, extracting a service requires more steps and each of them is more risky. On top of that IDEs usually don't provide this refactoring as an automatic procedure, so you have to do it manually. But even though the refactoring is risky, you should learn and master it, because it is very effective at splitting up code that started out simple and got more complex over time.
Stay up to date with regular new technological insights by subscribing to our newsletter. We will send you articles to improve your developments skills.