2
\$\begingroup\$

Related, but in C++

I have written a program that generates scrambling algorithms for and times a solving session of a 3x3 Rubik's cube.

Requirements for scramble and timer

  • The same face cannot be turned twice in a row.
  • The same family of faces (U/D, R/L, or F/B) cannot be turned three times in a row.
  • An "average" consists of 12 solves, where the highest and lowest times are thrown out and the mean of the middle 10 solves is found.

The goal of this code review is to improve code readability/understandability, particularly with respect to the names of the variables. Also, this is one of my first programs using arrays, so optimizations for the usage of arrays here would help, especially for printing and formatting arrays.

A file is outputted with the program as well as information to the console. The file is a yet-to-be-implemented (but review-irrelevant) system that will output LaTeX code to better visualize performance. The program should be run from the console.

My name has been <HIDDEN> for privacy reasons.

import java.io.BufferedWriter;
import java.io.File;
import java.io.FileWriter;
import java.io.IOException;
import java.math.RoundingMode;
import java.text.DecimalFormat;
import java.util.Arrays;
import java.util.Random;
import java.util.Scanner;

public class RubiksCubeTimer {
    public static void main(String[] args) {
        try {
            //Generating a file for the solving session
            File file = new File("/Users/<HIDDEN>/Desktop/Rubik's Cube Times.txt");
            if(!file.exists()) {
                file.createNewFile();
            }

            FileWriter fw = new FileWriter(file.getAbsoluteFile());
            BufferedWriter bw = new BufferedWriter(fw);

            DecimalFormat df = new DecimalFormat("0.00");
            df.setRoundingMode(RoundingMode.DOWN);

            Scanner keyboard = new Scanner(System.in);
            String userInput;

            //Introduction to the software
            System.out.println("<HIDDEN>'s Rubik's Cube Timer\n");
            bw.write("<HIDDEN>'s Rubik's Cube Timer\n\n");
            printInstructions();

            final int SESSION_LENGTH = 12;

            double[] solveTimes = new double[SESSION_LENGTH];

            for(int i = 1; i <= SESSION_LENGTH; i++) {
                //Scramble
                String scramble = generateScramble();
                System.out.print(scramble);

                userInput = keyboard.nextLine();
                if(userInput.equalsIgnoreCase("End")) {
                    System.exit(0);
                }
                long startTime = System.currentTimeMillis();

                //At this point, the user solves the cube or cancels the solve

                userInput = keyboard.nextLine();
                long endTime = System.currentTimeMillis();
                if(userInput.equalsIgnoreCase("Cancel")) {
                    i--;
                    continue;
                }
                else if(userInput.equalsIgnoreCase("End")) {
                    System.exit(0);
                }
                else { //Valid Solve
                    //Formatting time
                    long solveTimeMilliseconds = endTime - startTime;
                    double solveTimeSeconds = solveTimeMilliseconds / 1000.0;

                    double solveTime = Math.floor(solveTimeSeconds * 100) / 100.0;
                    String solveTimeString = df.format(solveTimeSeconds);

                    System.out.println(solveTimeString + "\n");
                    bw.write(scramble + "\n");
                    bw.write(solveTimeString + "\n\n");

                    //Setting up array to calculate average
                    solveTimes[i - 1] = solveTime; //i is 1-indexed
                }
            }

            //Formatting session
            double[] solveTimesOrdered = new double[SESSION_LENGTH];
            for(int i = 0; i < SESSION_LENGTH; i++) {
                solveTimesOrdered[i] = solveTimes[i];
            }           
            Arrays.sort(solveTimesOrdered);
            double maximumTime = solveTimesOrdered[0];
            double minimumTime = solveTimesOrdered[SESSION_LENGTH - 1];

            String[] formattedTimes = new String[SESSION_LENGTH];

            boolean maximumFound = false, minimumFound = false;
            for(int i = 0; i < SESSION_LENGTH; i++) {
                if(!maximumFound && solveTimes[i] == maximumTime) {
                    formattedTimes[i] = "(" + df.format(solveTimes[i]) + ")";
                    maximumFound = true;
                }
                else if(!minimumFound && solveTimes[i] == minimumTime) {
                    formattedTimes[i] = "(" + df.format(solveTimes[i]) + ")";
                    minimumFound = true;
                }
                else {
                    formattedTimes[i] = df.format(solveTimes[i]);
                }
            }
            System.out.println(Arrays.toString(formattedTimes));
            bw.write(Arrays.toString(formattedTimes) + "\n");

            //Calculating average
            double average = 0;
            for(int i = 1; i < SESSION_LENGTH - 1; i++) {
                average += solveTimesOrdered[i];
            }
            average = average / (SESSION_LENGTH - 2);
            average = Math.floor(average * 100) / 100.0;
            String averageString = df.format(average);

            System.out.println("Average: " + averageString);
            bw.write("Average: " + averageString + "\n\n");

            //Finish file
            bw.close();
        } catch(IOException e) {
            e.printStackTrace();
        }
    }

    private static void printInstructions() {
        System.out.println(
            "To begin a solve, press the enter key. The timer will begin running.\n"
          + "To finish a solve, press the enter key again. The time taken to solve the cube will be displayed.\n"
          + "To delete a solve, type \"Delete.\"\n"
          + "To end a solving session, type \"End.\"\n"
          + "LaTeX code for the session will be generated at the end of the session.\n\n"); //not yet implemented
    }

