patternjavaMinor
The Game of Life that scored 100%
Viewed 0 times
thescored100gamethatlife
Problem
Note: this is from a COMPLETED course. This is not a ploy to get help with homework, as I have already received a 100% on this assignment (last year). I just want to become a neater programmer. Can you please provide me with some stylistic feedback? Note, the assignment did not allow for the use of objects at the time and the dat files contained X's and .'s. X's = life, .'s = absence of life. It was an exercise to provide us practice with multi-dimensional arrays.
```
package java_labs.GameOfLife;
import java.util.Scanner;
import java.io.*;
public class GameOfLife {
final static int M = 25;
final static int N = 75;
public static void main(String[] args) {
char[][] oldBoard = new char[M + 2][N + 2];
char[][] newBoard = new char[M + 2][N + 2];
char[][] tempBoard = new char[M + 2][N + 2];
buildBoardFromFile(oldBoard, newBoard, tempBoard);
int generationCount = 0;
printBoard(oldBoard, generationCount); / prints Generation #0 every time/
boolean continueGame = true;
while (continueGame) {
if (isEmpty(oldBoard)) {
continueGame = false;
} else {
newBoard = getNextGeneration(oldBoard, newBoard);
if (isGenerationChanged(oldBoard, newBoard)) {
tempBoard = copyArray(newBoard, tempBoard);
Scanner input = new Scanner(System.in);
String inputValue = " ";
while ((!(inputValue.equalsIgnoreCase("Y")))
&& (!(inputValue.equalsIgnoreCase("Q")))) {
System.out.print("\nWould you like to see the next generation?\n"
+ "Enter 'Y' for yes or 'Q' to quit: ");
inputValue = input.next();
if (inputValue.equalsIgnoreCase("Q")) {
continueGame = false;
System.out.prin
```
package java_labs.GameOfLife;
import java.util.Scanner;
import java.io.*;
public class GameOfLife {
final static int M = 25;
final static int N = 75;
public static void main(String[] args) {
char[][] oldBoard = new char[M + 2][N + 2];
char[][] newBoard = new char[M + 2][N + 2];
char[][] tempBoard = new char[M + 2][N + 2];
buildBoardFromFile(oldBoard, newBoard, tempBoard);
int generationCount = 0;
printBoard(oldBoard, generationCount); / prints Generation #0 every time/
boolean continueGame = true;
while (continueGame) {
if (isEmpty(oldBoard)) {
continueGame = false;
} else {
newBoard = getNextGeneration(oldBoard, newBoard);
if (isGenerationChanged(oldBoard, newBoard)) {
tempBoard = copyArray(newBoard, tempBoard);
Scanner input = new Scanner(System.in);
String inputValue = " ";
while ((!(inputValue.equalsIgnoreCase("Y")))
&& (!(inputValue.equalsIgnoreCase("Q")))) {
System.out.print("\nWould you like to see the next generation?\n"
+ "Enter 'Y' for yes or 'Q' to quit: ");
inputValue = input.next();
if (inputValue.equalsIgnoreCase("Q")) {
continueGame = false;
System.out.prin
Solution
It's great that you strive to improve your skills, even though you already got a good score.
OOP
An OOP language like Java makes it easy to encapsulate closely related data and operations in ADTs (abstract data types). An exercise like this screams for an ADT called Board.
Methods with the word "board" in the name will naturally fit into the ADT, and instead of passing the
Single responsibility principle
A method should have one responsibility, one thing to do and do it well. Most of your method do multiple things. They can be split up to smaller methods. The result will be multiple shorter methods that are easier to understand and easier to understand.
Reading from standard input
The program reads from standard input in two places, and each time it creates a new
Avoid pointless flag variables
The flag variable
Instead of setting this variable to false, it would be simpler to just break out of the loop.
Not doing so makes the program harder to read. Here, for example, if I want to verify what else will happen until the end of this cycle, I have to read the entire loop body. By breaking out right here right now, I will know that we're definitely out of the loop, the mental burden is greatly reduced.
Another good example is in the other branch in this loop that sets the flag variable:
So the print statement tells me that we're exciting the loop. But on that line, far away from the statement where you set the flag, and far away from the statement of the loop, it's not obvious that we really will exit. That's an unnecessary mental burden. And making it obvious is easy:
And as we never change the flag variable anymore, the loop condition can be simplified to
Flag variables also often lead to overlooking performance issues, for example here:
Once the flag is set to true, it will never change, but the code continues to explore the entire board, instead of returning immediately.
Magic values
Some literal values appear at multiple places here and there, for example
OOP
An OOP language like Java makes it easy to encapsulate closely related data and operations in ADTs (abstract data types). An exercise like this screams for an ADT called Board.
Methods with the word "board" in the name will naturally fit into the ADT, and instead of passing the
char[][] parameters around everywhere, it could be hidden neatly inside the board, applying the good principles of encapsulation and information hiding. (Recommended reading: Code Complete chapter 6)Single responsibility principle
A method should have one responsibility, one thing to do and do it well. Most of your method do multiple things. They can be split up to smaller methods. The result will be multiple shorter methods that are easier to understand and easier to understand.
Reading from standard input
The program reads from standard input in two places, and each time it creates a new
Scanner instance. It would be better to create one scanner instance in the main method, and pass that to the methods that need it.Avoid pointless flag variables
The flag variable
continueGane here is unnecessary (like flag variables often are):boolean continueGame = true;
while (continueGame) {
if (isEmpty(oldBoard)) {
continueGame = false;
}
// ...Instead of setting this variable to false, it would be simpler to just break out of the loop.
Not doing so makes the program harder to read. Here, for example, if I want to verify what else will happen until the end of this cycle, I have to read the entire loop body. By breaking out right here right now, I will know that we're definitely out of the loop, the mental burden is greatly reduced.
Another good example is in the other branch in this loop that sets the flag variable:
} else {
continueGame = false;
// ... many many lines of code
System.out.println("\n\n\nYou will now exit the program!\n\n\n");
}So the print statement tells me that we're exciting the loop. But on that line, far away from the statement where you set the flag, and far away from the statement of the loop, it's not obvious that we really will exit. That's an unnecessary mental burden. And making it obvious is easy:
System.out.println("\n\n\nYou will now exit the program!\n\n\n");
break;And as we never change the flag variable anymore, the loop condition can be simplified to
true.Flag variables also often lead to overlooking performance issues, for example here:
static boolean isGenerationChanged(char[][] oldBoard, char[][] newBoard) {
boolean continueGame = false;
for (int row = 1; row <= M; row++) {
for (int col = 1; col <= N; col++) {
if (oldBoard[row][col] != newBoard[row][col])
continueGame = true;
}
}
return continueGame;
}Once the flag is set to true, it will never change, but the code continues to explore the entire board, instead of returning immediately.
Magic values
Some literal values appear at multiple places here and there, for example
'.' and 'X'. It would be better to put these in constants with descriptive names.Code Snippets
boolean continueGame = true;
while (continueGame) {
if (isEmpty(oldBoard)) {
continueGame = false;
}
// ...} else {
continueGame = false;
// ... many many lines of code
System.out.println("\n\n\nYou will now exit the program!\n\n\n");
}System.out.println("\n\n\nYou will now exit the program!\n\n\n");
break;static boolean isGenerationChanged(char[][] oldBoard, char[][] newBoard) {
boolean continueGame = false;
for (int row = 1; row <= M; row++) {
for (int col = 1; col <= N; col++) {
if (oldBoard[row][col] != newBoard[row][col])
continueGame = true;
}
}
return continueGame;
}Context
StackExchange Code Review Q#123117, answer score: 4
Revisions (0)
No revisions yet.