3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ +1 for the second version. Your variables have good names, so this is easily the clearest. \$\endgroup\$ – RobAu Nov 4 '14 at 10:57
2
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Mentally, chained calls are natural for me. I prefer to refactor variable extraction on demand. Whether I see local variable, I assume there is a proof of its existence (e.g. it's used twice), otherwise it could be misleading. \$\endgroup\$ – raindev Nov 4 '14 at 11:58
  • \$\begingroup\$ I like your variation as it's less verbose than the very first version. On the other side, it still separates different actions as some people find call chaining hard to read. \$\endgroup\$ – raindev Nov 4 '14 at 12:00
  • \$\begingroup\$ About another options you've suggested. Changing createInvalidationEvents() signature to accept two lists is not so good from application logic perspective in my particular case. Calculation of the lists difference seems as an appealing variant to me. It would be more intent revealing option as it will reduce indirection. I'd like to have less verbose way to calculate lists difference in Java still. \$\endgroup\$ – raindev Nov 4 '14 at 12:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.