Petals Around the Rose

Question

I have been working on a program that walks you through "Petals Around the Rose". I would like anyone to review it and point out any bad coding practices, as well as efficiency problems.

import java.util.Scanner;

public class Main {

    static Scanner sc = new Scanner(System.in);

    public static void main(String[] args) {
        byte[] practice = getResults(5);
        System.out.println("The name of the game is Petals Around the Rose."
                         + "\nThe name is important. I will roll five dice,"
                         + "\nand I will tell you how many petals appear."
                         + "\n\nFor example:"
                         + "\n" + getFormattedDice(practice)
                         + "Will result in " + getAnswer(practice) + "."
                         + "\n\nIf you answer correctly 8 times in a row, you"
                         + "\nwill be declared a \"Potentate of the Rose\".\n");
        byte streak = 0;
        while(true) {
            if(playOnce(getResults(5))) {
                streak++;
            }
            if(streak == 8) {
                System.out.println("You are now declared a \"Potentate of the Rose\"!");
                break;
            }
        }
        System.out.println("Thank you for playing!");
        sc.close();
    }

    private static boolean playOnce(byte[] nums) {
        System.out.println("How many petals here?\n" + getFormattedDice(nums));
        byte guess = 0;
        byte answer = getAnswer(nums);
        boolean valid = false;
        while (!valid) {
            try {
                guess = sc.nextByte();
                valid = true;
            } catch (Exception e) {
                sc.next();
                System.out.print("\nOops! That is not a number. Try again: ");
            }
        }
        if (guess == answer) {
            System.out.println("\nCorrect!");
            return true;
        }
        System.out.println("\nIncorrect. The answer is " + answer + ".");
        return false;
    }

    private static byte getAnswer(byte[] nums) {
        byte answer = 0;
        for (byte b : nums) {
            if (b == 3) {
                answer += 2;
            } else if (b == 5) {
                answer += 4;
            }
        }
        return answer;
    }

    private static byte[] getResults(int amt) {
        byte[] nums = new byte[amt];
        for (int i = 0; i < amt; i++) {
            nums[i] = rollADie(6);
        }
        return nums;
    }

    private static String getFormattedDice(byte[] nums) {
        final int SIZE = 8;
        final String DOT = "\u00b7";
        final String EMPTY = "       ";
        final String SPACE_5 = "     ";
        final String SPACE_3 = "   ";
        final String SPACE = " ";
        StringBuffer[] result = new StringBuffer[] {
                new StringBuffer(SIZE * nums.length + 1),
                new StringBuffer(SIZE * nums.length + 1),
                new StringBuffer(SIZE * nums.length + 1) };
        result[0].append("|");
        result[1].append("|");
        result[2].append("|");
        for (byte num : nums) {
            switch (num) {
            case (byte) 1:
                result[0].append(EMPTY + "|");
                result[1].append(SPACE_3 + DOT + SPACE_3 + "|");
                result[2].append(EMPTY + "|");
                break;
            case (byte) 2:
                result[0].append(SPACE + DOT + SPACE_5 + "|");
                result[1].append(EMPTY + "|");
                result[2].append(SPACE_5 + DOT + SPACE + "|");
                break;
            case (byte) 3:
                result[0].append(SPACE + DOT + SPACE_5 + "|");
                result[1].append(SPACE_3 + DOT + SPACE_3 + "|");
                result[2].append(SPACE_5 + DOT + SPACE + "|");
                break;
            case (byte) 4:
                result[0].append(SPACE + DOT + SPACE_3 + DOT + SPACE + "|");
                result[1].append(EMPTY + "|");
                result[2].append(SPACE + DOT + SPACE_3 + DOT + SPACE + "|");
                break;
            case (byte) 5:
                result[0].append(SPACE + DOT + SPACE_3 + DOT + SPACE + "|");
                result[1].append(SPACE_3 + DOT + SPACE_3 + "|");
                result[2].append(SPACE + DOT + SPACE_3 + DOT + SPACE + "|");
                break;
            case (byte) 6:
                result[0].append(SPACE + DOT + SPACE_3 + DOT + SPACE + "|");
                result[1].append(SPACE + DOT + SPACE_3 + DOT + SPACE + "|");
                result[2].append(SPACE + DOT + SPACE_3 + DOT + SPACE + "|");
                break;
            default:
                throw new IllegalArgumentException(
                        "A number higher than 6 in array.");
            }
        }
        return new String(result[0]) + "\n" + new String(result[1]) + "\n"
                + new String(result[2]) + "\n";
    }

    private static byte rollADie(int i) {
        return (byte) (Math.random() * i + 1);
    }

}

