6
\$\begingroup\$

I'm still new to Java and learning programming, so my main goal is to learn to making good code, so I don't want to do anything complex until I fix all of my bad coding habits on smaller programs.

So, this is my third version of this program, you can find last one here and here is where I started. I wanted to expand the game with few extra features. First one is that it now has interface, also, player can get points when he plays and base on how much points he has, he can play different levels of the game.

Also, username and points are saved to text file, first I wanted to use same text file for that but updating points in that looks quite complicated so I thought this is better way since I only have this to informations I want to store, so maybe I'll do that in next version if it's better or maybe there is better way of storing/updating points?

I'm not sure if I'm using communication between two classes (player and game) as I should, it's something I should look more into.

Main.java

import java.io.IOException;

public class Main {

    public static void main(String[] args) throws IOException {

        Game game = new Game();
        Player player = new Player();
        game.options(player);

    }
}  

Game.java

import java.io.IOException;
import java.util.concurrent.ThreadLocalRandom;
import javax.swing.JOptionPane;

public class Game {
    private int min=1;
    private int max;
    private int numberOfMines;
    private int randomNumber;
    int mine[] = new int[20];
    private int userGuess;
    int option = 0;
    public int promptForInteger(String messageText) {
        String askBoxes = JOptionPane.showInputDialog(messageText);
        int number = 0;
        if(askBoxes == null) {
            System.exit(0);
        } else {
                try { 
                   number = Integer.parseInt(askBoxes);

                } catch(NumberFormatException e) { 
                  JOptionPane.showMessageDialog(null, "Value must be number!");
                }
        }
        return number;
    }    

    public void options(Player player) throws IOException{

        do{
            option = promptForInteger("Hi " + player.name + "! \n"
                    + "Please select your game:\n1) Guess number between 1 and 10 with 1 mine.\n"
                    + "2) Guess number between 1 and 20 with 3 mines.  (You need 20 points to play this)\n"
                    + "3) Guess number between 1 and 100 with 20 mines.  (You need 100 points to play this)\n"
                    + "4) Exit the game (no points needed for this :) )"
                    + "\n \n \n you currently have: " + player.points + " points \n \n");

        // option = promptForInteger();

        }while (option < 1 || option > 4);

        switch (option) { // set max depending on user selection
            case 1: max = 10;
                    numberOfMines = 0; // one mine
                    break;
            case 2: if (player.points < 10){
                        JOptionPane.showMessageDialog(null, "You don't have enough points to play this game");
                        options(player);
                        return;
                    }
                    max = 20;
                    numberOfMines = 2; // 3 mines
                    break;
            case 3: if (player.points < 100){
                        JOptionPane.showMessageDialog(null, "You don't have enough points to play this game");
                        options(player);
                        return;
                    }
                    max = 100;
                    numberOfMines = 19; // 20 mines
                    break;  
            case 4: System.exit(0);
        }        
        createNumberAndMines();
        playGame(player);
    }

    public void createNumberAndMines(){ 
         randomNumber = getRandom(); // get random number to guess
         askForGuess(); // first user guess can't be mine, so I want to ask for first guess then set mines


         for (int i = numberOfMines; i >= 0; i--){

             do{ // we don't want to have same number for guessing and mine or first user guess to be a mine
                 mine[i] = getRandom();
                 for (int j = i+1; j<19; j++){ //check if generated number is duplicate mine
                     if (mine[i] == mine[j]){
                         i++; // if we find that this number is duplicated to previous one we want to do for loop again for this mine[i] 
                     }
                 }

             }while (mine[i] == randomNumber || mine[i] == userGuess);
         }
    }


    public int getRandom() {
        return ThreadLocalRandom.current().nextInt(min, max + 1);
    }

    public void playGame(Player player) throws IOException {
        for (int numGuesses = 0; true; ){
            if (userGuess > 0) numGuesses++; // if user inputs something other then number we don't want to increase numGuesses

            for (int i=numberOfMines; i>=0; i--){// check if user hits mine
                if (mine[i] == userGuess){
                    JOptionPane.showMessageDialog(null, "You lose! You have hit the MINE and lost 1 point",
                            null, JOptionPane.ERROR_MESSAGE);
                    player.updatePoints(-1);
                    return;
                }
            }          

            // tell user how close he is to right answer
                if (userGuess < randomNumber && userGuess != 0){ 
                   JOptionPane.showMessageDialog(null, "Too low!");
               }else if(userGuess > randomNumber){
                   JOptionPane.showMessageDialog(null, "Too high!");
               }else if(userGuess == randomNumber){
                   JOptionPane.showMessageDialog(null, "You win! You tried " + numGuesses + " time(s) total");
                   player.updatePoints(numGuesses,option);
                   options(player);

               }
               askForGuess();
        }
    }
    public void askForGuess(){
        // userGuess = Integer.parseInt(JOptionPane.showInputDialog("You are guessing random number between 1 and " + this.max + ". Enter your guess: "));
        userGuess = promptForInteger("You are guessing random number between 1 and " + this.max + ". Enter your guess: ");
     }
}

Player.java

import java.io.BufferedReader;
import java.io.File;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.io.FileWriter;
import java.io.IOException;
import java.io.PrintWriter;

import javax.swing.JOptionPane;

public class Player {

    protected String name;
    protected int points=0;
    protected String txtPathPlayer = "src/Player.txt";
    protected String txtPathPoints = "src/Points.txt";


    public Player() throws IOException{ // constructor, I want to run this every time someone creates new player
            getName(); // load username from Player.txt if it doesn't exists ask for username
            loadPoints();
    }


