The Pokémon Problem: a new anti-pattern
March 19th, 2009 |
It’s always fun to release a new piece of jargon into the wild. I’ve run into a number of bugs in our codebase that caused by an anti-pattern I’d like to dub The Pokémon Problem.
Much like the game of Whac-a-Mole, this is a class of bugs where fixing every occurrence does not prevent the bug from returning in new code: it is easy for code delta to result in an instance of the bug being re-introduced into the code base. Even if you “catch ‘em all“, nothing prevents someone else from introducing new Pokémon bugs later.
Not only is this bug easy to re-introduce, but it sometimes can be hard to find all currently existing instances of this pattern. Although tools like Eclipse make it easier to track down all the places that code is called, sometimes you’re looking for things that happen in a certain sequence (which tools like Eclipse don’t do a good job of searching for) and dynamic invocation mechanisms like Java Reflection can sometimes make it impossible to be exhaustive. This type of bug is also resistant to automated refactoring: changing the protocol of dealing with this corner of your code will require you to track down all places it was touched and manually refactor them. It generally signals a failure to use sufficient separation of concerns.
In general, this anti-pattern is a result of APIs that require the caller to be responsible for state management of resources that the API owns. This can include things like an object that requires the caller to have run an initialization method before calling any other method on the object. These bugs get even more insidious when a failure to do things in the right order does not cause a hard failure (like throwing an exception) but instead creates some sort of subtle corruption that may not be noticed or cause subsequent calls to fail unexpectedly.
Read on for some strategies on dealing with the Pokémon problem.
Solving the Pokémon Problem
How do we solve the Pokémon problem? Sometimes it is as easy as writing a unit test to verify that no new Pokémon exist. More often than not, though, you’ll have to use better encapsulation to solve the problem. If the Pokemon problem is the anti-pattern, encapsulation is its opposite.
For an example, I’ll turn to a real problem I ran into in the Palantir Government codebase: Let’s say I have class Parser and it has a method getAttribute(). I want to add the ability to have shortened (obfuscated) tag names to the Parser class.
The wrong approach
One approach I can take:
- Create a class called AttributeHandler that wraps calls to Parser.getAttribute(). It handles detection of the short or full XML dialect and then returns the appropriate attribute value.
- Stick an instance of this AttrbributeHandler class in a member variable of Parser and make sure to call AttributeHandler.getAttribute() instead of Parser.getAttribute() when processing attributes.
- Put comment on Parser.getAttribute() mentioning that it shouldn’t be called directly anymore.
I just introduced a Pokémon problem. The next person that edits this code may not read my comment and know that calling Parser.getAttribute() is a bug. They may not test the short tag case and find the bug. How do I fix this Pokémon problem?
The right approach
Approaching this encapsulation problem has a number of different approaches one could take. Here’s what I chose:
- Create a class called AttributeHandler that wraps calls to Parser.getAttribute(). It handles detection of the short or full XML dialect and then returns the appropriate attribute value.
- Stick an instance of this AttrbributeHandler class in a member variable of Parser.
- Create a wrapper class around Parser that delegates the getAttribute() call to the AttributeHandler member when processing attributes.
(You might ask why I used a delegate instead of sub-classing the parser and overriding the getAttribute() method: it didn’t fit for architectural reasons that aren’t relevant here). Now the developer no longer has access to the Parser.getAttribute() method that would cause the undesired behavior.
In-depth Example: Resource Management
Another common place you might run into the Pokémon problem is code that needs to clean up resources. Some coders are lazy and don’t want to always write their try/finally/close blocks properly. This can lead to resource leaks; for things like file handles, this usually isn’t a large issue, but for scarce resources like database connections, it’s critical that things get cleaned and returned to the pool. For locks, it’s absolutely essential to avoid deadlocks.
The wrong way
Here’s a simple example of bad resource management. In this (admittedly contrived) scenario, we have class that’s managing a resource for us, in this case it’s a status file. Different parts of the app need to read the status file where, presumably, something is storing its status.
Here’s the naive implementation of the status manager class:
public class PokemonStatusFileManager {
final File statusFile;
public PokemonStatusFileManager(File statusFilePath) {
this.statusFile = statusFilePath;
}
public InputStream getStatusFileInputStream() throws FileNotFoundException {
return new FileInputStream(this.statusFile);
}
}
Based on this implementation, here’s some code that would use it. Note that first method, parseStatusFileIncorrectly() does no error checking and may not close the InputStream properly if an exception is thrown. The second method does proper resource handling, but it’s kind of ugly to read.
public class PokemonParseStatusFile {
/**
* This is not proper resource management.
* @param manager
* @throws IOException
*/
public static void parseStatusFileIncorrectly(PokemonStatusFileManager manager)
throws IOException {
InputStream statusFile = manager.getStatusFileInputStream();
readFrom(statusFile);
statusFile.close();
}
/**
* This is proper resource management, but it's tedious to have to write.
* Some coders are too lazy to always do this the right way.
* @param manager
* @throws IOException
*/
public static void parseStatusFileCorrectly(PokemonStatusFileManager manager)
throws IOException {
InputStream statusFile = null;
try {
statusFile = manager.getStatusFileInputStream();
readFrom(statusFile);
} finally {
// carefully close the resource
if(statusFile != null) {
try {
statusFile.close();
} catch(Exception e) {
System.err.println("Error closing statusFile!");
e.printStackTrace(System.err);
}
}
}
}
static void readFrom(InputStream statusFile) throws IOException {
// do the reading here...
}
}
So this produces a classic Pokémon problem: everyone who interacts with the StatusFileManager has to do proper resource handling. Now imagine that this is an important lock instead of just a file handle: this Pokémon problem could cause deadlock (which would be bad).
So how do we fix this? The Visitor Pattern.
Solving the Pokémon problem with the visitor pattern
The Visitor Pattern is a potent weapon in fighting the Pokémon problem: it allows you to fully encapsulate access to a resource by injecting into the places it’s needed but preserving overall control of the resource in the code that “owns” the resource. Classically used for controlling things like iteration order, here the visitor pattern is applied as a form of dependency injection to enable lifecycle management.
The visitor pattern as applied to resource management is fairly straightforward:
- Define an interface, ResourceVisitor with one method, visit(Resource r) (where Resource is the type of resource we’re managing.
- Define the resource manager with a method that takes a ResourceVisitor as a parameter. Manage the lifecycle of the resource and call ResourceVisitor.visit(r) when appropriate, handling all initialization, error-handling and cleanup.
Here’s our re-spin of the status file manager class. You’ll notice that we’ve moved the resource handling code from the correct example above and encapsulated it in the visitor pattern:
public class UnPokemonStatusFileManager {
/**
* Visitor interface implemented by callers wishing to
* interact with the status file.
*/
public static interface StatusFileVisitor {
public void visitStatusFile(InputStream statusFile) throws IOException;
}
final File statusFile;
public UnPokemonStatusFileManager(File statusFilePath) {
this.statusFile = statusFilePath;
}
/**
* Note that this method is now private.
*/
private InputStream getStatusFileInputStream() throws FileNotFoundException {
return new FileInputStream(this.statusFile);
}
/**
* Here's the method that takes the visitor and
* fully encapsulates the lifecycle of the status file.
*/
public void parseStatusFile(StatusFileVisitor parser)
throws IOException {
InputStream statusFile = this.getStatusFileInputStream();
try {
parser.visitStatusFile(statusFile);
} finally {
// carefully close the resource
try {
statusFile.close();
} catch(Exception e) {
System.err.println("Error closing statusFile!");
e.printStackTrace(System.err);
}
}
}
}
Now that we’ve encapsulated the complexity of the resource handling the UnPokemonStatusFileManager class, code that needs to access the status file can be written in a highly correct manner without much work.
public class UnPokemonParseStatusFile {
public static void parseStatusFile(UnPokemonStatusFileManager statusFileManager)
throws IOException {
// generate anonymous visitor to do the processing
StatusFileVisitor visitor = new StatusFileVisitor() {
public void visitStatusFile(InputStream statusFile) throws IOException {
readFrom(statusFile);
}
};
statusFileManager.parseStatusFile(visitor);
}
static void readFrom(InputStream statusFile) throws IOException {
// do the reading here...
}
}
By encapsulating the full lifecycle of the status file in the visitor pattern, I’ve ensured that it’s always accessed properly. If I change the place where we store status to be a database rather than a file, nothing needs to change except this one class; the calling code remains the same.
We now have easy re-factoring, no resource leaks, and have simplified calling code. And finally: there are no new bugs to be introduced by callers that aren’t sure how to use our resource. Looks like we caught ‘em all!
Wrapping It All Up
So there it is, your new piece of jargon: The Pokémon Problem anti-pattern. You heard it here first! Please post any other great examples to the comments section on this post.
Let me know if I’m going down the wrong path with this, but it seems like any common “bad coding practice” would fall under this anti-pattern.
In the C world, typical memory leaks seem to fall under this quite easily (leading to macro’ing things like FREE to do a null-check, free, and set-null). Similarly in the C++ world, RAII was created to handle some commonly-recurring memory leak patterns.
(Of course, garbage collection is the giant-hammer solution to that subset of your pokemon problem space, leading to using Java in the first place..)
March 23rd, 2009 at 5:35 pm
I like to use the jargon to describe holes that could be plugged by a better API. For every type system there are bugs that it will not be able to catch and we call these leaks for the most part.
So leaks are already named and don’t need a new name. We cannot prevent potential leaks in the future, we may only fix leaks as they happen. The pokemon problem is when you have the ability to catch all of them. We are fixing bugs that haven’t yet been written.
I like to think of pokemon problems as places where you could use a language provided facility to stop these bugs from ever being written again.
April 6th, 2009 at 5:17 pm