Calculating set difference of lists with or without intermediate variables

Question

Which form of the method do you find more readable?

Version with local variables:

public void checkRetrievedIds( List<Integer> expectedIds, List<Integer> retrievedIds ) {
    Set<Integer> expected = new HashSet<>( expectedIds );
    Set<Integer> retrieved = new HashSet<>( retrievedIds );

    Set<Integer> difference = Sets.difference( expected, retrieved );
    createInvalidationEvents( difference );
}

Or the version without local variables:

public void checkRetrievedIds( List<Integer> expectedIds, List<Integer> retrievedIds ) {
    createInvalidationEvents( Sets.difference(
        new HashSet<>( expectedIds ),
        new HashSet<>( retrievedIds )
    ) );
}

I'm considering the second version as more readable as it reveals the method intent more clearly.

Answer 1

I'd prefer

public void checkRetrievedIds( List<Integer> expectedIds, List<Integer> retrievedIds ) {
    Set<Integer> difference = Sets.difference(
        new HashSet<>( expectedIds ),
        new HashSet<>( retrievedIds )
    );

    createInvalidationEvents( difference );
}

You're right that your first version is somewhat verbose for the task. However, there are some good reasons to separate out the difference calculation from the function call. First, what if you wanted to do something else with the difference? You'd immediately have to refactor to something like this version so as to have the difference to manipulate.

I'm also somewhat leery of a function that consists entirely of chained calls. One has to sort of mentally unwrap it in order to get to the real meaning. They often make a lot of sense when writing them, as you know exactly what you are trying to accomplish. They can make a lot less sense six months down the road.

Separating the two allows us to basically have one line that calculates the set difference of two lists while the other line makes use of that difference. That seems a reasonable division of responsibility.

Another possibility would be to change the signature of createInvalidationEvents to take two lists as input. Then you wouldn't need checkRetrievedIds. Of course, that wouldn't work if createInvalidationEvents is used elsewhere with the current signature.

A third possibility would be to calculate the set difference in whatever calls checkRetrievedIds. This would also get rid of checkRetrievedIds.

As written, checkRetrievedIds is simply an alias for createInvalidationEvents with a different method signature. Without other context, it's unclear why you need it.