    public void getName() throws IOException{
        FileReader in;
        boolean nameFound = false;
       if (!fileExists(txtPathPoints)){
          askName();
       }

        try {
            in = new FileReader(txtPathPlayer);
            BufferedReader br = new BufferedReader(in);
            String line;

            while ((line = br.readLine()) != null) { // search if there is saved username in Player.txt and store it in "name"
                if (line.contains("username: ")){
                    name = line.substring(10);
                    nameFound = true;
                }
            }
            if (!nameFound){ //if name is not stored in text file ask for name
                askName();
            }
            br.close();
        } catch (FileNotFoundException e) {
            e.printStackTrace();
        }

    }
    public void askName() throws IOException{
            try {
                FileWriter writer = new FileWriter(txtPathPlayer, true);
                name = JOptionPane.showInputDialog("Please enter your name: ");
                writer.write("username: " + name);
                writer.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
    }
    public boolean fileExists(String Path){
        File f = new File(Path);
        if (!(f.exists() && !f.isDirectory())) {  
           return false;
        }else {
           return true;
        }
    }
   public void loadPoints() throws IOException{
       FileReader in;
       boolean pointsEmpty = true;
       if (!fileExists(txtPathPoints)){
           updatePoints(0);
       }

       try { // look if there is points.txt file and load points to variable points
            in = new FileReader(txtPathPoints);
            BufferedReader br = new BufferedReader(in);

            String line;

            while ((line = br.readLine()) != null) {
                if (line.contains("points: ")){
                    points = Integer.parseInt(line.substring(8));
                    pointsEmpty = false;
                }
            }
            br.close();

        } catch (FileNotFoundException e) {
            e.printStackTrace();
        }
       if (pointsEmpty){ // if point.txt does exist but has invalid data update points to 0
           updatePoints(0);
       }
   }
    public void updatePoints(int points) throws FileNotFoundException{
        if (points < 0 && this.points < 0){
            JOptionPane.showMessageDialog(null, "You don't have any points so you lose anything! :) ");
        }else {
            this.points += points;
            PrintWriter pw = new PrintWriter(txtPathPoints);
            pw.print("points: " + this.points);
            pw.close();
        }
    }

    public static boolean isBetween(int x, int lower, int upper) {
          return lower <= x && x <= upper;
        }

    public void updatePoints(int numGuesses, int option) throws FileNotFoundException{
        if (option == 1){ 
             switch (numGuesses) {
            case 1: 
                updatePoints(7);
                JOptionPane.showMessageDialog(null, "You won 7 points!");
                break;
            case 2: 
                updatePoints(5);
                JOptionPane.showMessageDialog(null, "You won 5 points!");
                break;
            case 3: 
                updatePoints(3);
                JOptionPane.showMessageDialog(null, "You won 3 points!");
                break;
            default:
                updatePoints(1);
                JOptionPane.showMessageDialog(null, "You won 1 point!");
             }

        }
        if (option == 2){ 
            if (isBetween(numGuesses, 1, 3)) {
                updatePoints(7);
                JOptionPane.showMessageDialog(null, "You won 7 points!");
            }else if (isBetween(numGuesses, 4, 6)) {
                updatePoints(5);
                JOptionPane.showMessageDialog(null, "You won 5 points!");           
            }else if (isBetween(numGuesses, 7, 9)) {
                updatePoints(3);
                JOptionPane.showMessageDialog(null, "You won 3 points!");           
            }else{
                updatePoints(1);
                JOptionPane.showMessageDialog(null, "You won 1 point!");
            }
        }
        if (option == 3){ 
            if (isBetween(numGuesses, 1, 10)) {
                updatePoints(7);
                JOptionPane.showMessageDialog(null, "You won 7 points!");
            }else if (isBetween(numGuesses, 11, 20)) {
                updatePoints(5);
                JOptionPane.showMessageDialog(null, "You won 5 points!");           
            }else if (isBetween(numGuesses, 21, 30)) {
                updatePoints(3);
                JOptionPane.showMessageDialog(null, "You won 3 points!");           
            }else{
                updatePoints(1);
                JOptionPane.showMessageDialog(null, "You won 1 point!");
            }
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

First I just want to say I don't have a lot of time at the moment to write a full review, but I'll point some things I noticed by taking a quick first pass over your code and then edit my post tomorrow when I have some more time to take an in-depth look. EDIT: I added more at the end of my original post, please see below.

In your Game class, method playGame - this loop (below), while technically valid is not the standard pattern for a for loop which makes it a little harder to read for anyone else looking at your code. Readability is very important for long term maintenance and although this is a small program you said you wanted to learn how to write good code in general.

for (int numGuesses = 0; true; ){

I would replace this with something more readable by breaking it into two lines:

int numGuesses = 0; 
while(true){

Also in the Game class, in the options method you have:

option = promptForInteger("Hi " + player.name + "! \n"
    + "Please select your game:\n1) Guess number between 1 and 10 with 1 mine.\n"
    + "2) Guess number between 1 and 20 with 3 mines.  (You need 20 points to play this)\n"
    + "3) Guess number between 1 and 100 with 20 mines.  (You need 100 points to play this)\n"
    + "4) Exit the game (no points needed for this :) )"
    + "\n \n \n you currently have: " + player.points + " points \n \n");

This could potentially be executed many times. It would be more efficient to use StringBuilder in situations when you have a lot of string concatenation that is repeated many times. As long as I'm writing about this method, generally a method name should describe the purpose of the method - what action is it performing? I would suggest a better name, for the sake of readability, would be something like "promptForGameOptions". Furthermore I see that this method calls these other two methods:

createNumberAndMines();
playGame(player);

Each method should have a very clear purpose and generally that purpose should be to do one specific task. When you have options calling these methods it feels as though options does not have a clear purpose because it mixes view logic with controller logic. I suggest a more appropriate place for these method calls would be in Main.main because the main method's purpose is to act as a controller for the game. If you're not familiar with the MVC (model-view-controller) pattern, you can read about it on wikipedia.

In your Player class you have a lot of file reading/writing logic that would probably be better placed in a separate utility class. I say this because as it is written now you're mixing controller logic with data model and that can lead to code that is harder to maintain in the long run. Try to keep your data separate from the logic that loads and uses the data.

I also notice that in your file reading logic within the Player class you're not handling the scenario where the data in the file is invalid in some unexpected way. For example you have:

if (line.contains("username: ")){
    name = line.substring(10);
    nameFound = true;
}

but you never handle the situation where the line contains only "username: " and nothing else. If the line is "username: " then name = "" after you run this code. Also, are you intending for there to be multiple usernames in this file? If not you could just break out of the loop or forgo the loop entirely.

That's all I have for now, hope that gives you something to work with until I can take a closer look.

EDIT:

As promised, here are a few more items I see upon taking a closer look at the code you posted.

I'll start with the Game class.

In the options method there is a bug. You tell the user they need 20 points to play option 2 but the logic checks for 10 points.

+ "2) Guess number between 1 and 20 with 3 mines.  (You need 20 points to play this)\n"
...
case 2: if (player.points < 10){

In the method createNumberAndMines I see you're using the following loop structure:

for (int i = numberOfMines; i >= 0; i--)

Like I mentioned above with the other for loop this is technically correct, but unless there is a good reason to do this the standard pattern for a for loop should be used to improve readability:

for(int i = 0; i < numberOfMines; i++)

This would also help make Game.options more readable since you could set the value of numberOfMines to 1, 3, and 20 instead of how you have it now: 0, 2, 19.

Looking more at this createNumberAndMines method I have additional concerns. The name of the method would seem to say that its purpose is to initialize the location of the mines, yet it asks for the player's guess in the beginning. This is another example of mixing view with controller logic. Also, the following line is confusing and I think it's mostly due to the variable name:

randomNumber = getRandom(); // get random number to guess

If you named this variable something like correctAnswer it would be much more clear to anyone reading the code what its purpose is. Finally, there is nothing wrong with using an array to store the mines but if you used an implementation of the List object you wouldn't need to write so much code for things like checking if a number has already been added to the mines. Here's an example of how you might rewrite some of the logic of this method to use an ArrayList:

List<Integer> mines = new ArrayList<Integer>();
...
for(int i=0; i < numberOfMines; i++){
     //Assume the mine will be invalid
     boolean isValidMine = false;
    //generate a mine and add it to the list of mines if it is valid.
     do{ 
         int mine = getRandom();
         //a mine is valid if it is not already in the list, does not equal the correct answer, and is not the first guess made by the user
         isValidMine = !mines.contains(mine) &&  mine != correctAnswer && mine != userGuess;
         if(isValidMine)
             mines.add(mine);

     }while (!isValidMine);
 }

I really only bring up the above point because it's important for Java programmers to become familiar with the Java Collections Framework. If you haven't studied it yet, here is a link to get you started. Note that having the ability to call the contains method also simplifies your code in your Game.playGame method as well because you can replace:

for (int i=numberOfMines; i>=0; i--){// check if user hits mine
    if (mine[i] == userGuess){
        ...
    }
}   

with:

if(mines.contains(userGuess)){
    ...
}

If you're not comfortable with collections you can write your own contains method and it will simplify your code in much the same way:

public boolean minesContains(int element){
    for(int i = 0; i < mine.length; i++){
        if(mine[i] == element){
            return true;
        }
    }
    return false;
}

The reason I emphasize this so much is that one of the fundamental practices of Object Oriented Programming is to reuse code as much as possible by making it modular and generic. Since there are multiple places in your code that you use a containment check you should write a generic routine that you can call multiple times instead of rewriting the same logic over and over. This helps reduce errors, improve readability, and reduce maintenance.


Moving on to the Player class, it appears you're trying to create some constants:

protected String txtPathPlayer = "src/Player.txt";
protected String txtPathPoints = "src/Points.txt";

In general if you're creating a constant you use the keywords static and final and name it with all capital letters with underscores to separate words - for example: TXT_PATH_PLAYER. You can read this page for more details about why this is done.

This is somewhat a personal preference but I find that positive expressions are more easy to read than negative ones. For example in your fileExists method you have this:

if (!(f.exists() && !f.isDirectory())) {  
   return false;
}else {
   return true;
}

This forces me to stop for a moment and think through what the logic is doing. If it were written like this:

if (f.exists() && !f.isDirectory()) {  
   return true;
}else {
   return false;
}

I can very easily see that it is going to return true when a file exists and is not a directory. You could even simplify this method more by directly returning the result of your boolean expression like this:

return f.exists() && !f.isDirectory();

Looking at the updatePoints method I think the different game modes or "options" as they're called in the code deserve to be represented as objects. The updatePoints logic is handling the calculation of points for each mode when, if you had an object to represent each mode, the objects themselves could have a method that takes numGuesses as a parameter and returns the number of points the player earned. Also, the Game.options method would be simplified as each object could have its own value for numberOfMines and max. If you really feel ambitious you could use an enum to represent them since there is a fixed number of them and they do not change.

In conclusion I think you've done some great work so far. You have a good start on a modular design and most of the code is easy to read. Just don't be afraid to go back and change names of methods and variables. Try to think from an outsider perspective and consider the purpose of each variable or method when you name them. Remember that sometimes less is more - a simple method is easier to maintain than a complex one. If you have a complex task try to break it into smaller reusable pieces. To help with debugging I recommend you either throw in some System.out.println statements or research some java logging frameworks like SLF4J, Apache Commons Logging, log4j, logback, etc. Logging can be a huge help - especially in larger projects. Another thing you should consider is learning about JUnit and creating some automated tests for your code.

Take care!

\$\endgroup\$
3
  • \$\begingroup\$ Thanks D.B., I'll read more about MVC, it's something I haven't read much about so far. All your points make sense, can't wait for your edit/update, thanks! \$\endgroup\$ – Goran B Jul 11 '16 at 10:24
  • \$\begingroup\$ you say: "Looking more at this createNumberAndMines method I have additional concerns. The name of the method would seem to say that its purpose is to initialize the location of the mines, yet it asks for the player's guess in the beginning." while this is great advice, I'm not sure how to apply it in this example, since I have method that should create mines, then ask for first guess and then create mines, maybe I should have 2 separated methods, one for creating number for guessing and other for creating mines? and in between call askForGuess(), maybe you have better solution? \$\endgroup\$ – Goran B Jul 12 '16 at 11:46
  • 1
    \$\begingroup\$ You have exactly the right idea - split up the logic and call the methods in the appropriate order. Since you have a requirement that the first guess should never hit a mine, ask for the first guess then pass that as a parameter into your method for creating mines. Another option would be to create mines first, ask for first guess and in the rare case when first guess is a mine remove that mine and generate a new one. \$\endgroup\$ – D.B. Jul 13 '16 at 0:38
3
\$\begingroup\$

Since D.B. mentioned JUnit in his thorough review I thought I might speak up on this. JUnit is a testing framework. If used properly you can test your code in isolation and test against certain events and hopefully reduce bugs put into code or by flushing the bugs out. There are many ways and patterns that you might find out there, but really the point is to get used to them and practice writing them. It is a good practice to get into but takes a ton of practice to get ok at them.

So lets start. When writing unit tests on existing code I find it easiest to pick on small classes, and although Game is smaller than Player, I think Player has some interesting methods to start trying to get under test. Much like D.B. I'm interested in what the name of a class and name of the methods tell me about what it does or is supposed to do. My first thought about Player is that it is a very simple POJO (plain old java object) that should only have 2 things points and name. In essence that is all it is, but when it comes to testing it is very hard because you have to have a file in place for it to work. Personally I think I would want another class that can read/write Player properties to a file. I would want this for two reasons. One is that If I take the complex nature of reading and writing a file out of Player I can now very easily new up a Player for any reason and use it. I would also be able to write a test on this new PlayerLoader to check what happens in various scenarios. So Lets do that. Since I have no test in place I'll write a simple PlayerLoader class and in a different folder as src (probably called test) I would put a PlayerLoaderTests class.

public class PlayerLoader {

    public Player LoadPlayer(String playerPath){
        return null;
    }
}

and the test class

import org.junit.Test;

import static org.junit.Assert.*;

public class PlayerLoaderTest {
    @Test
    public void loadPlayer() throws Exception {

    }

}

The test class was autogenerated in my IDE so don't worry about it too much. Just know that with JUnit you mark methods that need to be run to test your card with the @Test attribute. Now I can think about what I want my class to do. If no player file with that name exists I want it to return null. If it does exist I want it to return that player; If that file got corrupted I want an exception thrown.

with those in mind I wrote the following 3 tests one at a time. After I finished writing one test. I worked on it until the test passed then wrote the next one.

import org.junit.Test;

import java.io.PrintWriter;
import java.nio.file.FileSystems;
import java.nio.file.Files;

import static org.junit.Assert.*;

public class PlayerLoaderTest {
    @Test
    public void whenPlayerFileDoesNotExistLoadReturnsNull() throws Exception {
        PlayerLoader loader = new PlayerLoader();
        Player player = loader.LoadPlayer("IDontExist.txt");

        assertNull(player);
    }

    @Test
    public void whenPlayerFileIsCorrectLoadReturnsPlayer() throws Exception {
        String fileName = "Robert.txt";
        PlayerLoader loader = new PlayerLoader();
        try (PrintWriter out = new PrintWriter(fileName)) {
            out.println("username: Robert");
        }

        Player player = loader.LoadPlayer(fileName);

        Files.deleteIfExists(FileSystems.getDefault().getPath(fileName));
        assertEquals("Robert", player.name);
    }

    @Test
    public void whenPlayerIsNotCorrectFormatCorruptedUserFileExceptionIsThrown() throws Exception {
        Boolean correctExceptionThrown = false;
        String fileName = "Robert.txt";
        PlayerLoader loader = new PlayerLoader();
        try (PrintWriter out = new PrintWriter(fileName)) {
            out.println("username:"); //missing space after...silly users.
        }

        try {
            loader.LoadPlayer(fileName);
        } catch (CorruptedUserFileException e) {
            correctExceptionThrown = true;
        }

        Files.deleteIfExists(FileSystems.getDefault().getPath(fileName));
        assertTrue("CorruptedUserFileException not thrown", correctExceptionThrown);
    }
}

which then gives me this for PlayerLoader

import java.io.IOException;
import java.nio.file.FileSystems;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.List;

public class PlayerLoader {

    public Player LoadPlayer(String playerPath) throws CorruptedUserFileException {
        final Path path = FileSystems.getDefault().getPath(playerPath);
        if(!Files.exists(path))
            return null;

        List<String> lines;
        try {
            lines = Files.readAllLines(path);
        } catch (IOException e) {
            e.printStackTrace();
            return null;
        }

        String userName = null;
        for (String line : lines){
            if(!line.startsWith("username: "))
                continue;

            userName = line.substring(10);
        }
        if(userName == null)
            throw new CorruptedUserFileException();

        Player player = new Player();
        player.name = userName;

        return player;
    }
}

If it wasn't so late I'd go back and clean it up some. But i want to point out that i can continue to add tests now that Loader class is mostly complete and the similar code in the Player file could be replaced with it. And to test if i broke anything I can just re-run my tests (which all 3 tests run in under 1 second on my computer)

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Nice examples of JUnit. If you're going this far with it you could mention code coverage and perhaps even briefly touch on tools like EclEmma \$\endgroup\$ – D.B. Jul 12 '16 at 5:46

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.