Privacy do not check reachability

[ArneBab]

No description provided.

[bertm]

This class has become a bit of a mess of tangled conditions, and through all the negations, ones and minus-ones it's somewhat difficult to review whether the logic here is sound.

If we define a clever helper method prefer that takes a Predicate<InetAddress> and returns a Comparator<InetAddress> and extract the cached isReachable check to a separate method, we could reduce this to the more descriptive below definition using Comparator's thenComparing chaining operations (assuming I reverse engineered this correctly 😃):

private final Comparator<InetAddress> innerComparator = prefer(Objects::nonNull)
		.thenComparing(prefer(not(InetAddress::isAnyLocalAddress)))
		.thenComparing(prefer(not(InetAddress::isLoopbackAddress)))
		.thenComparing(prefer(not(InetAddress::isLinkLocalAddress)))
		.thenComparing(prefer(is(InetAddress::isSiteLocalAddress).and(this::isReachable)))
		// NOTE: the last 3 lines below can also be replaced by simply 
		//   .thenComparing(InetAddressComparator.COMPARATOR);
		.thenComparing(prefer(is(Inet6Address.class::isInstance)))
		.thenComparingInt(Object::hashCode)
		.thenComparing(InetAddress::getAddress, Fields::compareBytes);

With the above defined, the actual compare implementation can be simplified to just a few lines:

@Override
public int compare(InetAddress arg0, InetAddress arg1) {
	if (Objects.equals(arg0, arg1)) {
		return 0;
	}
	return innerComparator.compare(arg0, arg1);
}

[ArneBab]

This class has become a bit of a mess of tangled conditions, and through all the negations, ones and minus-ones it's somewhat difficult to review whether the logic here is sound.
…

private final Comparator<InetAddress> innerComparator = prefer(Objects::nonNull)
...

That looks much nicer, yes. I’ll try to get there. Thank you!

[ArneBab]

We only have not since Java 11, so I implemented both prefer and preferNot.

[ArneBab]

@bertm could you have another look whether this is OK to be merged?

[Bombe]

Oh, snap, here’s a PR where I haven’t complained about missing tests yet! 🤣

[bertm]

This looks way more comprehensible already. Here's a few additional but minor remarks.

[bertm]

This can be simplified to return prefer(pred).reversed();.

[bertm]

This can be simplified to return (arg0, arg1) -> Boolean.compare(!pred.test(arg0), !pred.test(arg1));

Also avoids having to test the predicates twice.

[bertm]

The indentation is off, but this code can probably be removed as per the other comments.

[ArneBab]

Merged - thank you! @Bombe @bertm