    private static String generateScramble() {
        final int SCRAMBLE_LENGTH = 30;
        Random rand = new Random();

        //Picking faces to turn
        int[] scrambleFaceIntArray = new int[SCRAMBLE_LENGTH];
        for(int i = 0; i < SCRAMBLE_LENGTH; i++) {
            scrambleFaceIntArray[i] = rand.nextInt(6);

            if(i >= 1) {
                while(scrambleFaceIntArray[i] == scrambleFaceIntArray[i - 1]) {
                    scrambleFaceIntArray[i] = rand.nextInt(6);
                }

                while(i >= 2 && (scrambleFaceIntArray[i] == scrambleFaceIntArray[i - 2] && scrambleFaceIntArray[i] / 2 == scrambleFaceIntArray[i - 1] / 2)) {
                    scrambleFaceIntArray[i] = rand.nextInt(6);
                }
            }
        }

        //Replacing numbers with letters
        String[] scrambleFaceArray = new String[SCRAMBLE_LENGTH];
        for(int i = 0; i < SCRAMBLE_LENGTH; i++) {
            switch(scrambleFaceIntArray[i]) {
                case 0: scrambleFaceArray[i] = "U";
                    break;
                case 1: scrambleFaceArray[i] = "D";
                    break;
                case 2: scrambleFaceArray[i] = "R";
                    break;
                case 3: scrambleFaceArray[i] = "L";
                    break;
                case 4: scrambleFaceArray[i] = "F";
                    break;
                case 5: scrambleFaceArray[i] = "B";
                    break;
            }
        }

        //Picking directions to turn the faces
        int[] scrambleDirectionIntArray = new int[SCRAMBLE_LENGTH];
        for(int i = 0; i < SCRAMBLE_LENGTH; i++) {
            scrambleDirectionIntArray[i] = rand.nextInt(3);
        }

        //Replacing numbers with directions
        String[] scrambleDirectionArray = new String[SCRAMBLE_LENGTH];
        for(int i = 0; i < SCRAMBLE_LENGTH; i++) {
            switch(scrambleDirectionIntArray[i]) {
                case 0: scrambleDirectionArray[i] = "";
                    break;
                case 1: scrambleDirectionArray[i] = "'";
                    break;
                case 2: scrambleDirectionArray[i] = "2";
                    break;
            }
        }

        //Compiling scramble
        String[] scrambleArray = new String[SCRAMBLE_LENGTH];
        for(int i = 0; i < SCRAMBLE_LENGTH; i++) {
            scrambleArray[i] = scrambleFaceArray[i] + scrambleDirectionArray[i] + " ";
        }

        String scramble = "";
        for(int i = 0; i < SCRAMBLE_LENGTH; i++) {
            scramble += scrambleArray[i];
        }

        return scramble;
    }
}
\$\endgroup\$
1
\$\begingroup\$

Don't be afraid to delegate

Your main method does a lot of work. As an alternative, consider

    public static void main(String[] args) throws IOException {
        //Generating a file for the solving session
        File file = new File("Rubik's Cube Times.txt");
        if(!file.exists()) {
            file.createNewFile();
        }

        DecimalFormat df = new DecimalFormat("0.00");
        df.setRoundingMode(RoundingMode.DOWN);

        try (BufferedWriter bw = new BufferedWriter(new FileWriter(file.getAbsoluteFile()))) {
            //Introduction to the software
            System.out.println("<HIDDEN>'s Rubik's Cube Timer\n");
            bw.write("<HIDDEN>'s Rubik's Cube Timer\n\n");
            printInstructions();

            double[] solutionTimes = timeSolutions(bw, df);

            TimeStatistics stats = new TimeStatistics(solutionTimes, df);
            stats.calculate();
            String[] formattedTimes = stats.formatTimes();

            System.out.println(Arrays.toString(formattedTimes));
            bw.write(Arrays.toString(formattedTimes) + "\n");

            //Calculating average
            String average = stats.formatAverage();

            System.out.println("Average: " + average);
            bw.write("Average: " + average + "\n\n");
        } catch(IOException e) {
            e.printStackTrace();
        }
    }

The original version stops on an IOException and prints a stack trace. This version will still do that. But the original had the file creation in the try block. This version will throw the exception before the try block starts.

Because the try block starts later, we can use the try-with-resources form. So the BufferedWriter will close automatically regardless of how the try block is interrupted.

I like solutionTimes better as a name than solveTimes.

We delegate to timeSolutions to do the actual timing. This also does some output, as that's how the code's written. There's an argument that that should be separated.

We delegate to the new TimeStatistics class for processing the times into statistics. The calculate method has to be called before formatTimes and formatAverage. This is a bit fragile. You might consider having formatTimes and formatAverage trigger calculate if it hasn't been called yet.

There is also an argument in favor of moving the formatting methods to yet another class. As stands though, they are somewhat intertwined with the statistics.

Avoid parallel logic

    private static double[] timeSolutions(BufferedWriter writer, DecimalFormat df) throws IOException {
        double[] solutionTimes = new double[12];

        try (Scanner keyboard = new Scanner(System.in)) {
            for (int i = 0; i < solutionTimes.length; i++) {

In your original, you created a constant to replace the 12 and then you used the constant throughout the program. That's not necessary with Java arrays. They track their own length. So we can check against solutionTimes.length. So we only use the constant once, at the very beginning. Now if we change how the length of the array is set initially, we don't have to change the rest of the uses to match. The change will propagate naturally.

I used the try-with-resources to allocate the Scanner. It will get closed automatically this way. Not a huge deal with a Scanner, but it does make my IDE happy.

I changed the for loop to be zero-indexed. You only used i-1, so why not change it from one-indexed to zero-indexed and save a subtraction?

This can throw an IOException. That will get caught in the calling method. You could of course catch it here instead. It doesn't seem necessary here though. It should be a fatal error, so you want to propagate it back to the top.

\$\endgroup\$

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.