9

I've just come across a pattern I've seen before, and wanted to get opinions on it. The code in question involves an interface like this:

public interface MyCrazyAnalyzer {
    public void setOptions(AnalyzerOptions options);
    public void setText(String text);
    public void initialize();
    public int getOccurances(String query);
}

And the expected usage is like this:

MyCrazyAnalyzer crazy = AnalyzerFactory.getAnalyzer();
crazy.setOptions(true);
crazy.initialize();
Map<String, Integer> results = new HashMap<String, Integer>();
for(String item : items) {
    crazy.setText(item);
    results.put(item, crazy.getOccurances);
}

There's reasons for some of this. The setText(...) and getOccurances(...) are there because there are multiple queries you might want to do after doing the same expensive analysis on the data, but this can be refactored to a result class.

Why I think this is so bad: the implementation is storing state in a way that isn't clearly indicated by the interface. I've also seen something similar involving an interface that required to call "prepareResult", then "getResult". Now, I can think of well designed code that employs some of these features. Hadoop Mapper interface extends JobConfigurable and Closeable, but I see a big difference because it's a framework that uses user code implementing those interfaces, versus a service that could have multiple implementations. I suppose anything related to including a "close" method that must be called is justified, since there isn't any other reasonable way to do it. In some cases, like JDBC, this is a consequence of a leaky abstraction, but in the two pieces of code I'm thinking of, it's pretty clearly a consequence of programmers hastily adding an interface to a spaghetti code class to clean it up.

My questions are:

  1. Does everyone agree this is a poorly designed interface?
  2. Is this a described anti-pattern?
  3. Does this kind of initialization ever belong in an interface?
  4. Does this only seem wrong to me because I have a preference for functional style and immutability?

If this is common enough to deserve a name, I suggest the "Secret Handshake" anti-pattern for an interface that forces you to call multiple methods in a particular order when the interface isn't inherently stateful (like a Collection).

Kevin Peterson
  • 7,189
  • 5
  • 36
  • 43
  • To be honest, this "pattern" reminds me to DataReaders in ADO.NET. That doesn't mean that it's necessarily a good idea, but it's used even in serious APIs. – Tamas Czinege Apr 27 '09 at 08:42
  • Good anti-pattern. If objects were meant to be initialised like that we'd be able to specify constructors in interfaces. I like the term 'secret handshake' too. – CurtainDog Apr 27 '09 at 10:02
  • At least it's explicitly bad, unlike java.util.MessageFormat which uses temporary internal state during single method calls. – Tom Hawtin - tackline Apr 27 '09 at 13:38
  • 2
    I call this the combination lock object. It only works when you access it by calling things in the exact right order. – sal Apr 27 '09 at 21:30
  • @Tom MessageFormat, like many other classes (notably most collections), is not thread-safe. Truly, it is very annoying to have to go to the documentation to get this information, but there is no way in the language to make thread-safety part of the interface. Other languages may be better in this respect but none that I can say for sure. – CurtainDog Apr 28 '09 at 11:22

5 Answers5

18

Yes, it's an anti-pattern: Sequential coupling.

I'd refactor into Options - passed to the factory, and Results, returned from an analyseText() method.

Douglas Leeder
  • 52,368
  • 9
  • 94
  • 137
  • 15
    Excellent, now I can put "remove sequential coupling" in my checkin messages rather than the less polite "remove confused garbage". – Kevin Peterson Apr 27 '09 at 18:14
3
  1. I'd expect to see the AnalyzerFactory get passed the necessary params and do the construction itself; otherwise, what exactly is it doing?
  2. Not sure if it does have a name, but it seems like it should :)
  3. Yes, occassionally it's convenient (and the right level of abstraction) to have setters in your interface and expect classes to call them. I'd suggest that doing so requires extensive documentation of that fact.
  4. Not really, no. A preference for immutability is certainly a good thing, and setter/bean based design can be the "right" choice sometimes too, but your given example is taking it too far.
GaryF
  • 23,950
  • 10
  • 60
  • 73
  • +1 Because I was wondering what the factory was doing as well... seems like it's just being used as a global resource, another no-no. – CurtainDog Apr 27 '09 at 09:45
3

I'm not sure whether it's a described anti-pattern but I totally agree this is a poorly designed interface. It leaves too much opportunity for error and violates at least one key principle: make your API hard to misuse.

Besides misuse, this API can also lead to hard-to-debug errors if multiple threads make use of the same instance.

Joshua Bloch actually has an excellent presentation (36m16s and 40m30s) on API design and he addresses this as one of the characteristics of a poorly designed API.

Ronald Wildenberg
  • 31,634
  • 14
  • 90
  • 133
0

One possible solution - use Fluent chaning. That avoids a class containing methods that need to called in a certain order. It's a lot like the builder pattern which ensures you don't read objects that are still in the middle of being populated.

Sridhar Sarnobat
  • 25,183
  • 12
  • 93
  • 106
0

I can't see anything bad in here. setText() prepares the stage; after that, you have one or more calls to getOccurances(). Since setText() is so expensive, I can't think of any other way to do this.

getOccurances(text, query) would fix the "secret handshake" at a tremendous performance cost. You could try to cache text in getOccurances() and only update your internal caches when the text changes but that starts to look more and more like sacrifice to some OO principle. If a rule doesn't make sense, then don't apply it. Software developers have a brain for a reason.

Aaron Digulla
  • 321,842
  • 108
  • 597
  • 820