Tetris random piece generator

I was inspired by this Reddit /r/dailyprogrammer challenge:


Contrary to popular belief, the tetromino pieces you are given in a game of Tetris are not randomly selected. Instead, all seven pieces are placed into a "bag." A piece is randomly removed from the bag and presented to the player until the bag is empty. When the bag is empty, it is refilled and the process is repeated for any additional pieces that are needed.


The pieces are as follows: O, I, S, Z, L, J, T

I was just wanting some general feedback on the code regarding readability, structure, and efficiency.

```
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.Scanner;

/**
* Created on 6/23/2016.
*
* Inspired by r/dailyprogrammer
* https://www.reddit.com/r/dailyprogrammer/comments/3ofsyb/20151012_challenge_236_easy_random_bag_system/
*/

public class RandomBag {

/**
* This is the main method. The main acquires input from the user
* then generates a combination of pieces in a particular order.
* @param args Unused.
*/
public static void main(String args[]){
Scanner scanner = new Scanner(System.in); // Used to obtain user input.
loop : while (true) {
System.out.print(">>> ");
String input = scanner.nextLine().replace(" ", "");

// Checks for special cases.
switch (input) {
case "exit" : break loop;
case "" : continue loop;
}

// The generator class will return a string of its results.
System.out.println(new Generator(Integer.parseInt(input)));
}
}

/**
* Created on 6/23/2016.
* Used to create a bag that generates a set amount of pieces in a combination
* that will never have the same piece displayed 3 times in a row. This is
* accomplished by placing all pieces in a list and removing them one by one.
* Once the list has been deple

The hard part of the challenge is already done for you by Collections.shuffle(). I'll focus on style, efficiency, and elegance.

In the main() function, scanner.nextLine().replace(" ", "") is a weird thing to do. Would it be acceptable to interpret 5 0 as 50? Perhaps you meant to do String.trim()?

Style

You have written a comment on every single line, which is too much. Only beginner programmers are likely to want to state everything twice. This comment, for example, is silly:

Collections.shuffle(temp); // Shuffles the list

The two lines above it aren't good either. The // Checks if list is empty comment is noise. You should not omit the "optional" braces. You should pick a better variable name than temp.

if(temp.isEmpty()) // Checks if list is empty.
    temp = new ArrayList<>(Arrays.asList(pieces)); // Replaces the pieces in the list.

Efficiency

Repeated concatenation of strings using += is not recommended. Concatenating n characters that way is O(n2), since each result has to be rebuilt from scratch. Instead, use a StringBuilder (if you don't know the length of the final result) or a char[] (if you know exactly how long the final string will be).

Calling .remove(0) on an ArrayList is also not recommended. To fill in the gap after removal, it has to copy over all of the subsequent elements. Emptying an n-element ArrayList by repeatedly calling .remove(0) is O(n2). Instead of removing the first element, you could remove the last element. Better yet, just keep track of the index yourself, so that you don't have to remove anything, and you don't have to keep re-creating the list.

Elegance

A more elegant solution, I think, would be to generate an infinite stream, then take an excerpt from it of the desired length. Such an infinite stream could actually be used as part of a real Tetris implementation. Your Generator, on the other hand, has to be instantiated with a length limit (awkwardly named pieceNumber).

Suggested solution

This solution, using streams, produces an infinite sequence, then limits it to length 50.

You can easily tweak it to work like your interactive loop.

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.stream.Collectors;
import java.util.stream.Stream;

public class ShuffledStream {
    private ShuffledStream() {}   // Suppress the default constructor

    /**
     * Produces an infinite stream of the elements, reshuffling when
     * each batch is exhausted.
     */
    public static  Stream stream(T[] elements) {
        return Stream.iterate(Arrays.asList(elements), list -> {
            Collections.shuffle(list = new ArrayList(list));
            return list;
        }).skip(1)              // First batch is unshuffled; skip it
          .flatMap(List::stream);
    }

    public static void main(String[] args) {
        String fifty = ShuffledStream.stream("OISZLJT".split(""))
                                     .limit(50)
                                     .collect(Collectors.joining());
        System.out.println(fifty);
    }
}

Note that since this solution uses generics, you can reuse the same code to deal from 52-card decks.

StackExchange Code Review Q#133679, answer score: 6