Capitals and countries game

Question

This is my simple game that gives you a country and has you enter the corresponding capital. What do you think? (Before running it you need to do score.txt.)

package com.company;

import java.io.File;
import java.io.FileNotFoundException;
import java.io.PrintWriter;
import java.util.Random;
import java.util.Scanner;

public class Main {

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


        Random r = new Random();

        Scanner input = new Scanner(System.in);



        String[] capital = {"Lisbon", "Madrid", "Paris", "Berlin", "Warsaw", "Kiev", "Moscow", "Prague", "Rome"};
        String[] country = {"Portugal", "Spain", "France", "Germany", "Poland", "Ukraine", "Russia", "Czech Republic", "Italy"};
        Boolean[] was = new Boolean[9];

        int score = 0;
        int good = 0;
        int bad = 0;


        File file = new File("score.txt");
        Scanner lastscoreopener = new Scanner(file);
        String lastscore = lastscoreopener.nextLine();
        System.out.print("Your last score was: " +lastscore +"\n");



        for (int d = 1; d<4; d++) {
            int randomint = r.nextInt(9);
            while (was[randomint] == Boolean.FALSE){
                randomint = r.nextInt(9);
            }
            was[randomint] = false;


            while (was[randomint] == true);
            for (int x = 0; x < 3; x++) {
                System.out.print("What's capital of " + country[randomint] + "\n");
                String answer = input.nextLine();
                if (answer.toLowerCase().equals(capital[randomint].toLowerCase())) {
                    if (x==0){
                        score = score+3;
                        good++;
                    }
                    else if(x==1){
                        score = score+2;
                        good++;
                    }
                    else if (x==2){
                        score = score+1;
                        good++;
                    }
                    System.out.print("Good. Your score is" +score +". \n");
                    break;
                } else {
                    if (x != 2){
                        System.out.print("Bad. You have " +(2-x) +" chances. ");}
                    if (x == 2){
                        System.out.print("Bad. Good answer is" +capital[randomint] +"\n");
                    }
                    bad++;
                }
            }
        }
        System.out.print("End of game. Good answers:" +good +". Bad answers: " +bad +". Score: " +score);


        PrintWriter saver = new PrintWriter("score.txt");
        saver.println(score);
        saver.close();

    }
}

Answer 1

