Yak Shaving is a Good Way to Improve an API

Yak Shaving (uncountable):

  1. (idiomatic) Any apparently useless activity which, by allowing you to overcome intermediate difficulties, allows you to solve a larger problem.
  2. (idiomatic) A less useful activity done to consciously or unconsciously procrastinate about a larger but more useful task.

Both interpretations of the term Yak Shaving as explained by Wiktionary are absolutely accurate descriptions of most refactoring jobs. The Yak Shaving in refactoring itself can be described by this gif showing what happens when you want to change a light bulb:

light-bulb

However, when developing an API, it’s not such a bad idea to perform actual Yak Shaving (only the first interpretation, of course). Let’s look at an example why, from the daily work maintaining jOOQ.

The Task

For jOOQ 3.6, I wanted to implement a very simple feature. Feature #2639: Add stored procedure OUT values to DEBUG log output. This is not an important feature at all, but certainly very useful to a lot of jOOQ users. The idea is that every time you run a stored procedure with DEBUG logging activated, you’ll get the OUT parameters logged along with the procedure call. Here’s a visualisation:

debug-log-2

Now, the actual implementation would have been very easy. Just about 10 lines of code in the existing LoggerListener that already takes care of logging all the other things. But there were a couple of caveats, which reminded me of the above lightbulb changing gif:

The apparently useless activities

  1. There was no way to access the RETURN_VALUE meta information of a jOOQ Routine
  2. There was no easy way to access Routine IN and OUT values generically
  3. There was lifecycle event that modelled the moment when OUT parameters are fetched in jOOQ
  4. There was no way to format Routine OUT parameters in a nice way

Does this feel familiar? There is need for refactoring!

Now, this whole implementation is hidden in jOOQ’s internals. It wouldn’t matter too much for users, if this had been hacked together in one way or another. For instance, obviously the RETURN_VALUE meta information could be accessed through internal refactorings, the same is true for IN and OUT values. There are other lifecycle events that might have worked just as well, and formatting is easy to re-implement.

But this is a popular API that is used by many users who might profit from a cleaner solution. Thus, why don’t we simply refactor and implement:

  1. Add a public Routine.getReturnParameter() method
  2. Add public Routine.getValue() and setValue() methods
  3. Add ExecuteListener.outStart(ExecuteContext) and outEnd(ExecuteContext) to capture fetching of Routine OUT parameters
  4. Add Routine.outRecord() and Routine.inRecord() to view a Routine as a Record”

The thing is:

The API implementor is the first API consumer

It’s hard to foresee what API users really want. But if you’re implementing an API (or just a feature), and you discover that something is missing, always consider adding that missing thing to the public API. If it could be useful to yourself, internally, it could be even more useful to many others. This way, you turn one little nice feature into 5, amplifying the user love.

Don’t get me wrong. This doesn’t mean that every little piece of functionality needs to be exposed publicly, au contraire. But the fact that something is keeping you – as the maintainer from writing clean code might indicate that others implement the same hacky workarounds as you. And they won’t ask you explicitly for it!

Don’t believe it? Here’s an entirely subjective analysis of user feedback:

  • 0.2% – Hey, this is a cool product, I want to help the owner make it better, I’ll provide a very descriptive, constructive feature request and engage for the next 5 weeks to help implement it.
  • 0.8% – Whatever dudes. Make this work. Please.
  • 1.3% – Whatever dudes. Make this work. ASAP!
  • 4.0% – WTF is wrong with you guys? Didn’t you at least think about this once??
  • 4.7% – OK, I’m going to write this completely uninformed rant about this product now, which I hate so much. It makes my life completely miserable
  • 9.0% – Oh well, this doesn’t work. Let’s go home, it’s 17:00 anyways
  • 80.0% – Oh well, this didn’t work yesterday already. Let’s go home. It’s Friday, 16:00 anyways

Now, most of this list wasn’t meant entirely seriously, but you get the point. There may be those 0.2% of users / customers that love you and that actively engage with you. Others may still love you or at least like you, but they won’t engage. You have to guesstimate what they need.

So. Bottom line:

If you need it, they probably need it. Start Yak Shaving!

Keeping things DRY: Method overloading

A good clean application design requires discipline in keeping things DRY:

Everything has to be done once.
Having to do it twice is a coincidence.
Having to do it three times is a pattern.

— An unknown wise man

Now, if you’re following the Xtreme Programming rules, you know what needs to be done, when you encounter a pattern:

refactor mercilessly

Because we all know what happens when you don’t:

Not DRY: Method overloading

