Refactoring out of sheer laziness.
What’s a class to do?
The previously developed ELH_ShopRetriever class was the first real piece of the plugin I’ve developed and that’s communicating with external components.
What does this class do in plain terms? What’s its sole responsibility? It makes a request to retrieve the user shops.
The short definition above introduces the next class I will be developing: the ELH_ListingRetriever class; what’s this class sole responsibility? It makes a request to retrieve the shop listings.
I’m using easy sentences because defining a class responsibility should not be difficult to begin with and because it triggers the refactoring in my mind.
Both classes share a common task, they are both extending the same abstract ELH_AbstractSyncStep to begin with, and have very similar responsibilities: almost equals up to “It makes a request to retrieve…”.
Refactoring and the protection of testing
Under the protection of an ongoing green light from the tests I’ve refactored the ELH_ShopRetriever class making it an extension of a new and more specific abstract class: ELH_RequestingSyncStep.
This new abstract class is the code translation of the sentence “It makes a request to retrieve…” and features almost all of the code the ELH_ShopRetriever class had defined (see it on GitHub).
With little specifics left to define on its own the ELH_ShopRetriever class is slimmed down to some few lines
class ELH_ShopRetriever extends ELH_RequestingSyncStep {
public static function instance( ELH_KeychainInterface $keychain, ELH_ApiInterface $api, ELH_ApiRequestInterface $request, ELH_RequestCompilerInterface $request_compiler ) {
$instance = new self();
$instance->keychain = $keychain;
$instance->api = $api;
$instance->request = $request;
$instance->request_compiler = $request_compiler;
return $instance;
}
/**
* @return array
*/
protected function get_request_url() {
$data = array(
'user_id' => get_option( ELH_Main::USER_ID_OPTION ),
'api_key' => $this->api->get_api_key()
);
$this->request_compiler->set_request( $this->request );
$uri = $this->request_compiler->get_compiled_request( $data );
return ELH_Main::API_BASE . $uri;
}
/**
* @param $data
*
* @throws ELH_SyncException
*/
protected function ensure_request_specifics( $data ) {
if ( $data['response']['code'] != '200' ) {
$message = sprintf( 'Shop retrieving failed with code %d and message "%s"', $data['response']['code'], $data['response']['message'] );
throw new ELH_SyncException( $message );
}
}
}
Should I move the tests?
I’ve moved code from one class to another and even if the relation between them is inheritance shouldn’t I move the test code I had written for the ELH_ShopRetriever class (in the ELH_ShopRetrieverTest class) in a new test class dedicated to it? Like ELH_RequestingSyncStepTest?
I’d say no. The way I’d test an abstract class would be by extending it with a concrete class and then actually test that concrete class; that I have done already and while I have created a test dependency, I have to test ELH_ShopRetriever to test some methods and functionalities of ELH_RequestingSyncStep, I’m fine with it.
Next
I will move to the ELH_ListingRetriever class and TDD it with as little effort as possible.