Guess a number game with mines version 3

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!");

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

StackExchange Code Review Q#134042, answer score: 3