One of the least DRY things you can do that is still acceptable is method overloading – in those languages that allow it (unlike Ceylon, JavaScript). Being an internal domain-specific language, the jOOQ API makes heavy use of overloading. Consider the type Field (modelling a database column):

public interface Field<T> {

    // [...]

    Condition eq(T value);
    Condition eq(Field<T> field);
    Condition eq(Select<? extends Record1<T>> query);
    Condition eq(QuantifiedSelect<? extends Record1<T>> query);

    Condition in(Collection<?> values);
    Condition in(T... values);
    Condition in(Field<?>... values);
    Condition in(Select<? extends Record1<T>> query);

    // [...]

}

So, in certain cases, non-DRY-ness is inevitable, also to a given extent in the implementation of the above API. The key rule of thumb here, however, is to always have as few implementations as possible also for overloaded methods. Try calling one method from another. For instance these two methods are very similar:

Condition eq(T value);
Condition eq(Field<T> field);

The first method is a special case of the second one, where jOOQ users do not want to explicitly declare a bind variable. It is literally implemented as such:

@Override
public final Condition eq(T value) {
    return equal(value);
}

@Override
public final Condition equal(T value) {
    return equal(Utils.field(value, this));
}

@Override
public final Condition equal(Field<T> field) {
    return compare(EQUALS, nullSafe(field));
}

@Override
public final Condition compare(Comparator comparator, Field<T> field) {
    switch (comparator) {
        case IS_DISTINCT_FROM:
        case IS_NOT_DISTINCT_FROM:
            return new IsDistinctFrom<T>(this, nullSafe(field), comparator);

        default:
            return new CompareCondition(this, nullSafe(field), comparator);
    }
}

As you can see:

  • eq() is just a synonym for the legacy equal() method
  • equal(T) is a more specialised, convenience form of equal(Field<T>)
  • equal(Field<T>) is a more specialised, convenience form of compare(Comparator, Field<T>)
  • compare() finally provides access to the implementation of this API

All of these methods are also part of the public API and can be called by the API consumer, directly, which is why the nullSafe() check is repeated in each method.

Why all the trouble?

The answer is simple.

  • There is only very little possibility of a copy-paste error throughout all the API.
  • … because the same API has to be offered for ne, gt, ge, lt, le
  • No matter what part of the API happens to be integration-tested, the implementation itself is certainly covered by some test.
  • This way, it is extremely easy to provide users with a very rich API with lots of convenience methods, as users do not want to remember how these more general-purpose methods (like compare()) really work.

The last point is particularly important, and because of risks related to backwards-compatibility, not always followed by the JDK, for instance. In order to create a Java 8 Stream from an Iterator, you have to go through all this hassle, for instance:

// Aagh, my fingers hurt...
   StreamSupport.stream(iterator.spliterator(), false);
// ^^^^^^^^^^^^^                 ^^^^^^^^^^^    ^^^^^
//       |                            |           |
// Not Stream!                        |           |
//                                    |           |
// Hmm, Spliterator. Sounds like      |           |
// Iterator. But what is it? ---------+           |
//                                                |
// What's this true and false?                    |
// And do I need to care? ------------------------+

When, intuitively, you’d like to have:

// Not Enterprise enough
iterator.stream();

In other words, subtle Java 8 Streams implementation details will soon leak into a lot of client code, and many new utility functions will wrap these things again and again.

See Brian Goetz’s explanation on Stack Overflow for details.

On the flip side of delegating overload implementations, it is of course harder (i.e. more work) to implement such an API. This is particularly cumbersome if an API vendor also allows users to implement the API themselves (e.g. JDBC). Another issue is the length of stack traces generated by such implementations. But we’ve shown before on this blog that deep stack traces can be a sign of good quality.

Now you know why.

Takeaway

The takeaway is simple. Whenever you encounter a pattern, refactor. Find the most common denominator, factor it out into an implementation, and see that this implementation is hardly ever used by delegating single responsibility steps from method to method.

By following these rules, you will:

  • Have less bugs
  • Have a more convenient API

Happy refactoring!

How to Eliminate Bugs Through High Cohesion

Intuition tells us that methods like these ones suffer from a distinct code smell:

CompilationTask getTask(
    Writer out,
    JavaFileManager fileManager,
    DiagnosticListener<? super JavaFileObject> 
        diagnosticListener,
    Iterable<String> options,
    Iterable<String> classes,
    Iterable<? extends JavaFileObject> 
        compilationUnits
);

Why is that so? Let’s delve into this intuition. Here is an example from the JavaCompiler Javadoc:

Iterable<? extends JavaFileObject> compilationUnits1 =
    fileManager.getJavaFileObjectsFromFiles(
        Arrays.asList(files1));

