I am a Student who has been set the task of coding a Monopoly-style text-based game in java as a group project for our Easter Holidays.

I have finished, at least mostly, my two classes and just have a few functions to add in before we start to combine all our classes to make the final product. However, my Code is very messy, ugly, and inefficient; I was wondering if I could have some suggestions as to how to tidy it up a bit.

public class Blobfish 

public static void main(String args) throws InterruptedException 
 Board board = new Board(0);
 do
 for(board.x=0;board.x<4;board.x++)
 Dice dice = new Dice();
 dice.roll1();
 dice.roll2();

 System.out.println("The Players Turn = Player " +board.Players[board.x]);
 System.out.println("First Roll: " +dice.getDots1());
 System.out.println("Second Roll: " +dice.getDots2());
 // System.out.println("" + Dice.getSame());
 System.out.println("" +board.getPos());
 System.out.println("" +board.getMoney());
 System.out.println("n");
 if(board.mFlag!=0)
 System.out.println("nWe will not calculate the winner!");
 // System.out.println("The Winner is: " +Board.getWinner());
 break;
 
 Thread.sleep(1000);
 
while(board.Money>(board.pPos[board.i]));




public class Board 

 public static int i=0;
 public static int turn=0;
 public int Turn = 0;
 public int totalP = 0;
 public static int Pos = 0;
 public static int Players= 1, 2, 3, 4;
 public static int Square = 0...25;
 public static int Money = 400;
 public static int pPos =0,0,0,0;
 public static int pMoney = 0,0,0,0;
 public static int x=0;
 public static int mFlag = 0;

public Board(int totalP)
 this.totalP = totalP;


public static String getPos()
 int roll1 = Dice.roll1;
 int roll2 = Dice.roll2;
 pPos[i] = (roll1+roll2);
 return("Your Current Position: " +(pPos[i]));


public static String getMoney()
 pMoney[i] = (pPos[i] * 10);
 Money = Money - pMoney[i];
 if(Money < 0)
 mFlag++;
 return ("The bank does not have enough money left!");
 
 else if(Money==0)
 return ("You have run the bank Dry!");
 
 return ("Money left in bank: " +Money);





class Dice

static int dots,roll1,roll2,flag;
Random number = new Random();

public Dice()
 dots = number.nextInt(6)+1 ;


public void roll1()
 roll1 = number.nextInt(dots)+1;

 public void roll2()
 roll2 = number.nextInt(dots)+1;
 

public static int getDots1()
 return roll1;


public static int getDots2()
 return roll2;

public static void getSame()
 while(roll1 == roll2)
 flag++;
 



public class Board 

public int i; // i is number of runs in total
public int turn=0;
public int Turn = 0;
public int totalP = 0;
public int Pos = 0;
public int Players= 1, 2, 3, 4;
public int Square = 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25;
public int Money = 400;
public int pPos =0,0,0,0;
public int pMoney = 0,0,0,0;
public int x; // x is up to 4 
public int mFlag;
public int p; 

public Board(int Players)
 this.x=0;this.mFlag=0;this.i = 0;this.p=0;


public String getPos()
 int roll1 = Dice.roll1;
 int roll2 = Dice.roll2;
 pPos[i] = (roll1+roll2);
 return("Your Current Position: " +(pPos[i]));

public String getMoney()
 pMoney[i] = (pPos[i] * 10);
 Money = Money - pMoney[i];
 if(Money < 0)
 mFlag++;
 return ("The bank does not have enough money left!");
 
 else if(Money==0)
 return ("You have run the bank Dry!");
 
 return ("Money left in bank: " +Money);


public String getTally()
 return ("Your Money = " +pMoney[x]); // this is not yet implemented




import java.util.Random;

class Dice

static int dots,roll1,roll2,flag;
Random number = new Random();

public Dice()
 dots = number.nextInt(6)+1 ;


public void roll1()
 roll1 = number.nextInt(dots)+1;

 public void roll2()
 roll2 = number.nextInt(dots)+1;
 

public int getDots1()
 return roll1;


public int getDots2()
 return roll2;

public void getSame()
 while(roll1 == roll2)
 flag++;
 

Welcome to Code Review, thanks for sharing your code!

General issues

Naming

Finding good names is the hardest part in programming. So always take your time to think carefully of your identifier names.

Naming Conventions

Please read and follow the
Java Naming Conventions.

In your code you have some variable names that start with an upper case letter (e.g.: Players) but they should start with lower case letter. Only class names start with upper case letter.

Avoid abbreviations

In your code you use some abbreviations such as totalP and pPos.
Although this abbreviation makes sense to you (now) anyone reading your code being not familiar with the problem (like me) has a hard time finding out what this means.

If you do this to save typing work: remember that you way more often read your code than actually typing something. Also for Java you have good IDE support with code completion so that you most likely type a long identifier only once and later on select it from the IDEs code completion proposals.

Don't surprise your readers