Match what you say to what you are actually doing

        for (int d = 1; d<4; d++) {

You want to loop three times. Why does it say 4? Either

        for (int d = 1; d <= 3; d++) {

or more idiomatically

        for (int d = 0; d < 3; d++) {

Since you never use d except for iteration, there seems no reason to start from 1 instead of 0.

Use helper objects

class Country {
    private final String name;
    private final String capital;

    public Country(String capital, String name) {
        this.name = name;
        this.capital = capital;
    }

    public String getName() {
        return name;
    }

    public String getCapital() {
        return capital;
    }
}

Now you can say

List<Country> countries = new ArrayList<>();
countries.add(new Country("Lisbon", "Portugal"));

Same pattern for the other countries. Now if you add a new country, you can't accidentally add the capital to a different index than the country name.

Consider making this a class field

private static final List<Country> countries = new ArrayList<>();
static {
    countries.add(new Country("Lisbon", "Portugal"));

Don't forget to close the static initializer block after adding all the countries and capitals.

}

Either way, you can then say

List<Country> choices = new ArrayList<>(countries);

You use it like

Country choice = choices.remove(r.nextInt(choices.size()));

Now you don't need a was variable. This is self-maintaining. It only picks from the entries still in the list.

Also, if you add an entry to the list, it will automatically include it. You don't have to go through your code changing all your magic numbers to match each other.

Use methods to hide complexity

        File file = new File("score.txt");
        Scanner lastscoreopener = new Scanner(file);
        String lastscore = lastscoreopener.nextLine();
        System.out.print("Your last score was: " +lastscore +"\n");

The point of these four lines is to output one line to the screen. It creates three variables to do so which are never used again. So hide that in a method. Either

    private static final String SCORE_FILE_NAME = "score.txt";

    public void displayLastScore() {
         File file = new File(SCORE_FILE_NAME);
         try (Scanner scanner = new Scanner(file)) {
             System.out.println("Your last score was: " + scanner.nextLine());
         }
    }

Or with better separation of concerns

    private static final String SCORE_FILE_NAME = "score.txt";

    public String readLastScore() {
         File file = new File(SCORE_FILE_NAME);
         try (Scanner scanner = new Scanner(file)) {
             return scanner.nextLine();
         }
    }

which you use like

         System.out.println("Your last score was: " + readLastScore());

The latter is generally recommended, as it leaves readLastScore more flexible.

Either version will close the Scanner when done, as that's what the try-with-resources form does. It ensures that the AutoCloseable resource is closed before leaving the try block.

Answer 2

String[] capital = {"Lisbon", "Madrid", "Paris", "Berlin", "Warsaw", "Kiev", "Moscow", "Prague", "Rome"};
String[] country = {"Portugal", "Spain", "France", "Germany", "Poland", "Ukraine", "Russia", "Czech Republic", "Italy"};

Two string arrays with members that are supposed to match by index? Extremely difficult to maintain, as I learned from my own experience. Use a map instead, so you can do something like:

String capital = countryCapitalMap.get(countryName);

This will help ensure there is an entry for each country/capital and make it so you only have to update things in one place.

---

Don't hardcode the length of the array:

int randomint = r.nextInt(9);

Use the length of the array instead, or for a map, use the size() method.

---

Your naming isn't always clear. How do I know what r stands for without seeing the definition? How did you determine to use d as your loop variable?

---

Just use the ordinary false keyword instead of the explicit Boolean.FALSE field here:

while (was[randomint] == Boolean.FALSE)

Answer 3

Simple arithmetic instead of conditionals

if (x==0){
    score = score+3;
    good++;
}
else if(x==1){
    score = score+2;
    good++;
}
else if (x==2){
    score = score+1;
    good++;
}

Is the same as:

score += 3 - x;
good++;

That is much simpler and shorter.

Answer 4

• To link countries to their capitals, use a map. It allows you to access one by providing the other as an index, all in one object. I would break up your code as much as is reasonable, so I would have a separate method populate the map with values.
• I can't see a point to while (was[randomint] == true);
• Rather than answer.toLowerCase().equals(capital[randomint].toLowerCase()) you can use answer.equalsIgnoreCase(capital[randomint])

• Allow your code to be more clear by use of a while loop instead of a for loop when giving the user extra chances. Take a look at my code below, which does the same thing in fewer statements and provides a single variable which can be modified if you wish to change the number of attempts possible:

  int attemptsLeft = 3;
  
  do {
      System.out.println("What's the capital of " + country[randomint]);
  
      String answer = input.nextLine();
  
      if (answer.equalsIgnoreCase(capital[randomint])) {
          score += attemptsLeft;
  
          ++good;
  
          attemptsLeft = 0;
  
          System.out.println("Good. Your score is " + score + ".");
      } else {
          --attemptsLeft;
  
          ++bad;
  
          if (attemptsLeft == 0) {
              System.out.println("Wrong. The correct answer is " + capital[randomint]);
          } else {
              System.out.println("Wrong. You have " + attemptsLeft + " attempts remaining.");
          }
      }
  } while (attemptsLeft > 0);
  

• Rather than having your main throw an exception, I would handle the exception by either setting the score to 0 or by stopping the "last score" print from being printed

• I made this clear in my example above, however rather than using \n you can use System.out.println to have a new line printed at the end of your text.
• If you wish to continue using the print method instead of println, I would use \r\n instead of just \n as some environments require a carriage return to register a new line.
• I would use lowerCamelCase, a convention which joins words in variable names together and each additional word begins with an uppercase letter. More specifically, I would make the "randomint" variable named "randomInt" instead (although in my example above I used "randomint" for compatibility with the rest of the code).
• Try to make your variable names as detailed as you can, without making them too long. Something like "was" may make sense to you, however looking at the code fresh off the bat someone may not be able to understand what it is meant to be until they look at your usage of it. A name such as "previousCountries" or "previousIndexes" may be more appropriate.
• Instead of using a fixed boolean array I would create a List containing all previous indexes. You can use the contains method to see if the index has been come across before.
• Finally, I'd say take control of your spacing. You seem to have plenty of empty lines in some places, but no empty lines in others. Both too much spacing and a lack of spacing is generally considered a bad thing, so review different styles and find something neat and balanced.

Don't be off-put by the list! Learn from these points and keep coming back to Code Review, I personally have found it a great help.

Answer 5

Use String.equalsIgnoreCase and replace this:

if (answer.toLowerCase().equals(capital[randomint].toLowerCase()))

With:

if (answer.equalsIgnoreCase(capital[randomint]))

The score part can be simplified.

Replace:

    if (x==0){
        score = score+3;
        good++;
    }
    else if(x==1){
        score = score+2;
        good++;
    }
    else if (x==2){
        score = score+1;
        good++;
    }
    System.out.print("Good. Your score is" +score +". \n");

With

    score += 3 - x;
    good++;
    System.out.print("Good. Your score is" +score +". \n");

Answer 6

Not strictly a programming tip, but ... your user will need to be able to correctly spell the capital city. You can use algorithms like Soundex to give them a bit of leeway in the spelling of the word or a typo in the response.