4
\$\begingroup\$

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 depleted the list is renewed with the same pieces.
     */


    private static class Generator{
        private int pieceNumber; // Number of pieces that will be generated.
        private String generation = ""; // Used to store the piece generation.
        private final String[] pieces = {"O", "I", "J", "L", "S", "Z", "T"}; // Pieces used in the generation.

        /**
         * Constructor used to store the number of pieces that need to be
         * generated and execute the piece generation method.
         * @param pieceNumber Used to determine how long the generation should be.
         */
        Generator(int pieceNumber){
            this.pieceNumber = pieceNumber; // Initializes number of pieces.
            generatePieces(); // Generates piece combination.
        }

        /**
         * Used to generate the pieces in the specific order. Manipulates class instanced variables.
         */
        private void generatePieces(){
            ArrayList<String> temp = new ArrayList<>(Arrays.asList(pieces)); // Used to store all pieces.
            while(generation.length() <= pieceNumber){ // Generates string until it is the specified size.
                if(temp.isEmpty()) // Checks if list is empty.
                    temp = new ArrayList<>(Arrays.asList(pieces)); // Replaces the pieces in the list.
                Collections.shuffle(temp); // Shuffles the list
                generation += temp.get(0); // Places the first element of the list onto the string.
                temp.remove(0); // Removes the element that was added from the list.
            }
        }

        /**
         * Returns the generation constructed by the generatePieces method.
         * @return The string generated.
         */
        @Override
        public String toString(){
            return generation; // Returns piece generation once generated.
        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ I had no idea that modern Tetris used a single bag randomizer. That sounds extremely boring and predictable. \$\endgroup\$ – JS1 Jul 2 '16 at 3:12
  • \$\begingroup\$ @JS1 The game uses this method to make sure the player doesn't receive the same piece to often and also to make sure the game doesn't go to long without giving a certain piece. \$\endgroup\$ – JSextonn Jul 2 '16 at 3:22
  • \$\begingroup\$ I know but it sounds like easy mode to me. I bet players complained about the rng and they caved to the pressure. \$\endgroup\$ – JS1 Jul 2 '16 at 4:10
  • \$\begingroup\$ @JS1 Maybe, but in a way it adds room for more skill when playing the game too. Players not only have to judge where the best position for the current piece is, but also remember which pieces they've gotten so far. This gives players the ability to guess what the next piece is yes, but would give them a step up on other players if they were able too. \$\endgroup\$ – JSextonn Jul 2 '16 at 4:18
6
\$\begingroup\$

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 <T> Stream<T> stream(T[] elements) {
        return Stream.iterate(Arrays.asList(elements), list -> {
            Collections.shuffle(list = new ArrayList<T>(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.

\$\endgroup\$
  • \$\begingroup\$ Thanks a lot for all the critiques and the suggested solution. Had to research Stream's generate method a bit, but think I got it. Is it possible to turn the get method within the supplier into a lambda expression in this case? if so what would that look like? \$\endgroup\$ – JSextonn Jul 2 '16 at 6:17
  • \$\begingroup\$ Excellent question, which we can discuss in chat. \$\endgroup\$ – 200_success Jul 2 '16 at 6:33

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.