Critique Java Game: Help Me Improve
Hello
I am new to Java, so I gave myself a goal of making a Console Game in Java.
So I have finished it & I am looking for Java programmers to go over it pretty harshly & give me advice where I can improve.
I am looking for advice on:
- Java syntax - does my code follow correct java syntax
- Architecture - does my code adhere to Java OOP, should some functions be removed from the player class & placed in the Game logic class etc.
- Any "Big no no's" that I have done in my code that you can point out
- Overall - Is this good java code
The game I have made is a Game of Nim clone. If you dont know what Nim is, its supposably this ancient chinese strategy game, although I cant see why people have been playing this game for so long :P The aim is to be the person to grab the last match from a number of piles of matches.
Download: My Java Game Source Code
Edit: Ok I will post my code but its long...
GameLogic.java
Code :
/*
******************************************************************************************
Nim Game
Student:
SID:
******************************************************************************************
Game Logic Class:
Controls the actual flow of game play & performs game loop.
******************************************************************************************
*/
import java.util.*;
public class GameLogic
{
// Private Variables:
private enum GameStatus {ON, OFF};
private GameStatus gameState;
private boolean gameEnd;
Scanner in;
private int numOfPiles;
private Vector <Pile> piles;
private Vector <Player> players;
// Public Variables & Methods:
public GameLogic()
{
// Constructor:
gameState = GameStatus.ON;
gameEnd = false;
in = new Scanner( System.in );
numOfPiles = 0;
players = new Vector <Player>();
piles = new Vector <Pile>();
registerPlayers();
setPileSize();
}
public void initiateGameLoop()
{
// Post: Loop game until the game has finished & a winner has been announced
// By using a vector of Player objects & a for loop, the game loop is
// scalable & can manage many players.
while ( !gameEnd )
{
for (int i=0; i<players.size(); i++)
{
int pileNum, matchNum;
Player selPlayer = players.elementAt(i);
// Prompt player to decide what action to take
if ( selPlayer instanceof Human )
pileNum = ((Human) selPlayer).selectPile( in, piles.size() - 1 );
else pileNum = ((Computer) selPlayer).selectPile( piles.size() - 1 );
if ( pileNum < piles.size() )
{
// Remove n matches from a pile
if ( selPlayer instanceof Human )
matchNum = ((Human) selPlayer).selectMatch( in, piles.elementAt(pileNum).size() );
else matchNum = ((Computer) selPlayer).selectMatch( piles.elementAt(pileNum).size() );
piles.elementAt( pileNum ).remove( matchNum, selPlayer.alias );
selPlayer.storeLastMove( pileNum, matchNum );
}
else if ( pileNum == piles.size() )
{
// undo last move
int lastMove = selPlayer.getLastMove();
pileNum = (int) Math.floor(lastMove / 100);
matchNum = lastMove % 100;
piles.elementAt( pileNum ).replace( matchNum, selPlayer.alias );
selPlayer.storeLastMove( 0, 0 );
}
else if ( pileNum == piles.size() + 1 )
{
// redo last move
int lastMove = selPlayer.getLastMove();
pileNum = (int) Math.floor(lastMove / 100);
matchNum = lastMove % 100;
piles.elementAt( pileNum ).remove( matchNum, selPlayer.alias );
selPlayer.storeLastMove( 0, 0 );
}
else if ( pileNum == piles.size() + 2 )
{
// clear board & restart
selPlayer.storeLastMove( 0, 0 );
clear();
i = -1;
}
printGrid();
// Check if the game has finished
if ( assessGameState() )
{
// print out that selPlayer is the winner & how many matches they have
printGameStats( selPlayer );
clear();
i = -1;
}
}
}
}
public void registerPlayers()
{
// Post: Prompt user to play against AI or human. (Are there 2 players
// or one).
Player p1, p2;
int playerDecision = -1;
boolean validInput = false;
while ( !validInput )
{
try
{
System.out.printf("** No. of players ** \n1. Human VS AI \n2. Human VS Human \nEnter selection: ");
playerDecision = in.nextInt();
// Perform error checking
switch ( playerDecision )
{
case 1:
validInput = true;
p1 = new Human("White");
p2 = new Computer("Black");
players.add( p1 );
players.add( p2 );
break;
case 2:
validInput = true;
p1 = new Human("White");
p2 = new Human("Black");
players.add( p1 );
players.add( p2 );
break;
default:
System.out.println("Invalid input");
break;
}
}
catch (Exception io)
{
System.out.println("Invalid input");
in.nextLine(); // flush input
}
}
}
public void setPileSize()
{
// Post: Prompt user to select how many piles(heaps) of matches will
// be played with in the game
boolean validInput = false;
while ( !validInput )
{
try
{
System.out.printf("Please select the number of piles(heaps) you wish to have: ");
numOfPiles = in.nextInt();
// Perform error checking
if ( numOfPiles > 0 && numOfPiles <= 9 )
{
for (int i=0; i<numOfPiles; i++)
{
piles.add( new Pile() );
}
validInput = true;
}
else System.out.println("Invalid input");
}
catch (Exception io)
{
System.out.println("Invalid input");
in.nextLine(); // flush input
}
}
}
public int printGrid()
{
// Post: Output/display every heap(pile) in the nim game
Pile selPile;
System.out.println( "\n\n*** Nim Game Board ***" );
// 10 because there are 10 positions in each pile
for (int i=0; i<10; i++)
{
System.out.print(" ");
for (int j=0; j<piles.size(); j++)
{
selPile = piles.elementAt(j);
System.out.print( selPile.at(i).value + " " );
}
System.out.println();
}
System.out.println( "\n\n" );
return 1;
}
public void clear()
{
// Post: Clear game board & restack all piles with matches
for (int i=0; i<piles.size(); i++)
{
piles.elementAt( i ).clearHeap();
}
}
public boolean assessGameState()
{
// Post: Returns true if the game is finished & we have a winner
for (int i=0; i<piles.size(); i++)
{
if ( piles.elementAt(i).size() > 0 )
return false;
}
return true;
}
public void printGameStats( Player winner )
{
// Post: Display who is the winner & other player statistics
String winnerStats = " %s is the winner: Total matches collected = %s \t No. of wins = %s \t No. of losses = %s \n";
String loserStats = " Player Name: %s Total matches collected = %s \t No. of wins = %s \t No. of losses = %s \n";
System.out.printf( winnerStats, winner.playerName, winner.getMatches(),
winner.incrementWin(), winner.getLosses() );
System.out.println(" Other Player statistics: ");
for (int i=0; i<players.size(); i++)
{
Player selPlayer = players.elementAt(i);
if ( selPlayer.playerName != winner.playerName )
{
System.out.printf( loserStats, selPlayer.playerName, selPlayer.getMatches(),
selPlayer.getWins(), selPlayer.incrementLoss() );
}
}
System.out.println("\n");
}
}
Player.java
Code :
/*
******************************************************************************************
Nim Game
Student:
SID:
******************************************************************************************
Player Class:
Uses inheritance to define the player object(both human & AI).
******************************************************************************************
*/
import java.io.*;
import java.util.*;
public class Player
{
// Private Variables:
protected enum PlayerType { HUMAN, AI };
private int matchesOwned;
private int lastMove;
private int wins;
private int losses;
public String playerName;
public char alias;
// Public Variables & Methods:
public Player( String name )
{
// Constructor:
matchesOwned = 0;
lastMove = 0;
wins = 0;
losses = 0;
playerName = name;
alias = ( playerName.toUpperCase() ).charAt(0);
}
public String getName()
{
// Post: Return this Players name
return playerName;
}
public int incrementWin()
{
// Post: Increment wins & return its value
wins++;
return wins;
}
public int incrementLoss()
{
// Post: Increment wins & return its value
losses++;
return losses;
}
public void incrementMatches( int num )
{
// Post: Increment number of matches player owns
matchesOwned += num;
}
public int getWins()
{
// Post: Return the total no. of wins this player has
return wins;
}
public int getLosses()
{
// Post: Return the total no. of times this player has lost
return losses;
}
public int getMatches()
{
// Post: Return number of matches this player owns
return matchesOwned;
}
public void storeLastMove( int pileValue, int matchValue )
{
// Post: Store the most recent game move made by this player
lastMove = (pileValue * 100) + matchValue;
}
public int getLastMove()
{
// Post: Return the most recent game move player has made in encrypted form
return lastMove;
}
}
///////////////////////////////////////////////////////////////////////////////////////////////
// //
// Child class of Player: Human Class //
// //
///////////////////////////////////////////////////////////////////////////////////////////////
class Human extends Player
{
// Private Variables:
// Public Variables & Methods:
public Human( String name )
{
// Constructor:
super( name );
}
public int selectPile( Scanner in, int numPiles )
{
// Post: Prompt user to identify which pile they want to remove matches from
char playerDecision = 'z';
boolean validInput = false;
while ( !validInput )
{
try
{
String pileOptions = "";
for (int i=0; i<=numPiles; i++)
{
pileOptions += i;
pileOptions += ',';
}
System.out.printf("Which pile does %s select (%su,r,b): ", getName(), pileOptions);
playerDecision = in.next().charAt(0);
// Perform error checking
if ( Character.digit(playerDecision,10) >= 0 && Character.digit(playerDecision,10) <= numPiles )
{
validInput = true;
return Character.digit(playerDecision,10);
}
else if ( playerDecision == 'u' || playerDecision == 'U' )
{
if ( getLastMove() != 0 )
return numPiles + 1;
}
else if ( playerDecision == 'r' || playerDecision == 'R' )
{
if ( getLastMove() != 0 )
return numPiles + 2;
}
else if ( playerDecision == 'b' || playerDecision == 'B' )
{
return numPiles + 3;
}
System.out.println("Invalid input");
}
catch (Exception io)
{
System.out.println("Invalid input");
in.nextLine(); // flush input
}
}
return playerDecision;
}
public int selectMatch( Scanner in, int maxRemoval )
{
// Post: Prompt user to select x many matches to remove from a pile
int matchRemoveNum = 0;
boolean validInput = false;
while ( !validInput )
{
try
{
System.out.printf("How many matches do you wish to remove: ");
matchRemoveNum = in.nextInt();
if ( matchRemoveNum <= maxRemoval )
validInput = true;
else System.out.printf("You cant remove that many. Maximum is %s. \n", maxRemoval);
}
catch (Exception io)
{
System.out.println("Invalid input");
in.nextLine(); // flush input
}
}
incrementMatches( matchRemoveNum );
return matchRemoveNum;
}
}
///////////////////////////////////////////////////////////////////////////////////////////////
// //
// Child class of Player: Computer Class //
// //
///////////////////////////////////////////////////////////////////////////////////////////////
class Computer extends Player
{
// Private Variables:
// Public Variables & Methods:
public Computer( String name )
{
// Constructor:
super( name );
}
public int selectPile( int numPiles )
{
// Post: Get computer to randomly select a pile to remove matches from
return (int) (Math.random() * numPiles);
}
public int selectMatch( int maxRemoval )
{
// Post: Get computer to randomly select how many matches to remove
int matchRemoveNum = (int) (Math.random() * maxRemoval);
incrementMatches( matchRemoveNum );
return matchRemoveNum;
}
}
Pile.java
Code :
/*
******************************************************************************************
Nim Game
Student:
SID:
******************************************************************************************
Pile & Match Class:
...
******************************************************************************************
*/
import java.util.*;
public class Pile
{
// Private Variables:
private Vector <Match> myPile;
private int freeMatches;
public class Match
{
public char value;
public Match()
{
value = '*';
}
}
// Public Variables & Methods:
public Pile()
{
// Constructor:
myPile = new Vector <Match>();
freeMatches = 10;
for (int i=0; i<10; i++)
{
myPile.add( new Match() );
}
}
public int capacity()
{
// Post: Return the capacity of this pile
return myPile.size();
}
public int size()
{
// Post: Return the remaining matches in this pile
return freeMatches;
}
public Match at( int index )
{
// Post: Return the Match object at the specified index
if ( index >= 0 && index < myPile.size() )
{
return myPile.elementAt( index );
}
else return null;
}
public int remove( int nMatches, char alias )
{
// Post: Remove n Matches from the pile
for (int i = 0, index = freeMatches-1; i<nMatches; i++, index--)
{
myPile.elementAt( index ).value = alias;
freeMatches--;
}
return freeMatches;
}
public int replace( int nMatches, char alias )
{
// Post: Replace n Matches back into the pile
for (int i = 0, index = freeMatches; i<nMatches; i++, index++)
{
myPile.elementAt( index ).value = '*';
freeMatches++;
}
return freeMatches;
}
public void clearHeap()
{
// Post: Replace all matches in pile(heap)
while ( freeMatches != this.capacity() )
{
myPile.elementAt( freeMatches ).value = '*';
freeMatches++;
}
}
}
Re: Critique Java Game: Help Me Improve
hmm... from a first look-over, you may want to consider using Javadoc comments. This way you can automatically generate an API doc for someone to look at. It also allows certain IDE's (such as Eclipse or NetBeans) to provide documentation tooltips when you are coding.
Java doc looks like this:
Code Java:
/**
* Notice how the javadoc starts with two asterisks instead of 1 for normal block comments
*/
You can also add various annotations to make the javadoc more helpful for users.
Code Java:
/**
* This method adds two numbers
*
* @param num1
* The first number
* @param num2
* The second number
* @return the sum of num1 and num2
*/
public static int addem(int num1, num2)
{
return num1 + num1;
}
Javadoc also give you a place to layout a method/class contract, i.e. things that can be guaranteed when using that object (or, at least things that are suppose to be guaranteed), as well as any special formatting of results/input, or the behavior of the method/class in "strange" situations.
Code Java:
/**
* Adds two numbers. If either number is less than 0, returns -1.
*/
public static int addem(int num1, int num2)
{
if(num1 < 0 || num2 < 0)
{
return -1;
}
return num1 + num2;
}
That's the only major thing I can see right off the bat without any thorough analysis, I'm assuming the game runs and works correctly (if it doesn't, this would be a problem. I didn't actually try to compile or run it).
Re: Critique Java Game: Help Me Improve
Ditto on the subject of javadoc comments. They can be quite useful. Other more minor things (and I'll mention that I didn't try and compile the code either):
- You may wish to declare Person abstract, defining the methods used in both the child classes selectPile and selectMatch as abstract. This would allow you to remove the instanceof's and just call the method on Player (and is better design imo because any addition classes written that extend Player wouldn't necessitate changing the code of all the instanceof's)
- I saw a few public variables which you may wish to encapsulate and make private.
- Minor point, but you may wish to use an ArrayList rather than a Vector (Vector is synchronized and ArrayList not)
Re: Critique Java Game: Help Me Improve
You need a main method in class GameLogic for the program to work.
Re: Critique Java Game: Help Me Improve
The use of the hardcoded literals: 100 about line 97 aren't explained.
Would a final int be better then the name would self document
What is the logic about comparing: pileNum vs piles.size()
There are several comparisons made but not documented.
As mentioned above, the method selectPile() needs to be redesigned. The following looks awkard:
Code :
if ( selPlayer instanceof Human )
pileNum = ((Human) selPlayer).selectPile( in, piles.size() - 1 );
else pileNum = ((Computer) selPlayer).selectPile( piles.size() - 1 );
