1
\$\begingroup\$

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++;
    }
}
}
\$\endgroup\$
  • 1
    \$\begingroup\$ You could start making the code less ugly by fixing the indentation in the method Blobfish.main(String[]) and in the class Dice. \$\endgroup\$ – Stingy Apr 4 '18 at 20:26
  • 4
    \$\begingroup\$ Is this how your code is really indented? Note that the easiest way to post code here is to paste it into the question editor, highlight it, and press Ctrl-K to mark it as a code block. \$\endgroup\$ – 200_success Apr 4 '18 at 21:10
  • \$\begingroup\$ Are you sure this code compiles ? i made a quick test and it prompted some errors. Fix them first. \$\endgroup\$ – Isac Apr 4 '18 at 23:57
  • 1
    \$\begingroup\$ Since you say the indent/formatting is from having problems pasting here and not with how you normally format the code I suggest you edit the question again, delete all the code and re-paste it one class at a time. When pasting a new code block, select it all and press the {} button at the top of the question editor (or type ctrl+K as 200_success suggested). To separate the code blocks for each class, add a line above each one with ** before and after a word (I suggest the classname). That way, you get a normal text with the classname in bold, followed by the class implementation in code. \$\endgroup\$ – Imus Apr 5 '18 at 13:28
  • 2
    \$\begingroup\$ This is getting worse with every edit. Now you simply added the apparently up-to-date code to the old code instead of replacing the old code with the new code. And the indentation is still off. I wonder what IDE you are using, because at least NetBeans and IntelliJ have a feature where you can automatically add or remove one level of indentation to a whole selection of code, so you can simply select all your code, add one level of indentation (if one level is set to be 4 spaces, which seems to be a common default value), copy it and paste it here, and remove the indentation from your […] \$\endgroup\$ – Stingy Apr 5 '18 at 15:21
2
\$\begingroup\$

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 MonopolyTM

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.

\$\endgroup\$
0
\$\begingroup\$
  • 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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.