2
\$\begingroup\$

In the last days I've written a number guessing game that I've posted on this site.

I have now written a lottery simulation in which I tried to acknowledge the criticisms of the project mentioned above. I also tried to add error handling. If the user enters invalid numbers or complete bullshit, the program will not crash any longer.

You can test the program here. Dont wonder: The online interpreter needs a little time before it start the program.

Have I sufficiently implemented the criticism of the last Code Review? Are there new things that should be better?

Thank you for answers!

Main.java

import java.util.Scanner;

public class Main {
    private static Scanner scanner = new Scanner(System.in);

    public static void main(String[] args) {
        LotteryGame game = new LotteryGame(6, 1, 49);
        System.out.println(game.getExplanationString());
        boolean run = true;
        while (run) {
            game.play();
            System.out.print("Continue (Y / N): ");
            System.out.flush();
            run = scanner.nextLine().equalsIgnoreCase("Y");
            System.out.println();
        }
    }   
}

LotteryGame.java

import java.util.Scanner;

public class LotteryGame {
    private Scanner scanner = new Scanner(System.in);

    private final int drawSize;
    private final int drawLowerLimit;
    private final int drawUpperLimit;

    public LotteryGame(int drawSize, int drawLowerLimit, int drawUpperLimit) {
        this.drawSize = drawSize;
        this.drawLowerLimit = drawLowerLimit;
        this.drawUpperLimit = drawUpperLimit;
    }

    public void play() {
        // generate new random draw
        Draw draw = Draw.generateRandomDraw(drawSize, drawLowerLimit, drawUpperLimit);

        // let the user guess
        Draw userDraw = guess();

        // compare the draws and print result
        int rightNumbers = draw.compare(userDraw);
        System.out.println("Your guess: " + userDraw.getStringRepresentation());
        System.out.println("Draw:       " + draw.getStringRepresentation());
        System.out.println("You guessed " + rightNumbers + " right!");
        System.out.println();
    }

    public Draw guess() {
        int numbers[] = new int[drawSize];
        while (true) {
            try {
                for (int i = 0; i < drawSize; i++) {
                    numbers[i] = HelpfulFunctions.saveIntInput("Number " + (i+1) + ": ");
                }
                System.out.println();
                return new Draw(numbers);
            } catch (IllegalArgumentException e) {
                System.out.println("The numbers have to be unique\n");
            }
        }
    }

    public String getExplanationString() {
        StringBuilder sb = new StringBuilder();
        sb.append("You have to guess the numbers of a lottery draw.\n");
        sb.append("A draw consists of 6 different numbers.\n");
        sb.append("Each number is in a range between 1 and 49.\n");
        sb.append("The more numbers you guess right, the luckier you can be!\n");
        return sb.toString();
    }
}

Draw.java

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

public class Draw {
    public int[] numbers;

    public Draw(int[] numbers) throws IllegalArgumentException {
        // check if all numbers are unique
        for (int i = 0; i < numbers.length; i++) {
            for (int j = 0; j < numbers.length; j++) {
                if (i == j) continue;
                if (numbers[i] == numbers[j]) {
                    throw new IllegalArgumentException("All numbers have to be unique.");
                }
            }
        }

        this.numbers = numbers;
    }

    public static Draw generateRandomDraw(int numberOfEntries, int lowerLimit, int upperLimit) {
        // generate list that contains possible values
        List<Integer> possibleValues = new ArrayList<>();
        for (int i = lowerLimit; i <= upperLimit; i++) {
            possibleValues.add(i);
        }

        // fill draw with randomly picked values
        int[] numbers = new int[numberOfEntries];
        Random random = new Random();
        for (int i = 0; i < numberOfEntries; i++) {
            int randomIndex = random.nextInt(possibleValues.size());
            Integer pickedValue = possibleValues.get(randomIndex);
            possibleValues.remove(pickedValue);
            numbers[i] = pickedValue;
        }

        Arrays.sort(numbers);

        return new Draw(numbers);
    }

    // returns count of equal numbers
    public int compare(Draw draw) throws IllegalArgumentException {
        if (draw.numbers.length != numbers.length) {
            throw new IllegalArgumentException("The draws dont have the same length.");
        }

        int count = 0;
        for (int i = 0; i < numbers.length; i++) {
            for (int j = 0; j < numbers.length; j++) {
                if (numbers[i] == draw.numbers[j]) {
                    count++;
                }
            }
        }

        return count;
    }

    public String getStringRepresentation() {
        StringBuilder sb = new StringBuilder();
        sb.append("[" + numbers[0]);
        for (int i = 1; i < numbers.length; i++) {
            sb.append(", " + numbers[i]);
        }
        sb.append("]");
        return sb.toString();
    }
}

HelpfulFunctions.java

import java.util.Scanner;

public class HelpfulFunctions {
    private static Scanner scanner = new Scanner(System.in);

    public static int saveIntInput(String message) {
        while (true) {
            try {
                System.out.print(message);
                System.out.flush();
                return Integer.valueOf(scanner.nextLine());
            } catch (NumberFormatException e) {
                System.out.println("Not a valid number.");
            }
        }
    }
}
\$\endgroup\$
3
\$\begingroup\$

I'll put my comments inline. In order:

Main:

import java.util.Scanner;

public class Main {

I saw this came up in your previous question, too, but Main is not a good name. As someone seeing your code for the first time, I have no idea what Main is - I know from the name that it's the entrypoint to your logic, but I have no idea what logic it's the entrypoint for. For classes with a main() method that starts up some other class' logic, I like the _Runner naming scheme. E.g. LotteryGameRunner. What does it do? It runs the lottery game. I already know what type of logic to expect (a main() method) and what system the logic belongs to.

    private static Scanner scanner = new Scanner(System.in);

    public static void main(String[] args) {
        LotteryGame game = new LotteryGame(6, 1, 49);

Magic numbers: I don't know what 6, 1, and 49 mean. I see them defined in another class, but it's a pain to have to look there every time I need to double check whether it's 6 draw size and 1 lower limit, or 1 draw size and 6 lower limit. Use variables just like you do in the other class, so when you need to change them later you can be sure you're changing the one you think you're changing, without needing to count its index in the parameters to be sure.

Whether it's necessary to pull out parameters into temporary variables varies, but in this case there are two strong arguments for it: 1. these are int's, so we have no information about their meaning as we would if they were named objects, 2. these are really configuring your program - they're not calculated, you're just providing them in the code. Configuration, as much as possible, should be pulled out of logic, so that when you need to change the configuration, you're not touching a line of logic.

        System.out.println(game.getExplanationString());
        boolean run = true;
        while (run) {
            game.play();
            System.out.print("Continue (Y / N): ");
            System.out.flush();
            run = scanner.nextLine().equalsIgnoreCase("Y");
            System.out.println();
        }
    }   
}

The above looks good. Nice!

LotteryGame:

import java.util.Scanner;

public class LotteryGame {
    private Scanner scanner = new Scanner(System.in);

    private final int drawSize;
    private final int drawLowerLimit;
    private final int drawUpperLimit;

These are nicely named.

    public LotteryGame(int drawSize, int drawLowerLimit, int drawUpperLimit) {
        this.drawSize = drawSize;
        this.drawLowerLimit = drawLowerLimit;
        this.drawUpperLimit = drawUpperLimit;
    }

And this is a good way to do the constructor.

    public void play() {
        // generate new random draw
        Draw draw = Draw.generateRandomDraw(drawSize, drawLowerLimit, drawUpperLimit);

        // let the user guess
        Draw userDraw = guess();

        // compare the draws and print result
        int rightNumbers = draw.compare(userDraw);
        System.out.println("Your guess: " + userDraw.getStringRepresentation());

See comment on getStringRepresentation() below.

        System.out.println("Draw:       " + draw.getStringRepresentation());
        System.out.println("You guessed " + rightNumbers + " right!");
        System.out.println();
    }

    public Draw guess() {
        int numbers[] = new int[drawSize];
        while (true) {
            try {
                for (int i = 0; i < drawSize; i++) {
                    numbers[i] = HelpfulFunctions.saveIntInput("Number " + (i+1) + ": ");
                }
                System.out.println();
                return new Draw(numbers);
            } catch (IllegalArgumentException e) {
                System.out.println("The numbers have to be unique\n");
            }

Try-catch blocks should generally not be used for normal logic. A good rule of thumb is that, if the same method keeps running after the try-catch block is executed, it shouldn't be a try-catch block (there are a lot of caveats to this; one very common pattern that rightfully breaks this rule is failing slowly - running a processing loop over a collection that processes every item, catching every exception and waiting until the end to throw them all). In this case, you're using the try-catch to enable a loop in the logic itself. Instead, just check whether all the numbers the user has given you were unique:

        while (true) {
            for (int i = 0; i < drawSize; i++) {
                numbers[i] = HelpfulFunctions.saveIntInput("Number " + (i+1) + ": ");
            }
            System.out.println();

            // a set inserts only unique items, leaving out duplicates
            if (new HashSet<Integer>(Arrays.asList(numbers)).size() == numbers.length) {
                return new Draw(numbers);
            } else {
                System.out.println("The numbers have to be unique\n");
            }
        }
    }

    public String getExplanationString() {
        StringBuilder sb = new StringBuilder();
        sb.append("You have to guess the numbers of a lottery draw.\n");
        sb.append("A draw consists of 6 different numbers.\n");
        sb.append("Each number is in a range between 1 and 49.\n");
        sb.append("The more numbers you guess right, the luckier you can be!\n");
        return sb.toString();
    }
}

1 and 49?! If that's set in stone, then they shouldn't be input parameters above. If it's not set in stone, those same variables should be used here - you'll probably need to pull them out into public final static constants.

Also, is there ever a case when you're not immediately printing the result of this method? If there is not, then it would be simpler to rename it to public void printExplanationString() and have it System.out.println() each of its currently appended lines.

Draw:

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

public class Draw {
    public int[] numbers;

    public Draw(int[] numbers) throws IllegalArgumentException {
        // check if all numbers are unique
        for (int i = 0; i < numbers.length; i++) {
            for (int j = 0; j < numbers.length; j++) {
                if (i == j) continue;
                if (numbers[i] == numbers[j]) {
                    throw new IllegalArgumentException("All numbers have to be unique.");
                }
            }
        }

Use the same uniqueness checking as above - it'll be much shorter, and it's actually more efficient than a nested loop :)

        this.numbers = numbers;
    }

    public static Draw generateRandomDraw(int numberOfEntries, int lowerLimit, int upperLimit) {
        // generate list that contains possible values
        List<Integer> possibleValues = new ArrayList<>();
        for (int i = lowerLimit; i <= upperLimit; i++) {
            possibleValues.add(i);
        }

        // fill draw with randomly picked values
        int[] numbers = new int[numberOfEntries];
        Random random = new Random();
        for (int i = 0; i < numberOfEntries; i++) {
            int randomIndex = random.nextInt(possibleValues.size());
            Integer pickedValue = possibleValues.get(randomIndex);
            possibleValues.remove(pickedValue);
            numbers[i] = pickedValue;
        }

Rather than generate your own random ints between lowerLimit and upperLimit, you can let Random do it for you. Check out Random.ints()

        Arrays.sort(numbers);

        return new Draw(numbers);
    }

    // returns count of equal numbers
    public int compare(Draw draw) throws IllegalArgumentException {
        if (draw.numbers.length != numbers.length) {
            throw new IllegalArgumentException("The draws dont have the same length.");
        }

        int count = 0;
        for (int i = 0; i < numbers.length; i++) {
            for (int j = 0; j < numbers.length; j++) {
                if (numbers[i] == draw.numbers[j]) {
                    count++;
                }
            }
        }

Careful! What's the maximum value returned by compare()? It should probably be numbers.length. Is it?

        return count;
    }

    public String getStringRepresentation() {
        StringBuilder sb = new StringBuilder();
        sb.append("[" + numbers[0]);
        for (int i = 1; i < numbers.length; i++) {
            sb.append(", " + numbers[i]);
        }
        sb.append("]");
        return sb.toString();
    }
}

As another answer has mentioned: this method is doing what toString() is supposed to do. Just name it toString and add an @Override tag, and you'll be good to go.

HelpfulFunctions:

This name is not the best. Classes like these are nifty for beginning programmers who tinker with similar things a bunch. However, they're still not as good as just bundling related logic into helpful library classes named for what they do (InputSaving.java, anyone?). Furthermore, this class only has one method. Until you know you need this method in another place, just leave it as part of a class for this system. LotteryGame is probably a good spot.

import java.util.Scanner;

public class HelpfulFunctions {
    private static Scanner scanner = new Scanner(System.in);

    public static int saveIntInput(String message) {
        while (true) {
            try {
                System.out.print(message);
                System.out.flush();

The method is called saveIntInput, but it's not saving anything (it's just returning an attempt to parse a String to an int), and worse, it is doing things other than that: it's printing messages. Another good rule of thumb is that, except for temporary debugging println()'s, methods should only call System.out.println() if they're communicating directly with the user. Couple this with the fact that communication with the user should be at the highest possible level, wrapping logic that doesn't know anything about the user at all, and it's clear that we shouldn't be printing here.

As usual, there's an exception (kind of) to the above. Logging - methods throughout a system might log messages to make a record of what has happened. However, this is generally done via a Logger from a logging framework, not using System.out().

                return Integer.valueOf(scanner.nextLine());
            } catch (NumberFormatException e) {
                System.out.println("Not a valid number.");
            }
        }
    }
}

Overall: Your code style is good, and overall naming and conventions are clean. There are a couple spots where small changes could be cleaner, but your code is in general nice to read.

You've obviously incorporated feedback from your last review, and that's great. A lot of my suggestions here are less "you did something wrong" and more "here's a better way you might not know about". Nice job!

\$\endgroup\$
1
\$\begingroup\$

Some suggestion, I have seen you used a method called to getStringRepresentation() in the Draw class to print a representation of the object: for all classes you can use the common method toString, overriding it for every class with the @Override annotation. You can then use in your code System.out.println("Your guess: " + draw) instead of System.out.println("Your guess: " + draw.getStringRepresentation()) obtaining the same result. It seems me from your code that you have multiple Scanner instances referring to stdin inside your classes; it should be better encapsulate the scanner in one place and call it from every class that use it, see this for details.

\$\endgroup\$
  • \$\begingroup\$ I've heard that toString() is only meant for technical sort of output and that this method should not be used for displaying user output \$\endgroup\$ – Dexter Thorn Aug 26 at 14:21
  • \$\begingroup\$ It is used for representation as a 'String' for every object internal state and it has been included in the Object class and consequently in all classes including those created by a user, so it is already present in your classes .From the link to Java documentation "Returns a string representation of the object and it is recommended that all subclasses override this method". \$\endgroup\$ – dariosicily Aug 26 at 16:15
  • \$\begingroup\$ Hmmm, okay I guess you're right - I will account that in a later version! \$\endgroup\$ – Dexter Thorn Aug 26 at 17:49

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.