patternjavaModerate
Petals Around the Rose
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
```
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
I think you should also create some more functions. Looking at your
Why do you use
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
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.
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.