Thread: 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 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

/*
   ******************************************************************************************
   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

/*
   ******************************************************************************************
   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

/*
   ******************************************************************************************
   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++;
		}
	}
 
}

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:

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

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

/**
 * 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).

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)

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:

				if ( selPlayer instanceof Human )
					 pileNum  = ((Human) selPlayer).selectPile( in, piles.size() - 1 );
 
				else pileNum  = ((Computer) selPlayer).selectPile( piles.size() - 1 );