Lottery in the command line

Question

I have written a game in which the user has to guess six numbers between 1 and 49. I want that my program gets checked in the following criteria:

1. Does this program meet the requirements of object-oriented thinking and programming?

2. Did I have used the library of the Java language wisely or were parts of the program implemented more cumbersome than they must be?

3. Are I / O instructions in the right place?

4. Are there other things that are not mentioned here but can be improved?

The program can be tested here.

Main.java

public class Main {
    public static void main(String[] args) {
            Game game = new Game();
            game.play();
    }
}

Game.java

import java.util.Collections;
import java.util.List;
import java.util.ArrayList;
import java.util.Scanner;

public class Game {
    private Lottery lottery;
    private List<Integer> guess;
    private Integer guessedNumber;
    private boolean inputAccepted;
    private int rightNumbers;
    private static Scanner scanner = new Scanner(System.in);

    public Game() {
        lottery = new Lottery();
        guess = new ArrayList<Integer>();
        guessedNumber = 0;
        inputAccepted = false;
        rightNumbers = 0;
    }

    public void play() {
        System.out.println("You have to guess six numbers.\n");
        guessNumbers();
        compareGuess();
        printScore();
    }

    private void guessNumbers() {
        while (guess.size() < 6) {
            System.out.print("Number nr." + (guess.size() + 1) + ": ");
            String input = scanner.nextLine();
            guessedNumber = Integer.valueOf(input);
            checkInput();
            if (inputAccepted) {
                guess.add(guessedNumber);
            }
        }
        System.out.println();
        Collections.sort(guess);
    }

    // checks if input is valid
    private void checkInput() {
        inputAccepted = true;
        // check if number is out of range
        if (guessedNumber > 49 || guessedNumber < 1) {
            System.out.println("You have to guess a number between 1 and 49.");
            inputAccepted = false;
        }

        // check if number allready exists in list
        for (int i = 0; i < guess.size(); i++) {
            if (guess.get(i).equals(guessedNumber)) {
                System.out.println("You allready have guessed this number");
                inputAccepted = false;
                break;
            }
        }
    }

    private void compareGuess() {
        for (int i = 0; i < 6; i++) {
            for (int j = 0; j < 6; j++) {
                if (lottery.getDraw().get(i).equals(guess.get(j))) {
                    rightNumbers++;
                }
            }
        }
    }

    private void printScore() {
        System.out.println("Draw:       " + lottery.getDraw());
        System.out.println("Your guess: " + guess);

        if (rightNumbers == 0) {
            System.out.println("You have no right numbers.");
        } else if (rightNumbers == 1) {
            System.out.println("You have one right numbers.");
        } else if (rightNumbers > 1) {
            System.out.println("You have " + rightNumbers + " right numbers.");
        }
    }
}

Lottery.java

import java.util.Collections;
import java.util.List;
import java.util.ArrayList;
import java.util.Random;

public class Lottery {  
    private static Random random = new Random();
    private List<Integer> draw;

    public Lottery() {
        renewDraw();
    }

    public List<Integer> getDraw() {
        return draw;
    }

    public void renewDraw() {
        draw = new ArrayList<Integer>();
        while (draw.size() < 6) {
            Integer number = new Integer(random.nextInt(49) + 1);
            boolean drawHasNumber = false;
            for (int i = 0; i < draw.size(); i++) {
                if (draw.get(i).equals(number)) {
                    drawHasNumber = true;
                    break;
                }
            }
            if (!drawHasNumber) {
                draw.add(number);
            }
        }
        Collections.sort(draw);
    }

    public int compare(List<Integer> guess) {
        int rightNumbers = 0;
         for (int i = 0; i < 6; i++) {
             for (int j = 0; j < 6; j++) {
                 if (draw.get(i).equals(guess.get(j))) {
                     rightNumbers++;
                 }
             }
         }
         return rightNumbers;
    }
}

Answer 1

First of all, I've seen much worse programs from "professionals". So take my following hints as suggestions for improvement and not at all as criticism.

Object-oriented thinking

I'd introduce another class, Guess, consisting of the six numbers the user guessed, and give it a method public static Guess fromConsole() { ... }, responsible for doing the console dialogue, checking for validity and returning a valid Guess.

A minor suggestion : rename the Lottery class to LotteryDraw, as one instance doesn't represent a whole weekly lottery, but a single lottery draw (but as English isn't my native language, maybe my understanding of the word "lottery" is wrong...).

Usage of the Java library

You're already doing a good thing: you're declaring your fields and variables with a rather generic interface, e.g. List, and only create the instance with the implementation you want (ArrayList).

But you can make things much simpler by using the Set instead of the List interface. A Set doesn't contain a single element more than once, and it doesn't care about the insertion order. These features perfectly fit both the draw and the guess, and if you use a sorted set (interface SortedSet, implementation e.g. TreeSet), the numbers come out in the neat ascending order that you needed to create with the Collections.sort() call. And the comparison becomes a trivial one-liner:

new TreeSet<>(draw).retainAll(guess).size();

a.retainAll(b) removes from a all the elements not contained in b, so effectively computes the intersection (but modifies a, so it's a good idea to make a copy beforehand, what I did with new TreeSet<>(draw)). And then what remains is just returning the size of the result.

Creating the draw also becomes very easy:

public void renewDraw() {
    draw = new TreeSet<>();
    while (draw.size() < 6) {
        Integer number = new Integer(random.nextInt(49) + 1);
        // if number is already there, nothing happens.
        draw.add(number);
    }
}

I/O instructions

You already placed the I/O mostly into two methods, guessNumbers() and printScore(), so that's ok.

What I don't like, is the checkInput() method. It mixes business logic (numbers mustn't appear twice) with user output. Why is that bad? If you later want to give your game a fancy graphical user interface with e.g. Swing or SWT or publish it as a web service, you want to keep the business logic, but replace the console I/O, and that will be easier if they are separated rigorously. So you could e.g. have the checkInput() method return the information string in case of invalid data or null if everything is ok. Then another method (probably guessNumbers()) will only be responsible for presenting that to the console I/O.

Local variables vs. method arguments vs. instance fields

You declare nearly everything as instance fields. I typically use instance fields only for things that are needed to persist between calls of top-level methods, and describe a valid state of the instance.

E.g. guessedNumber is only used inside the guessNumbers() method and for communication into the checkInput() method. I'd make it a local variable inside the loop of guessNumbers(), and pass it to the checkInput() method as a parameter.

Another thing: when getting input from the user, you immediately put it into the instance field guess, so temporarily the instance has a guess filled with less than six numbers, something I'd not call a valid instance state. I'd instead use a local variable inputGuess inside the guessNumbers() method, and only assign the instance field when the input has been completed and validated. And by the way, as you don't clear the guess field at the beginning of the input, calling play() twice will not work as expected (not accept an input at all, or add another six numbers, depending on the implementation details). Using a local variable will avoid that problem. As a consequence, you'll then have to pass that local var to the checkInput() method as well: private String checkInput(int guessedNumber, Set<Integer> guessSoFar) { ... }.

Summary

This program isn't bad, but maybe I was able to show you a few aspects worth considering for improvement. It's based on my personal style that I developed in 20 years of Java experience.

Answer 2

One quick suggestion would be to create an interface for the methods in Lottery and Game and implement those methods separately in each of the classes.