Consider moving Seq implementations from Seq to SeqImpl, avoiding default methods where they aren't "defaults" but "trait implementations"

[lukaseder]

No description provided.

[stephenh]

Hey, just curious why you're wanting to avoid the default methods? Just wondering if you have a lesson learned I could pick up.

[lukaseder]

Just a matter of style. I've realised that suddenly, most implementation has moved to the interface, rather than the backing class and that feels kind of weird for a "default" method. Seq as it is now is a trait, not an interface.

I realise that the title is a bit too agressive. I'm undecided yet, whether this change should be made or not.

[stephenh]

Cool, that makes sense. Thanks for the insight!

[lukaseder]

@stephenh Hmm, just encountered another issue, proving my concerns to be justified:

I've implemented Collectable (#166), a common super type for Seq and the new Window. Both Seq and Window can produce streams, but Window is not a stream. Yet, as they can produce streams, they can both aggregate streams, too, hence the new common super type, where all the aggregations are declared.

Now, what happens to, for instance:

long count();

In Seq, it is inherited from Stream. In Collectable, I'd like to implement it as such:

default long count() {
    collect(Collectors.counting());
}

But because Seq now extends Stream and Collectable, I get a conflict because I'm inheriting the same method from two interfaces:

• abstract from Stream
• default from Collectable

Seq no longer compiles.

Default methods can be a very tricky thing from a backwards-compatibility point of view. It's dangerous to provide default methods, unless there's really no other way...

[tlinkowski]

But because Seq now extends Stream and Collectable, I get a conflict because I'm inheriting the same method from two interfaces:

• abstract from Stream
• default from Collectable

Seq no longer compiles.

Well, you could have overcome this by providing a default for count in Seq:
default long count() {
return Collectable.super.count();
}
But I see you've removed the default implementation from Collectable so you solved it in another way.

[tlinkowski]

Default methods can be a very tricky thing from a backwards-compatibility point of view. It's dangerous to provide default methods, unless there's really no other way...

Well, having some C# background, I treat default methods as kind of extension methods wherever possible. Here's how I see it:

Pros of default methods:

1. Thanks to them, one can clearly see what's the main part of the interface, and what's just an extension to it. This is crucial if you want to implement the interface - this lets you understand what the interface really represents.
2. Calls to default methods look much nicer than calls to static utility methods accepting the interface as their first argument (if anyone considered this as a solution to the problem above).

Cons of default methods:

1. Potential clashes between interface methods (but can be solved, as shown above).
2. And the greatest calamity - interfaces become implementation-ridden.

Here's what I try to use in my projects to address the above: I stick to default methods but delegate any non-trivial implementation (say, longer than 2 or 3 lines) to a static method in some (potentially package-private) utility class.

For example:

• Seq.leftOuterJoin(Seq,BiPredicate) should delegate to SeqJoins.leftOuterJoin(Seq,Seq,BiPredicate)
• Seq.window(*) should delegate to SeqWindows.window(Seq,*), etc.

In the same way, we could delegate all the many static methods in Seq. IMHO most of these methods (except static factories) shouldn't be present in Seq at all but since you probably don't want to break backwards compatiblity by removing them, delegating them would do:

• Seq.zip(Seq,Seq,BiFunction) to SeqZippers.zip(Seq,Seq,BiFunction), etc.
• Seq.grouped(Seq,Function) to SeqOperations.grouped(Seq,Function)
• Seq.partition(Stream,Predicate) to SeqOperations.partition(Stream,Function)
• etc.

JVM will optimize such calls anyway so there's no performance penalty but thanks to such approach, the Seq interface would be so much lighter in terms of lines of code (it now has over 8k lines!).

[lukaseder]

Thanks to them, one can clearly see what's the main part of the interface, and what's just an extension to it. This is crucial if you want to implement the interface - this lets you understand what the interface really represents.

I'd agree if there was the possibility to define final methods in interfaces. But without them, I'm not so sure if we can treat default methods just like methods in (abstract) classes.

I can say that in Seq, default was mostly chosen out of laziness.

Seq.zip(Seq,Seq,BiFunction) to SeqZippers.zip(Seq,Seq,BiFunction), etc.

I have never been a big fan of this practice of having "almost Seq" types that do operations for Seq not in Seq. This opinion originates from experience when maintaining the jOOQ API. There is only a single class to look for any API entry point, not dozens. After all, all these naming choices are absolutely random and usually not very far sighted, and thus subject to more change. Which leads to the question: Why not just keep everything in Seq, where people will look for things, first?

JVM will optimize such calls anyway

One caveat: As of Java 8, default methods are not (yet) inlined: http://www.scala-lang.org/blog/2016/07/08/trait-method-performance.html

[tlinkowski]

I'd agree if there was the possibility to define final methods in interfaces. But without them, I'm not so sure if we can treat default methods just like methods in (abstract) classes.

Good point :)

I have never been a big fan of this practice of having "almost Seq" types that do operations for Seq not in Seq. This opinion originates from experience when maintaining the jOOQ API. There is only a single class to look for any API entry point, not dozens. After all, all these naming choices are absolutely random and usually not very far sighted, and thus subject to more change. Which leads to the question: Why not just keep everything in Seq, where people will look for things, first?

OK, I can accept that you prefer to have all these methods in Seq (single entry point). This, however, does not mean that all the implementations must be in Seq because the methods in Seq can simply delegate to package-private utility classes like SeqZippers, SeqWindows. The only price we pay is the duplication of method declarations in Seq and these utility classes.

One caveat: As of Java 8, default methods are not (yet) inlined: http://www.scala-lang.org/blog/2016/07/08/trait-method-performance.html

I didn't realize that but what I meant is the inlining of static methods to which the default methods delegate.

Here's an example of what I mean in case it's not entirely clear:
public interface Seq {
default Seq<Tuple2<Window<T>, Window<T>>> window(
WindowSpecification<T> specification1,
WindowSpecification<T> specification2
) {
return SeqWindows.window(this, specification1, specification2);
}
}

class SeqWindows {
static Seq<Tuple2<Window<T>, Window<T>>> window(
Seq<T> seq,
WindowSpecification<T> specification1,
WindowSpecification<T> specification2
) {
List<Tuple2<T, Long>> buffer = seq.zipWithIndex().toList();

Map<?, Partition<T>> partitions1 = SeqUtils.partitions(specification1, buffer);
Map<?, Partition<T>> partitions2 = SeqUtils.partitions(specification2, buffer);

return Seq.seq(buffer)
.map(t -> tuple(
(Window<T>) new WindowImpl<>(t, partitions1.get(specification1.partition().apply(t.v1)), specification1),
(Window<T>) new WindowImpl<>(t, partitions2.get(specification2.partition().apply(t.v1)), specification2)
))
.onClose(seq::close);
}
}

[tlinkowski]

One caveat: As of Java 8, default methods are not (yet) inlined: http://www.scala-lang.org/blog/2016/07/08/trait-method-performance.html

I didn't realize that but what I meant is the inlining of static methods to which the default methods delegate.

Now that I considered it again, I realized the static methods might not be inlined into the body of the default methods because of their length (these static methods may be too long). This would leave us with inlining the calls to the default methods which, as you pointed out, is not yet possible.

Nevertheless, I'm still in favor of extracting many of these methods. For static methods (like Seq.zip(Seq,Seq)) it won't matter (in terms of performance), and for default methods it will matter only slightly now, and probably not at all in future.

[lukaseder]

This, however, does not mean that all the implementations must be in Seq

That's what this issue is about :)

because the methods in Seq can simply delegate to package-private utility classes like SeqZippers, SeqWindows.

Why not just put everything in SeqImpl, which is already there, and which is what's suggested by this issue?

I didn't realize that but what I meant is the inlining of static methods to which the default methods delegate.

Even better, avoid default methods and implement classic virtual methods in SeqImpl

Nevertheless, I'm still in favor of extracting many of these methods.

Maybe I misunderstood you somewhere in the discussion. The intention of the issue is clear. Somehow, you have a very different change in mind (I believe). What's the intention of the change you have in mind?

[tlinkowski]

OK, as a foreword: I understand that the original proposal consist in moving the implementations from Seq by making all the (currently default) methods in Seq abstract, and implementing them in SeqImpl (and I've just recalled that this is what has been done to Collectable).

What I propose is to move the implementations from Seq by delegating them to various package-private utility classes.

My intention is just that Seq be easier to custom-implement if need be. Assuming you need a wrapper over Seq implementing Seq, currently you have to delegate all the Stream methods (*), Seq.stream() and most of Collectable but that's it. However, if we get rid of the default methods from Seq, you'll have to delegate every single method (like recently added findLast(T) and findLast(Predicate<T>) 😉).

To sum up, it seems we both agree that Seq, in its current form, displays too much implementation detail for an interface. And I'd like a compromise: provide the default implementations in Seq but hide them from Seq.java by delegating to utility classes.

As a result, Seq.java readability would considerably increase, and perhaps it would be easier for IDEs like IntelliJ to handle it 😛 And even if you move everything to SeqImpl, as you propose, then SeqImpl will become hard to browse because of all these zip and window implementations, and we're back to square one.

(*) BTW: Why some of the Stream methods aren't default in the first place? I mean collect(Collector), forEachOrdered, min, max, noneMatch or toArray(). I guess there is some rationale behind it (maybe the same that you had with Collectable) but I just don't really understand it yet...