While implementing a new feature, I came across the flag parameter anti-pattern in two unrelated places. I would like to take this as an opportunity to take a closer look at this anti-pattern.
A simple example
Lets assume we want to load all PDF documents via an API method. This could look like this:
public interface DocumentService {
Document[] loadDocuments();
}
It Starts Smelling
In the next step we need a method to load all TIFF documents. Since the difference in implementation might be only one line of code, the result might look like this:
public interface DocumentService {
/**
* @param loadTiff true: loads TIFF documents - false: loads PDF documents
*/
Document[] loadDocuments(boolean loadTiff);
}
The implementation is also modified very quickly with an additional if block:
public Document[] loadDocuments(boolean loadTiff) {
int format = 1; // load PDF documents by default
if (loadTiff) {
format = 2; // except when TIFFs are requested
}
...
}
However, if we look at the client side of this API, we already notice an unpleasant smell:
Document[] documents = documentService.loadDocuments(false);
At this code point, can you tell what the meaning of false
is without looking at the definition of the method? And what happens if we change this to true
?
The Smell Begins to Stink
In the next step we also need a method to load RTF documents. Again, the difference in implementation is only one line of code. But how can we reuse the existing method without having to re-implement the logic? The flag parameter stands in our way! The boolean
parameter can only take two states, true
or false
. (Okay, the clever Java developer changes this to a Boolean
parameter and has true
, false
and null
as choices).
However, the flag parameter also shows us something else: the single responsibility principle is violated here. In fact, this method currently has more than one responsibility. On the one hand, it knows how to load PDF documents. On the other hand, it also knows how to load TIFF documents. Or more generally: such a method does two different things - once on true
and once on false
.
Possible Solutions
Spontaneously I can think of two simple ways to get rid of the flag parameter.
Option 1: We define an interface for the document types to query for information. Then we can generalize the method by using the document type as a parameter. However, there is a possibility of unnecessary over-generalization of the business logic.
public interface DocumentService {
Document[] loadDocuments(DocumentType documentType);
}
Option 2: We create a specific method for each possible domain context.
public interface DocumentService {
Document[] loadPdfDocuments();
Document[] loadTiffDocuments();
Document[] loadRtfDocuments();
}
In both cases, from the client's side, this results in readable code:
Document[] documents = documentService.loadDocuments(DocumentType.PDF);
Document[] documents = documentService.loadPdfDocuments();
What About Setter Methods?
If we were strict, then we would also have to change
public class Action {
private boolean enabled;
public void setEnabled(boolean enabled) {
this.enabled = enabled;
}
}
to
public class Action {
private boolean enabled;
public void enable() {
this.enabled = true;
}
public void disable() {
this.enabled = false;
}
}
However, we don't have to be more papal than the pope here. If I had to choose between
action.setEnabled(actionCheckBox.isChecked());
and
if (actionCheckBox.isChecked()) {
action.enable();
} else {
action.disable();
}
for the client code, I would stay with the first one.
Further Reading
- FlagArgument by Martin Fowler
- Clean Code Tip: Eliminate Boolean Arguments by Uncle Bob
I hope this article was helpful to you. Feel free to leave a comment. For more Java stuff, follow me on Twitter.
Top comments (5)
"Nullable booleans" with three values are an offense to the mathematical laws of boolean algebras.
Booleans can by definition only have two values.
Please don't do that.
Instead use an enum with three values
Yes I can because I would be calling the API from Kotlin who has named (and default) parameters
Ok that's a bad faith argument, you can do the same in Java by using the (good but cluncky) Builder pattern, and I don't actually like non obvious boolean parameters either.
Great article, thanks for sharing.
I would go for something like following
Some people argue - add the arguments to an object and there will be no boolean. I think it still is, just hidden.
`loadDocuments(UserInput $userInput)
{
if ($userInput->loadTiff) {
return generateTiff();
}
return generatePdf();
}`
vs
`
loadDocuments(bool loadTiff)
{
if (loadTiff) {
return generateTiff();
}
return generatePdf();
}
`
UserInput will have bool flag. How this makes you not go to look at definition what it is doing?
And how will it then not violate single responsibility principle.
And is here priniple violated? Generating document is dedicated to some other function like generatePdf(). I heard then it does not count as loadDocuments doing many things.
Hi @dariusv_65 ! good points.
syntax tip: change those Β«commasΒ» using three instead for code blocks: