@Leon Excellent question! Could you please join me in chat?

Hi!

hey whats up

I'll be honest: I had originally written the solution with a lambda.

This is what it looked like:

public class ShuffledStream<T> {
    private final List<T> list;
    private int i = -1;

    public ShuffledStream(T[] elements) {
        this.list = Arrays.asList(elements);
    }

    public Stream<T> stream() {
        return Stream.generate(() -> {
            if (i < 0) {
                Collections.shuffle(list);
                i = list.size() - 1;
            }
            return list.get(i--);
        });
    }
}

yea i saw in the documentation that you could use lambda expressions with suppliers

Take a few minutes to study the code, then tell me what you think.

i just figured you were trying to keep it simple

Actually, I was trying to keep it correct.

oh? using lambda isnt correct? or?

It's wrong, but it's not obvious why. =)

hmmm

let me see if i can figure it out

Sure. Take your time.

It's not buggy per se, but it's certainly a design with a pitfall.

yea i got nothing, lmao

looks clean to me

OK. There are two strikes against this code.

The first reason (the weak reason) is that, as a rule of thumb, if a class contains two methods, one of which is a constructor, you might want to reconsider whether it needs to be a class.

Writing new ShuffledStream(blah).stream() is cumbersome, and it would be nicer if the caller could just write ShuffledStream.stream(blah).

That was the first consideration, an aesthetic one.

The technical objection is… what if you construct a new ShuffledStream(blah), then call .stream() on it twice, to obtain two streams?

its just going to pick back off where it left off right?

back up*

The caller might have a reasonable intuitive expectation that the two streams would be independent.

But they're not. They share the same i, since i belongs to the ShuffledStream object, not the stream that was returned.

cant just declare the i within the stream method?

So, as you see, it's like hiring two dealers to deal from the same deck of cards. Not buggy per se, but certainly prone to misunderstanding. The way I wrote it with a single function eliminates the possibility of that misunderstanding.

So, you ask, why not write it this way…?

public class ShuffledStream {
    public static <T> Stream<T> stream(T[] elements) {
        List<T> list = Arrays.asList(elements);
        int i = -1;

        return Stream.generate(() -> {
            if (i < 0) {
                Collections.shuffle(list);
                i = list.size() - 1;
            }
            return list.get(i--);
        });
    }
}

The reason is simple. You can't.

ShuffledStream.java:11: error: local variables referenced from a lambda expression must be final or effectively final
            if (i < 0) {
                ^
ShuffledStream.java:13: error: local variables referenced from a lambda expression must be final or effectively final
                i = list.size() - 1;
                ^
ShuffledStream.java:15: error: local variables referenced from a lambda expression must be final or effectively final
            return list.get(i--);
                            ^

ah well thats a fair reason i guess

haha

That's just a Java language rule. If a lambda captures a local variable, then that local variable must be final or effectively final.

So, I can't use a lambda; I have to write an anonymous inner class to hold that state.

hmm, interesting

sorry for such long responses, lot of reading going on, haha

Yeah. Interesting follow-up question. I'm glad you asked it.

Well you've been a huge help, ide offer to buy you a beer if I could

was just reading my other structure question you answered as well

you've gotten me to think very differently that's for sure

Anyway, I'm glad to have helped. Hope to see you around some more on Code Review!

For sure!

Take care, and thanks again

Bye!