Sunday, August 18, 2013

Iterable is an Anti-Pattern

I couldn't decide I hated UnsupportedOperationException or Iterator more, so I decided to pick on Iterable instead...

Iterable limits the implementer to having one interesting iterable attribute. 

Having a single iterator producing method (Iterable.iterator()) restricts the implementing object to only ever have one interesting quality to iterate over. This requires that it be obvious from the name of the iterable class and the class name of the iterable components (that you often can't control) what it is you're allowed to iterate over.
Company workCo;
for(Position currP : workCo) { ... }
What are we iterating here? Positions within the Company? What does that even mean? Is it even ordered? Is it open positions or filled positions? These are all things you'd normally hit at with a well named method. 

The JDK authors at least sort of figure this out for Map, but in order to make it work they had to expose the inner workings of how a Map works, and in doing so dictate implementation details in an interface. Your Map must implement entries() and it can't just be a view; it has to be mutable (or explosive) and tightly coupled with the Map it came from. This is terrible for asynchronous execution and expensive to implement.

Now, to play devil's advocate, one might say that if you have multiple iterable attributes you shouldn't be iterable... That's valid. Maybe the Company class really needs to contain multiple iterable collections, like currentEmployees() and openPositions(). But this just hammers home my point by pulling "an Apple" and telling users they're holding/using it wrong.

Iterable is not extensible.

Your object may only have one iterable element, but what about extensions? Iterable forces any extensions with any other iterable attributes to be via compositions and views. This is especially true for elements with multiple attributes or multiple views.

Liskov's Substitution Principle requires that all extensions demonstrate an "is-a" relationship to their parent. If you missed that day in CS class, it means that "Nissan370Z" can extend "Car", but "Truck" can't. Even if a trunk does all the same things a car does, a truck is not a car.

The more API you define the harder it is to extend. 

Iterable is poorly composable. 

So when you can't extend to add another view type what do you do? You compose the class with a view.
class Sizes implements Iterable {
  Collection> stuff;
  Viewer(Collection stuff) {
    this.stuff = stuff;
  }
  Iterable iterable() {
    return new Iterator() {
      [impl goes here]
    };
  }
}
So you took your collection of collections and composed it, and now you get to write your own custom Iterator... Yay.

Terrible. 

Interface method names shouldn't hide what they return. 

If an object has multiple Iterable fields, for example, and it implements Iterable... which field does the returned Iterator, from the method iterator(), actually iterate over? The method name doesn't tell you, and that means the code that iterates the object doesn't tell you, and is thus less readable.

Abstract the type, sure, but don't abstract the method name. Otherwise you end up with and API with:
Iterator iterator();
What does that return?
It might make sense if the type is more specific, but are you really going to wrap your String in an object just to give it a better name? No, that's stupid and expensive.

You shouldn't have to implement something to say you're not doing it.

Iterator is an anti-pattern itself, with its mutability baked right in. You have to explode with an UnsupportedOperationException if you want to tell the user (at runtime) that the view you're iterating over isn't mutable.
@Override
public void remove() {
    throw new UnsupportedOperationException("Sorry, No.");
}
How many times have you had to write that crap?
Google Guava even has an abstract class to clean up their code, but that doesn't legitimize it.

To be honest, UnsupportedOperationException is pretty bad all by itself. Just it's existence means that the JDK regularly violates the Liskov's Substitution Principle.

All JDK collections are mutable. 

This is really one of the core problems with the JDK and the reason Iterator is bad. The API for immutable objects is conceptually a subset of a mutable API with the primary exception that a builder or constructor from a mutable is required. 

So what's the fix?

There isn't one really. The JDK is hosed. It's never going to get fixed, because it would break reverse compatibility horribly, and because we can't delete those old classes we can't really reuse their names either, they're already taken.

I've considered spinning up an alternative JDK, starting with collections, taking what we've learned about immutability and substitution and composition and starting over.... Who's with me?

We'll start with this:
interface ForwardCursor {
  boolean hasNext();
  T next();
}
interface BackwardCursor {
  boolean hasPrevious();
  T previous();
}
interface BidirectionalCursor extends ForwardCursor, BackwardCursor {
}
And we'll add some magic syntactical sugar similar to the for each loop to work with ForwardCursor, except we're goign to call it "for rest" because it will start where the cursor is currently, and never needs to worry about making a new cursor. That way your object can have multiple cursor'd attributes, each with their own methods:
CollectionOfCollections {
  Collection> collections();
  BidirectionalCursor sizes();
}
Granted, that last usage example is a little silly... Let me know if you come up with a better one!

1 comment: