Final or Not Final?

I recently implemented an extension to Spring's HsqlSequenceMaxValueIncrementer. When I digged into the Spring source code of HsqlSequenceMaxValueIncrementer and its ancestors, I noticed something astonishing: almost all methods are public or at least protected and there is not one final class or field. Everything is extensible and somewhat accessible.

Let's take a look at AbstractSequenceMaxValueIncrementer, one of the parent classes of HsqlSequenceMaxValueIncrementer:

public abstract class AbstractDataFieldMaxValueIncrementer implements DataFieldMaxValueIncrementer, InitializingBean {
	private DataSource dataSource;
	private String incrementerName;

	public AbstractDataFieldMaxValueIncrementer() {
	}

	public AbstractDataFieldMaxValueIncrementer(DataSource dataSource, String incrementerName) {
		Assert.notNull(dataSource, "DataSource must not be null");
		Assert.notNull(incrementerName, "Incrementer name must not be null");
		this.dataSource = dataSource;
		this.incrementerName = incrementerName;
	}

	public void setDataSource(DataSource dataSource) {
		this.dataSource = dataSource;
	}

	public DataSource getDataSource() {
		return this.dataSource;
	}

	public void setIncrementerName(String incrementerName) {
		this.incrementerName = incrementerName;
	}

	public String getIncrementerName() {
		return this.incrementerName;
	}

	public void afterPropertiesSet() {
		if (this.dataSource == null) {
			throw new IllegalArgumentException("Property 'dataSource' is required");
		}
		if (this.incrementerName == null) {
			throw new IllegalArgumentException("Property 'incrementerName' is required");
		}
	}

	public int nextIntValue() throws DataAccessException {
		return (int) getNextKey();
	}

	public long nextLongValue() throws DataAccessException {
		return getNextKey();
	}

	public String nextStringValue() throws DataAccessException {
		// removed implementation for brevity 
	}

	protected abstract long getNextKey();
}

Since I was extending the framework, the degree of flexibility was great for me. All I had to do was override a few methods to plug in my code and that's it.

If everything is accessible, the way it is in case of AbstractDataFieldMaxValueIncrementer, users can do almost everything they want with the classes you published. They can override all the methods and subclass each and every class. As shown above, that was obviously intended. The class wouldn't be abstract and have public/protected methods if it wasn't intended to be subclassed.

After reading the book Practical API Design, which preaches backward compatibility, I wonder how you would be able to evolve such a class? Let's say you wanted to extend the meaning of the field incrementerName. It should convey more information than simply the name of the sequence. Therefore you'd create a class IncrementerName, which replaced the String field. With this simple change, you'd break all the clients relying on the field incrementerName. I know this example might seem far-fetched, but I think you get the idea.

Another thing that caught my eye, when looking at AbstractDataFieldMaxValueIncrementer, is the field dataSource. It is basically a public field and I'm not quite sure why. Is it really necessary to replace the DataSource after you initialized the bean? Do you really want everyone to screw with the DataSource?

If it was for me, I would have answered those questions with a plain "No!". Why not implement AbstractDataFieldMaxValueIncrementer as follows:

public abstract class AbstractDataFieldMaxValueIncrementer implements DataFieldMaxValueIncrementer {
	private final DataSource dataSource;
	private final String incrementerName;

	public AbstractDataFieldMaxValueIncrementer(DataSource dataSource, String incrementerName) {
		Assert.notNull(dataSource, "DataSource must not be null");
		Assert.notNull(incrementerName, "Incrementer name must not be null");
		this.dataSource = dataSource;
		this.incrementerName = incrementerName;
	}

	protected DataSource getDataSource() {
		return this.dataSource;
	}

	public String getIncrementerName() {
		return this.incrementerName;
	}

	public int nextIntValue() throws DataAccessException {
		return (int) getNextKey();
	}

	public long nextLongValue() throws DataAccessException {
		return getNextKey();
	}

	public String nextStringValue() throws DataAccessException {
		// removed implementation for brevity 
	}

	protected abstract long getNextKey();
}

I think this would not restrict the use of the class, except that you have to use constructor injection, but that's a different story. It would, however, restrict the use of the fields dataSource and incrementerName. I think those fields should be immutable anyways, since I cannot think of a reason why you would want to change them during the life-cycle of the object. I know dataSource isn't truly immutable, but the access is limited to subclasses, which should confine the wrong usage.

That leaves me with the question, why the authors of AbstractDataFieldMaxValueIncrementer didn't go that route? I think it's simply a design decision of Spring authors, to be as open and extensible as possible. They have InitializingBean and @Required to enforce necessary dependencies, which work quite well. I guess they push the responsibility of handling immutables with care to the developer, which is reasonable, but in my opinion rather risky. I think using final in such cases is an easy way to remove unnecessary sources of errors.

I'm not quite sure which approach is the best here. Does it all boil down to "it depends"? Or is all a matter of taste? I don't think so. In my day to day coding, I tend to consciously follow the advices of Effective Java, which favors immutability and limiting accessibility. However, looking at Spring's source code is a truly inspiring. They are doing a phenomenal job of keeping it stable, considering how open and extensible their classes are.