Related Links

Bill Gates and Petals Around the Rose

Wikipedia

Answer 1

I see you're writing Java code but you're not using object orientation at all. I think it would be nice if you think at how you could structure in classes your program.

Why are you using a static field for your sc variable? I'd also change its name to scanner, it will improve the readability of your code.

I think you should also create some more functions. Looking at your main method I immediately felt it needs at least to be split in showHelp() and in playGame(), or something similar. As I said earlier, you could probably get a better result if you introduce some classes.

playOnce() looks a bit too complex as it mixes your game logic with dealing with user interaction. Ideally they should be separated. You should have a method that deals with reading the number from the keyboard (dealing with possible errors as you do) and another one that takes as parameter the number inserted by the user and determines whether it is the right one.

Why do you use Math.random() when you could use Random.nextInt(int n)? Both the methods work, but I think the second is closer to the result you want to achieve.

You can probably also replace most of the code you use to generate the formatted representation of the string if you create some methods like String formatRow(List<int> numberOfSpaces) that takes as parameter some integers that represents the number of spaces and puts dots between them.

You asked for comments on efficiency. I think that the problem you are addressing is simple enough that any reasonable implementation will perform well. I recommend you to focus on how you structure your code and on how to make it readable and easy to maintain rather than doing some micro-optimisations that you won't ever notice.

Answer 2

    while(true) {
        if(playOnce(getResults(5))) {
            streak++;
        }
        if(streak == 8) {
            System.out.println("You are now declared a \"Potentate of the Rose\"!");
            break;
        }
    }

This will make it so you win after any 8 wins, not just 8 wins in a row. If I answer 4 questions right, then 20 wrong, then another 4 right, I still get it.

    while(true) {
        if(playOnce(getResults(5))) {
            streak++;
        }else{
            streak = 0;
        }  
        if(streak == 8) {
            System.out.println("You are now declared a \"Potentate of the Rose\"!");
            break;
        }
    }

This resets the streak counter if you don't have consecutive wins.

Answer 3

Nice code. I just have a few points:

• magic numbers: 5, 6 and 8 are magic numbers (as are all the number in getAnswer), I would store them in a field. It makes them easier to change, and the field name makes them easier to understand. getResult(5) is confusing, getResult(DICE_COUNT) or getResult(NUMBER_OF_DICE) is not.
• variable names: amt is not very clear, use amount or diceCount instead
• byte: computers have enough memory nowadays, use int instead. First of all, you are already using int in your code (also for very small values), so this would be consistent. Secondly, it is what most other programmers are more used to seeing, and thirdly - as Emanuele Paolini pointed out - it doesn't help memory usage, but actually might hurt performance
• method names: getResults is a bit unclear. Maybe use getDiceResults instead
• oop: your code is not object oriented at all, you might want to change that (by creating a Dice class, which can hold all the dice related code - rolling a dice, maybe printing a dice, etc - and a game class, which can hold the game logic - playing, checking answers, etc. You might also consider creating a class that retrieves user input. If you write a good interface for this class (and maybe one for the output), it would also be very easy to create a gui for the program later on.)
• Comments: you don't have any comments. Methods such as getAnswer and getResults are not really that self explanatory (especially when just reading the method name), so comments might help here.

Answer 4

The rest have offered a good number of improvements already, so I'll just focus on the use of an array of StringBuffers in getFormattedDice().

StringBuilder is recommended over StringBuffer if you do not need synchronization, as it is the case here. Usually, one creates a String from a StringBuilder using its toString() method, instead of new String(stringBuilder). Also, if you really need to use StringBuilder, then you should either append single characters (such as '|') or your individual Strings, instead of concatenating them first, to properly 'optimize' their usage:

stringBuilder.append('a').append("bc").append("def"); // marginally better use
stringBuilder.append("a").append("bc" + "def"); // poor use of StringBuilder

The poor use of StringBuilder is nothing better than just saying string + "abcdef", and I will then prefer the easier-to-read form.

I think the three-element StringBuffer array is unnecessary too, it will be more straightforward to just use them as, e.g. firstLine, secondLine, thirdLine.

Is EMPTY really just seven spaces? Why not call it as SPACE_7 then to standardize the names? I will go with something like NARROW_PADDING, NORMAL_PADDING and WIDE_PADDING though, or even create a simple enum to represent them. Since you are already referring to " " as SPACE, perhaps you can refer to "|" as SEPARATOR.

Finally, yeah, you should avoid using byte and stick with int here to reduce the amount of unnecessary casting.

Answer 5

Use int to represent an integer, not byte even if the values are very small. (see this question).