You claim (even in your title) that this is a monopoly like game but from what I see this in more a combination of a simple dice game and black jack. Especially there is no money transfer between the players directly based on type of the fields and their ownership, which is the "main feature" of monopoly.

Maybe this is an (very) early version but I cannot see how that could evem possibly develop towards Monopoly^TM

procedural approach / primitive obsession

Your code works with a lot of arrays of primitive types.
This leads to rather cryptic code which is hart to maintain and hard to extend.

Also you distribute information that somehow belong together to random places throughout your program.

avoid Static access

All over your program you access static variables in other classes. This is very bas since it effectively inhibits polymorphism which is the main feature of an object oriented language like Java. Also it violates the information hiding / encapsulation principle. It leads to higly coupled, hardly maintainable code.

• 
  

  Dice

  
  
  

  You are "initializing" a static variable in a constructor. Do you realize the absurdity of this? Because if not, you have not understood the purpose of classes and objects. A class can be seen as a kind of blueprint, like a general definition, so a class is something abstract. An object, on the other hand, is something concrete, because it is a specific occurrence, or "instance", of a class. For example, "Movie" could be a class, and "Pulp Fiction", "The Brain" and "Troll 2" could be objects of type "Movie". Or "OperatingSystem" could be a class, and "Linux" and "Windows" would be instances of "OperatingSystem". I strongly suggest you read the chapters on classes and objects in the Java Tutorials.

  
  
  

  So how can this be a applied to the representation of a die, or a pair of dice? In object-oriented-programming, an object is defined through state and behavior, where state is represented by fields and behavior is represented by methods. A die can have any number of properties that can be represented in code, like weight, size, or what's written on its faces, but all of this is not relevant for your program, you are only interested in one specific behavior, which is that, when rolled, it will randomly yield a number between 1 and 6. So this means the class Die only needs one method, which is roll(). All of the other things that you have put into your class Dice, like the two separate methods for two rolls, or the fact that you store the results of a roll, or that you compare the results of the two rolls, are not the responsibility of a die itself, but of whoever uses the die, so they do not belong into the class Die. The only "state" I can think of that would be relevant to the Die class is the random number generator, because if two instances of Die have two random number generators with identical seeds, they will roll the same sequence of numbers, which is not desirable. So a Die can also have a field that stores a random number generator with a seed that is, for all intents and purposes, unique:

  
  
  

  public class Die 
   private final Random randomGenerator;
  
   public Die() 
   randomGenerator = new Random();
   
  
   public int roll() 
   return randomGenerator.nextInt(6) + 1;
   
  
  

  
  
  

  Note that the roll() method does not store the result of the roll anywhere, but simply returns it. The die does not care what happens with the result of the roll, so storing the result is the responsibility of the caller, not of the die. Also note that randomGenerator is final, so a Die is now immutable, and immutable objects make life easier.

  
  


