OOP is wonderful. Programmers usually criticize those who don't follow this paradigm, while the knowledge of patterns is often a must. However, even the right approach doesn't completely protect from errors. Today, we'll learn how to break a program using the standard template method.
This is another article born from the check of DBeaver 24 using the PVS-Studio static analyzer. Along the way, I found some suspicious code fragments. They inspired me to devote separate articles to them. Here's an updatable list of articles in the series:
Not so long ago, I've written an article on how you can apply OOP in your daily work. It discusses the theoretical and hands-on approaches and covers the topic more broadly than we'll do today. So, if you're interested, I invite you to read that article. However, don't worry, we'll be revising all the necessary information here. In this article, let's treat the need to use OOP and adhere to SOLID as a fundamental principle.
So, one of the simplest patterns is the template method. However, if some of you've suddenly forgotten how it's implemented, or if you're still a newbie, let's recall its contents.
This section covers the basic information about the method, so if you're familiar with it, you may skip right to the next one. The rest are welcome to the theoretical part.
Let's pay tribute to GoF and take the classic scheme of this pattern from their book:
It's that simple. If you know how to read class diagrams, of course. Otherwise, we can decipher it using an example in code that strives to appear real.
Let's say we need to output the contents of the Person class to the console (although, it can be a file or anything else). The class contains only the first and last name.
public class Person {
private final String name;
private final String surname;
public String getName() {
return name;
}
public String getSurname() {
return surname;
}
public Person(String name, String surname) {
this.name = name;
this.surname = surname;
}
}If you think I don't want to overcomplicate things, then you're right :)
We can't just output the class contents, we need to serialize them. Just for the fun of it, let's imagine we have two serialization formats: json and xml. Of course, we could do everything in one class and choose the required serialization type via an enumeration, but then we'd violate two SOLID principles:
Of course, once we remember this, we immediately realize that this isn't the right way. Instead, let's define an abstract serializer method in our printer class. The class implementation is trivial:
public abstract class AbstractPersonPrinter {
protected abstract String serialize(Person person);
public void print(Person person) {
System.out.println(serialize(person));
}
}All we need to do is create derivatives that implement their own logic. For json:
public class JsonPersonPrinter extends AbstractPersonPrinter {
@Override
public String serialize(Person person) {
var sb = new StringBuilder();
var s = System.getProperty("line.separator");
sb.append("{").append(s);
sb.append(" name: \"").append(person.getName())
.append("\"")
.append(s);
sb.append(" surname: \"").append(person.getSurname())
.append("\"")
.append(s);
sb.append("}");
return sb.toString();
}
}And for xml:
public class XmlPersonPrinter extends AbstractPersonPrinter {
@Override
public String serialize(Person person) {
var sb = new StringBuilder();
var s = System.getProperty("line.separator");
sb.append("<root>").append(s);
sb.append(" <name>").append(s);
sb.append(" ").append(person.getName())
.append(s);
sb.append(" </name>").append(s);
sb.append(" <surname>").append(s);
sb.append(" ").append(person.getSurname())
.append(s);
sb.append(" </surname>").append(s);
sb.append("</root>");
return sb.toString();
}
}Voilà. Now we can configure a logger and get the output in the format we want. If we ever wanted to get the json or xml output at all.
public class ConsoleLogger {
private final AbstractPersonPrinter printer;
public ConsoleLogger(AbstractPersonPrinter printer) {
this.printer = printer;
}
public void logPerson(Person person) {
printer.print(person);
}
....
}json:
{
name: "John"
surname: "Doe"
}xml:
<root>
<name>
John
</name>
<surname>
Doe
</surname>
</root>Let's get back to the class diagram. We can redo it for this example, so that it's easier to understand the previous one:
Okay, the long explanation of the pattern is now over. Almost. The last thing to be mentioned: if you thought that it'd be better to use composition (i.e. a strategy) instead of inheritance, you aren't wrong. I just wanted to demonstrate a particular pattern, so we shall use it.
One could say that the template method is basic in many ways, but there are some non-obvious parts of it.
For example, we've had the following task: a web page had a form with mostly editable data. We needed to keep track of the data before and after the changes. This way, when a user closed the page, we could notify them that they hadn't saved the changes. There were many such forms on different pages, so we needed a common solution.
Let's try to solve it:
We need to fix the second issue somehow. To do this, we introduce an overridden method where we can specify the fields that we don't need. Here's a slightly simplified solution that I've written back then:
public abstract class ParameterBase<TEntity>
: ObservableValidator where TEntity : class
{
protected virtual List<string> GetIgnoredProperties()
=> new List<string>();
public ParameterBase(TEntity entity)
{
var ignore = GetIgnoredProperties();
var sourceProp = GetType().GetProperties();
var colNumber = sourceProp.Length - ignore.Length;
var colCounter = 0;
foreach (var prop in sourceProp)
if (!(ignore.Contains(prop.Name)))
{
prop.SetValue(this, typeof(TEntity)
.GetProperty(prop.Name, Consts.PropertyBindingFlags)
.GetValue(entity, null), null);
colCounter++;
}
if (colNumber != colCounter)
throw new InvalidOperationException(
"Not every parameter field got its value");
this.PropertyChanged += Change;
}
....
}My Java isn't broken, this is just the C# code :) Please take this as a stylistic digression. I tried to keep the code simple.
You may ask why C# is used in the front-end task, I'll answer that it's the Blazor framework. Our company actively uses it.
By the way, since we're talking about other programming languages, we have an article on a similar topic for C++.
The algorithm is quite simple: using reflection, we copy all properties from the generic model to the ParameterBase descendant and ignore those specified in the descendant itself. If the quantity doesn't add up, an exception is thrown. Actually, the GetIgnoredMappingProperties method is a special case of a template method called a hook. The task is complete. Everything's fine, right?
It could be fine. But after the build is finished, if we enable the incremental analysis, then PVS-Studio issues the following message for the code above:
V3068 Calling overrideable class member 'GetIgnoredProperties' from constructor is dangerous. ParameterBase.cs(34)
At this point, I have to say that I'm not a big fan of code smell analysis. Most of the time, one would feel outraged, curse the tool, and suppress the warning looking like a smarty-pants. No need for suspense: in this case, the analyzer is wrong, and my solution has no issue. That's exactly what I did back then, putting the warning in the project suppress file. This is where I got it from now.
Sure, you can imagine a possible issue and how to fix it:
But why use these fancy tricks just to make the analyzer leave us alone, when it's easier to just suppress the warning? Well, it's boring.
This case came to my mind when I stumbled upon a similar PVS-Studio message for Java while browsing DBeaver:
V6052 Calling overridden 'isBinaryContents' method in 'TextWithOpen' parent-class constructor may lead to use of uninitialized data. Inspect field: binary. TextWithOpenFile.java(77), TextWithOpen.java(59)
Looking back on my experiences, I wanted to move on but decided to examine the code anyway. So, the isBinaryContents method is in the TextWithOpenFIle class:
public class TextWithOpenFile extends TextWithOpen {
private final boolean binary;
....
@Override
protected boolean isBinaryContents() {
return binary;
}
....
}It's not exciting. Let's look at the only code fragment where it's used, though:
public TextWithOpen(Composite parent, boolean multiFS, boolean secured) {
super(parent, SWT.NONE);
....
if (!useTextEditor && !isBinaryContents()) {
....
editItem.setEnabled(false);
}
....
}The analyzer pointed to a huge constructor that I've shortened for convenience. The previously mentioned isBinaryContents is used in the condition, whose body I've shortened by about 40 code lines. Note that we're now in the parent class of TextWithOpen. Now it'd be nice to see what's inside the parent isBinaryContents:
protected boolean isBinaryContents() {
return false;
}Oh, the hook we've discussed above. So, developers wanted the second condition in the parent class to always be true (don't forget about the negation in it). Okay, what does the diagnostic documentation say?
The analyzer has detected a parent-class constructor that uses a method overridden in the derived class. As a result, the overridden method can be used by uninitialized class fields.
We need to check the constructor of the TextWithOpenFIle class:
public TextWithOpenFile(
Composite parent,
String title,
String[] filterExt,
int style,
boolean binary,
boolean multiFS,
boolean secured
) {
super(parent, multiFS, secured); // <=
this.title = title;
this.filterExt = filterExt;
this.style = style;
this.binary = binary; // <=
}Wow. An error is here. We call the TextWithOpenFile constructor first. Then, it calls the TextWithOpen constructor, where isBinaryContents is called. The isBinaryContents method reads the value of binary that is false by default. Only then the binary field is initialized in the TextWithOpenFile constructor.
The most annoying thing is that it's not immediately clear how to fix it. Simply moving the super call down doesn't work, unfortunately :)
The easiest way would be to put the initialization in a separate method and call it separately in all constructors. It's nice and simple but inefficient: if we overcomplicate the initialization, the probability of an error when creating a new derivative is higher.
Using creational patterns would be a good alternative:
It seems that here I have to admit that my skepticism had been put to shame, and the analyzer warnings shouldn't be ignored. I'll still stand by my opinion, though. After all, even our documentation states the following:
If you do want the program to behave as described above when initializing an object and you want to hide the analyzer warning, mark the message as a false positive.
In my case, I estimated and still estimate the probability of an error to be incredibly small. However, there are no guarantees. Here we have a much more complex initialization with more parameters. Under these circumstances, a dangerous trick has naturally led to an error. All in all, sound risk assessment is the key to decision-making. It doesn't sound exciting too, I know :)
I'd like to end the article here. I hope you enjoyed reading about how the right approach can lead to a frustrating mistake. And if using overridden methods in constructors you'll double-check everything, then my job here is done :)
If you'd like to search for those or other errors in your project, you may try PVS-Studio for free by following this link.
I'd also like to remind you that these aren't all the interesting things found in DBeaver. You can find links to other articles in the beginning. In the meantime, I'd recommend subscribing to my X-twitter so you don't miss the release of new ones.