I’m trying to decide the best course of action based on two design principles that seem to be conflicting with each other: “a function should only do one thing” (SRP) and “don’t repeat yourself” (DRY).
I need to perform two actions:
- Get document properties (returns json data)
- Download document (returns raw file data)
I’m calling an API for those two actions, and the only difference between those two calls are the API path I need to use, otherwise, they are exactly the same.
So should I have two separate functions and break the “don’t repeat yourself” (DRY) rule:
function getDocument($id) {
// Build path:
"/api/v1/$id"
// Call API
// Return Result
}
function downloadDocument($id) {
// Build path:
"/api/v1/$id/download"
// Call API
// Return Result
}
Or should I have a single function and break the “do only one thing” (SRP) rule:
function getDocument($id, $download = FALSE) {
// Build path:
IF $download == TRUE
"/api/v1/$id/download"
ELSE
"/api/v1/$id"
// Call API
// Return Result
}
4
I would recommend against using flag parameter as your suggestion. To follow SRP and DRY, you can create a new function accept an API URL and call that function in your current ones:
function callDocumentApi($url) {
// Call API
// Return results
}
function getDocument($id) {
// Build path:
$url = "/api/v1/$id";
return callDocumentApi($url);
}
function downloadDocument($id) {
// Build path:
$url = "/api/v1/$id/download";
return callDocumentApi($url);
}
There’re several and good reasons why you should avoid flag function arguments whenever possible. Some reasons are:
-
Ideally, functions should do only one thing. Flag arguments are good hints of functions doing more things besides the one denoted by their names. Functions doing more than one thing are at the edge of breaking the principle of least astonishment. Taking care of POLA improves significantly the best code quality indicator you can have.
-
Functions doing more than one thing are especially troubling when their name don’t tell the reader what other things they do and how the flag argument is conditioning the behavior. That’s the case of the function in option #2. See how it’s seen from the consumer standpoint. [N1]
//What the hell, "true" does mean?
$id = "myId";
getDocument($id, true);
//... not much better
$download = true;
getDocument($id, download);
- Functions with no arguments (niladic) or one argument (monadic) are easy to reason about (when both, function and argument are properly named). But things start getting complicated with 2 (dyadic) or more arguments tryadic,polyadic, etc). They are not only harder to understand at first glance; they are also harder to test because it takes testing all the permutations possible.
With these 3 points in mind, the proper approach would be option #1. Having two well-differentiated functions, properly named, short and concise.
class DocumentApi{
private const API_DOCUMENT_URI_TEMPLATE = '/api/v1/$id';
private const API_DOWNLOAD_URI_TEMPLATE = self::API_DOCUMENT_URI_TEMPLATE.'/download';
private const DEFAULT_HTTP_REQUEST_HEADER = array(
'Accept' => 'application/json'
);
private function doGet($path, $uriParams, $httpRequestHeaders){
$uri = strtr($path, $uriParams);
//http client goes here
}
function getDocument($documentId){
$queryParameters = array(
'$id' => $documentId
);
doGet(self::API_DOCUMENT_URI_TEMPLATE ,
$queryParameters,
self::DEFAULT_HTTP_REQUEST_HEADER );
}
function downloadDocument($documentId){
$queryParameters = array(
'$id' => $documentId
);
//In PHP arrays are assigned by copy
$httpRequestHeaders = self::DEFAULT_HTTP_REQUEST_HEADER;
$httpRequestHeaders['Accept-Encoding'] = 'gzip, deflate';
$httpRequestHeaders['Accept'] = 'mime-type/*';
doGet(self::API_DOWNLOAD_URI_TEMPLATE, $queryParameters, $httpRequestHeaders);
}
}
[N1]: Some may say that it just takes checking the function’s signature to know what’s the argument’s name and guess what is it for. This is bad for three reasons: 1. Code that makes you dive into the source code is clearly not clean code, it’s untrusty code. 2. The laziness of the developer in charge of the code still can surprise you with things like getDocument(id, bool). 3. For a reason, you neither have documentation nor the source code of the function and the IDE shows up something like getDocument(arg0, arg1)
Which is more important, SRP or DRY ? This is not the right question.
SRP and DRY are not goals per se. The goal is to write the easiest code to maintain(1). SRP and DRY are tools that can be use to achieve it.
The question you should ask yourself is: which version is easier to maintain over time, the SRP or the DRY version? And note that it could be any other version that could break SRP and DRY altogether…
(1) One could argue that the goal is not even to write the easiest code to maintain. It’s to give the largest ROI to your sponsor for its investment. Writing maintainable code may not be important in some scenarios.