• 
  

  Board

  
  
  

  Next, let's start to clean up the mess in the class Board a litte bit. You have three distinct variables, Players, pPos and pMoney, that are related to each other. This makes the code very fragile, because if one variable is modified, the others become invalid. The code would be more robust if the relationship between the data these variables store were also implicitly reflected in the code design.

  
  
  

  So what is the relationship between the data represented by these variables in the first place? Money is a property of a player, so you could make a Player class that defines a field money:

  
  
  

  public class Player 
   private int money;
  
   public Player() 
   money = 0;
   
  
   public int getMoney() 
   return money;
   
  
   public void addMoney(int money) 
   if (money < 0) 
   throw new IllegalArgumentException("Attempted robbery.");
   
   this.money += money;
   
  
  

  
  
  

  Note that the field money is private and can only be accessed via public methods. That way, you have control how exactly the field can be modified, which you would not have were the field money public.

  
  
  

  The position on the board, on the other hand, is not a property of a player, but a value that's associated with a player by the board, so the position on the board should be stored somewhere in Board. A way to do this would be to create a Map<Player, Integer>, which would then be a field of Board. The most commonly used implementation of Map is HashMap, however, a HashMap does not have a predictable iteration order, which might be unsuitable for your program, since the game rules require a fixed order in which the players take their turns. An alternative would be a LinkedHashMap instead of a HashMap, which returns its elements in the order they were inserted.

  
  
  

  So the question is whether it is the board's responsibility to know the order in which the players have to take their turns. Originally, I would have said "yes", but after thinking about it for a while, I think that it would be better to move this responsibility outside Board, so that a Board is only responsible for mapping the players to their positions and advancing them, and to leave it to some code outside Board to determine whose player's turn it is.

  
  
  

  So this could be a start for the class Board, using a HashMap after all:

  
  
  

  public class Board 
  
   private Map<Player, Integer> playerPositions;
  
   public Board() 
   playerPositions = new HashMap<>();
   
  
   public void addPlayer(Player player) 
   Objects.requireNonNull(player);
   if (playerPositions.containsKey(player)) 
   throw new IllegalStateException("Player has already joined the game.");
   else 
   playerPositions.put(player, 0);
   
   
  
   public int getPosition(Player player) 
   Integer position = playerPositions.get(player);
   if (position == null) 
   throw new IllegalArgumentException("Player does not play in the game.");
   else 
   return position; //auto-unboxing to int would throw a NullPointerException if position == null
   
   
  
  

  
  
  

  Note that Board itself doesn't create any players, it merely "registers" them, so to speak. The creation of Player instances is not the responsibility of the board (at least, I would see it that way, this might be slightly subjective).

  
  
  

  Now, the number of players, their money and their positions on the board are tightly coupled together, and you don't need to store separate arrays whose indexes just happen to be related to each other. This should also rid you of the necessity for variables like i and x, although I'm not entirely sure what you are trying to do with them, because i is only ever written to once, yet you use it as if it stores the index of the current player.

  
  
  

  The next thing to rework is the method getPos(). Contrary to its name, this method not only "gets" the current player's position, but it also modifies it. In addition, it also rolls the dice and determines which player to advance. These are all things outside Board's responsibility (at least, I think it would make things easier if determining whose player's turn it is is not the responsibility of Board, as I already said earlier). So what to do? Make the method accept two arguments, one for the player, and one for the number of fields the player should advance. The board does not care that the number of fields to advance was determined by rolling two dice:

  
  
  

  public void advance(Player player, int numberOfFields) 
   if (!playerPositions.containsKey(player)) 
   throw new IllegalArgumentException("Player does not play in the game.");
   else if (numberOfFields <= 0) 
   throw new IllegalArgumentException("numberOfFields must be positive: " + numberOfFields);
   else 
   playerPositions.merge(player, numberOfFields, Integer::sum);
   player.addMoney(numberOfFields * 10);
   this.Money -= numberOfFields * 10;
   
  
  

  
  
  

  Note that, if a player's money is determined exclusively by the player's position on the board (which, in your code, it is, but I don't know the whole setup of the game), then the player's money is, in fact, not a property of the player, but a value than can be computed solely from the player's position on the board, which means that it shouldn't be a field in Player. And without the field money, a player would have no state at all, which means that the Player class would be redundant and the players' positions can, in fact, be stored in a simple int (or a List<Integer>) in Board, like in your code. But still, the variables Players and pMoney would be redundant, because the number of players would be implicitly stored in the size of the array (or List) that contains the player positions, and the players' money would not be a state per se, but a value that depends on another state, i.e. the players' positions.

  
  
  

  So if a player's money really only depends on the player's position on the board, then you can store the positions of the players either in an int or in a List<Integer>. A List has the advantage that it is resizable, so players can be added (or removed) later, so this decision depends on your intention. Here's an example with a List<Integer>:

  
  
  

  private List<Integer> playerPositions;
  
  public Board() 
   playerPositions = new ArrayList<>();
  
  
  public void addPlayers(int numberOfPlayers) 
   for (int i = 0; i < numberOfPlayers; i++) 
   playerPositions.add(0);
   
  
  
  public int getPosition(int playerIndex) 
   return playerPositions.get(playerIndex);
  
  

  
  
  

  Now the most straightforward way to get a player's money would be:

  
  
  

  public int getMoney(int playerIndex) 
   return playerPositions.get(playerIndex) * 10;
  
  

  
  
  

  Likewise, the money in the bank would not be a field of its own, but calculated based on the players' accumulated money:

  
  
  

  public int getMoneyInBank() 
   int money = 400;
   for (int playerIndex = 0; playerIndex < playerPositions.size() && money > 0; playerIndex++) 
   money -= getMoney(playerIndex);
   
   return Math.max(money, 0);
  
  

  
  
  

  But again, this only applies if the players' and the bank's money only depend on the position of the players. Note also that these methods return an int instead of a String. A String is only relevant for the output format/user interaction, and separating user interface from program logic goes a long way towards making code easily maintain- and readable.

  
  


• 
  

  Finally, a remark about this line (even though the variable is not used):

  
  
  

  public int Square = 0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25;
  

  
  
  

  I assume the reason you wrote out the 26 (not a square number, is this a bug?) numbers and did not use a loop is that this was originally a static variable and that you were under the impression that, in order to initialize the variable as soon as the class is loaded, you have to initialize it immediately with its declaration, i.e. in one line. But this is not true: There is a language construct called a static initialization block (I'm going to rename the variable to square, starting with a lowercase letter, because it adheres to Java naming conventions):

  
  
  

  public class Board 
  
   public static int square;
  
   static 
   square = new int[26];
   for (int i = 0; i < 26; i++) 
   square[i] = i;
   
   
  
  

  
  
  

  This would be especially relevant if square were static and final, because then you would have to initialize it as soon as the class is loaded.

  
  

This does not address all the issues with your code, like the countless unused variables that might be relics from earlier attempts, but it might give you an idea on what you can improve with your code. I would advise you to return to get another review if you have reworked your code (simply start a new question and create mutual links in the old and new question), since, as you say, the code is really very messy.