compiler.getTask(null, fileManager, null, 
                 null, null, compilationUnits1)
        .call();

So what’s wrong here? We have a lot of very disjunctly typed parameters that are very likely to be set to null. This decreases the above method’s reusability, or in the terms of the JArchitect guys, we’re probably in the “Zone of Pain” as we have a low level of stability coupled with a low level of abstractness.

  • Low stability: it is very likely that we’re going to need another very specific argument in a future version of the JavaCompiler, e.g. another Iterable of something. This will make for incompatible API enhancement
  • Low abstractness: Even if the above is an interface method, there is very little chance of this method being implemented more than once, as it is quite hard to fulfil the above contract in a useful way.

A common way to circumvent this issue for single methods is to use the builder pattern as Petri Kainulainen nicely described it.

High cohesion instead of “Zone of Pain”

Maybe, for this compiler API, this isn’t too important you may think. But the biggest value of “high cohesion”, i.e. of an ideal stability / abstractness balance is the fact that you have highly reusable code. This isn’t just good because your developers spend less time implementing a specific task, it also means that your code es extremely error-resistant. For example, check out the data type conversion logic from the internals of jOOQ:

jOOQ's data type conversion call hierarchy

jOOQ’s data type conversion hierarchy

The above is just an extract of the call hierarchy leading towards a single data type conversion API that is indirectly used in the whole framework. Everything leads through there, so if there is any data type conversion bug, it is either

  • Extremely local to a single method / single leaf of the above tree representation
  • Extremely global to the whole tree

In other words, any bug related to data type conversion is either merely cosmetic, or completely catastrophic. Which basically means that there is almost no possibility for a regression in that area, as any data type conversion regression will immediately break hundreds of unit and integration tests. This is a major benefit of having high cohesion in your code.

How to attain high cohesion

It’s simple: By refactoring mercilessly. You should never introduce a new feature only locally. For instance, let’s consider this fix here [#3023] DefaultRecordMapper does not map nested UDTs onto nested POJOs. So we want the jOOQ RecordMapperProvider feature to be applied to nested records. Why? Imagine we have a PERSON table with Oracle OBJECT types for ADDRESS and STREET properties. Yes, you could also just normalise this data, but imagine we are using UDTs:

CREATE TYPE street_type AS OBJECT (
  street VARCHAR2(100),
  no VARCHAR2(30)
);

CREATE TYPE address_type AS OBJECT (
  street street_type,
  zip VARCHAR2(50),
  city VARCHAR2(50)
);

And now, we would like to recursively map this data onto custom nested POJOs:

public class Street {
    public String street;
    public String number;
}

public class Address {
    public Street street;
    public String city;
    public String country;
}

public class Person {
    public String firstName;
    public String lastName;
    public Address address;
}

And the mapping should be available through:

// The configuration object contains the
// Mapping algorithm implementation
Person person = DSL.using(configuration)
                   .selectFrom(PERSON)
                   .where(PERSON.ID.eq(1))

// We want to make the mapping algorithm recursive
// to automatically map Address and Street as well
                   .fetchOneInto(Person.class);

Mapping of a Record onto a POJO is already implemented, but recursion is not. And when we do implement recursion, we want to respect the existing, aforementioned customisable mapping SPI that was introduced in jOOQ 3.1. It’s very simple, we just have a single implementation point at the top in the ConvertAll type.

Implementing this in a highly cohesive code base means that:

  • We have to implement this new feature only once
  • Implementing this new feature costs less effort than writing this blog post
  • Nesting of record mapping and conversion will work for all use-cases in one go
  • We have only slightly increased complexity (low risk of bugs) while adding an awesome new feature

Do you refactor mercilessly? tweet this

The perfect design cannot be foreseen. It grows, slowly. Today, we know so many things about Java and collections, it took a while for the new Streams API to surface. No one would have implemented such a great new API in the JDK 1.2 from scratch, although from that perspective, it has already been pretty good at the time.

This mainly means two things for you:

  • For your essential core code, it is important to get it to a state where you attain high cohesion. If you’re an E-Banking vendor, your payment and brokerage logic should be exactly as above, with a balanced stability / abstractness ratio
  • For your non-essential code (e.g. UI / DB-access), you should rely on third-party software, because someone else will spend a lot more time at getting their code on a high level of quality (UI: such as Vaadin, ZK or DB-access: such as Hibernate, jOOQ, Spring Data, just to name a few)

… and if you request a new feature from a highly cohesive framework, it might just be that the only thing that needs to be done is these four lines of code.