HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaModerate

Petals Around the Rose

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
aroundpetalstherose

Problem

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

Solution

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 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.

Context

StackExchange Code Review Q#59310, answer score: 10

Revisions (0)

No revisions yet.