Jump to content
  • Advertisement
Sign in to follow this  
gretty

Critique Beginner Console Java Code:

This topic is 2987 days old which is more than the 365 day threshold we allow for new replies. Please post a new topic.

If you intended to correct an error in the post then please contact us.

Recommended Posts

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

}


Share this post


Link to post
Share on other sites
Advertisement
Quote:
- Java syntax - does my code follow correct java syntax


Easiest way to find this out for yourself is to compile it. If the compiler doesn't say anything bad about, it follows correct Java syntax. If it throws up, then you know that it probably doesn't. [wink]

Share this post


Link to post
Share on other sites
No I mean things like:
- in java class names should begin with capitals
- in java functions names should be camel case
- in java x should be blah (I dont know any other java syntax rules)

Share this post


Link to post
Share on other sites
After a quick look I came up with these. This is not a definitive list, there might be more :)

1. Your inheritance is lacking a little. selectMatch and selectPile takes different amount of parameters for both computer and human so you have to use instanceof. Consider making those the same so you can get rid of those instanceof parts. Declare those as public methods of player and override in the sublasses. Scanner can be an attribute of the human so you don't have to pass it around in every method call.

2. Consider replacing Vector with some list implementation. List<Player> players = new ArrayList<Player>.

Share this post


Link to post
Share on other sites
Quote:
if (selPlayer instanceof Human)

instanceof should be considered a design flaw.

There are two methods that are called:
public int selectPile( Scanner in, int numPiles )
public int selectPile( int numPiles )
These are actually the same functionality, so they should be same virtual function in Player class.

public class Player {
public abstract int selectPile(int numPiles);
public abstract int selectMatch(int maxRemoval);
}



Scanner is only relevant for Human, so it should be part of that class:
public class Human extends Player {
private Scanner in;

public Human(String name, Scanner in) {
super(name);
this.in = in;
}

// these two can now use locally stored 'in'
public int selectPile(int numPiles) { ... }
public int selectMatch(int maxRemoval) { ... }

};


The calling code now becomes trivial:

pileNum = selPlayer.selectPile(piles.size() - 1);
if (pileNum < piles.size()) {
matchNum = selPlayer.selectMatch(piles.elementAt(pileNum).size());
piles.elementAt(pileNum).remove(matchNum, selPlayer.alias);
selPlayer.storeLastMove(pileNum, matchNum);
}


Pile class doesn't seem to really do anything beyond what List does. Might as well use that directly.


Quote:
        lastMove = (pileValue * 100) + matchValue;
Make a class out of this or something, something that holds the two values directly.


Prefer ArrayList over Vector.
When declaring data structures, use interfaces rather than actual classes (List instead of Vector or ArrayList, Map instead of HashMap, etc...).
Don't document this:
    // Public Variables & Methods:
// Constructor:
// Private Variables:
Child class of Player:
In Java, those are well defined.
For any kind of documentation (functions, variables, parameters, ...), JavaDoc should be used. Only rare comments should be inline.

Share this post


Link to post
Share on other sites
Thanks for the replies guys :D

I was having so much difficulty trying to figure out how to make SelectPiles & SelectMatches virtual that I thought Java didn't allow virtual functions lol...so thanks for clearing that up :)


Yeah, with storing the last move, coming from C++ I wanted to use my Structs but ofcourse I cant, so I will go and make a class out of it, I was just worried about overkill.

Ah & Javadoc comments, sigh, I'll go away & learn it, but coming from C++ it seems so stupid to have this way of commenting code, I prefer my C++ style commenting :(

Thanks again

Share this post


Link to post
Share on other sites
Quote:
Original post by gretty
No I mean things like:
- in java class names should begin with capitals
- in java functions names should be camel case
- in java x should be blah (I dont know any other java syntax rules)


Minor nitpick, but that would be style not syntax. In general, naming conventions like the ones you mentioned are not part of the syntax, except in some cases where the language enforces them. Java is not one of these languages, for the most part. If you're looking for a style guide, Sun's will probably give you something to work with. Much Java code is written with the above style guide in mind, in my experience, but really you can write your code using any convention you wish. It's a good idea to be consistent, however.

Share this post


Link to post
Share on other sites
I concur with what the others have said.

It is generally a good idea to seperate the game logic from the presentation. Your GameLogic and Player classes should be totally unaware that you are playing a console game.

You may need to invert some of the logic to allow this to happen. The game loop might need to be moved to the presentation class, and the player (or computer player) commands could be represented by methods on the GameLogic, or the player instances.

Other than that, I would mark fields that don't change as final.

What is the purpose of the GameState enum? Any binary enums, particularly ON/OFF, are a hint to consider using a boolean property (where it makes sense, an enum has also the null state which might be significant).

There are some naming issues, they are minor but might be worth mentioning. I would call "initiateGameLoop" "runGameLoop", because init*() functions generally have a specific meaning. "assessGameState()" is ambiguous, I would have a function called "isGameOver()" to make it clearer.

This line is a potential bug:

selPlayer.playerName != winner.playerName

You should be very careful when identity comparing objects in Java, typically any use of the identity comparison would need a comment to justify why you aren't using .equals() instead. In this case you should be safe as the "winner" is one of the members of the vector, but you should be comparing the player references directly rather than their names.

I have skipped the detail of the game here so there might be other things.

Share this post


Link to post
Share on other sites
Sign in to follow this  

  • Advertisement
×

Important Information

By using GameDev.net, you agree to our community Guidelines, Terms of Use, and Privacy Policy.

We are the game development community.

Whether you are an indie, hobbyist, AAA developer, or just trying to learn, GameDev.net is the place for you to learn, share, and connect with the games industry. Learn more About Us or sign up!

Sign me up!