Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I've created a simple game, in which the user needs to guess 4 digits number between 0-9, generated randomly using Random() Each of the 4 digits are different from each other, with no repeated digits. The user has 5 attempts to guess, and each failed (almost correct) guess will tell the user in which part they got it correct.

Do you think this is too hard, or too easy for the user to guess? I also need some review of this code. Perhaps some of it can be simplified?

import java.util.Arrays;
import java.util.Random;
import java.util.Scanner;

public class Game {

    public static void main(String[] args) {      
        System.out.println("The computer has generate a unique 4 digit number.\n"
                + "You can try to guess the 4 digits number in 5 attempts.\n");
        System.out.println("_______________________________________________________\n");
        int[] random=numberGenerator();
        int maxTry=5;
        int indexMatch=0;
        int match=0;
        while(maxTry>0 && indexMatch!=4){
            int[] guess=getGuess();
            indexMatch=0;
            match=0;
            for(int i=0;i<guess.length;i++){
                if(guess[i]==random[i]){
                    indexMatch++;
                }
                else if(guess[i]==random[0] || guess[i]==random[1] || guess[i]==random[2] || guess[i]==random[3]){
                    match++;
                }
            }
            if(indexMatch==4){
                System.out.print("Well done! Your guess is Correct! The number is: ");
                for(int i=0;i<guess.length;i++){
                    System.out.print(guess[i]);
                }
            }
            else{
                maxTry--;
                if(maxTry>1){
                    System.out.println("You have guess "+indexMatch+" correct number in correct position,"+
                    " and "+match+" correct number in incorrect position. \n"+maxTry+" attempt remaining.");
                }
                else if(maxTry==1){
                    System.out.println("You have guess "+indexMatch+" correct number in correct position,"+
                    " and "+match+" correct number in incorrect position. \nLast attempt!. Good luck");
                }
                else{
                    System.out.println("Sorry, you failed to guess the number in 5 attempts.");
                    System.out.print("The number is: ");
                    for(int i=0;i<random.length;i++){
                        System.out.print(random[i]);
                }
            }
        }
    }   
}

public static int[] getGuess(){
    Scanner keyboard = new Scanner(System.in);
    System.out.println("Please enter your guess: ");
    String input = keyboard.nextLine();
        if(input.length()!=4 || input.replaceAll("\\D","").length()!=4){
            System.out.println("Invalid number. You must enter 4 digits between 0-9 only.");
            return getGuess();
    }
    int[] guess = new int[4];
    for (int i = 0; i < 4; i++) {
        guess[i] = Integer.parseInt(String.valueOf(input.charAt(i)));
    }
    return guess;
}

public static int[] numberGenerator() {
    Random randy = new Random();
    int[] randArray = {10,10,10,10};

    for(int i=0;i<randArray.length;i++){
        int temp = randy.nextInt(9);
        while(temp == randArray[0] || temp == randArray[1] || temp == randArray[2] || temp == randArray[3]){
            temp=randy.nextInt(9);
        }
        randArray[i]=temp;      
    }
    return randArray;
}
}
share|improve this question

6 Answers 6

up vote 6 down vote accepted

I will not comment on the random number generation, variable naming, or general code structure - you have enough good information in the above.

But here is my two cents for the "is it too hard?" part of your question.

The game you describe used to be known as "bulls and cows", and

It's proven that any number could be solved for up to seven turns. Minimal average game length is 26274/5040=5.2131 turns.

References: http://fourdigits.sourceforge.net, http://www.cs.nccu.edu.tw/~chaolin/papers/science3203.pdf

So yes, only four guesses is pretty hard. Five guesses gives you a less than even chance, six is better than even.

If you are trying to solve the game with a computer, you typically want to find the guess that minimizes the maximum number of valid combinations that remain. It turns out that if you start with the guess

1234

the "worst answer" you can get back is

1 white peg (one number correct, in the wrong location)

which leaves you with 1440 possible combinations (any of the 4 numbers could be the one with the white peg, but it would be in the wrong position. That leaves you with 4 possible numbers in each of three positions, times 6x5x4 for the other three open positions (which must be filled by one of the numbers not yet used).

You can actually make a list of all possible combinations, and "cross off" the ones that don't fit in every turn; combining this with "if I guess abcd, and the response is [x white, y black], how many combinations are there left?" and again picking the guess that minimizes that number. Quite easy to do in a computer.

One other bug in your code

In some situations, the best guess is one that is "illegal" - that is, you ask for a combination that is inconsistent with the information you have so far. It can even include a doubled-up digit - e.g.

1123

You have to make sure that your code deals with that input correctly. The way to do this is to change your test loop slightly - instead of

for(int i=0;i<guess.length;i++){
  if(guess[i]==random[i]){
    indexMatch++;
  }
  else if(guess[i]==random[0] || guess[i]==random[1] || guess[i]==random[2] || guess[i]==random[3]){
    match++;
  }
}

You need

for(int i=0;i<guess.length;i++)
  if(guess[i]==random[i]){
    indexMatch++;
  }
  else if( random[i] == guess[0] || random[i] == guess[1] || random[i] == guess[2] || random[i] == guess[3]){
     match++;
   }
 }

Look what happens if the random code is

1234

and the guess is

3132

The correct answer would be "one black, two white". But your code would give "one black, three white" since the repeated three in the guess gets counted twice. By switching around the test, each value in the random number only gets looked at once - and that makes all the difference.

share|improve this answer

As this part of the question has been skipped so far, I'll take it:

Do you think this is too hard?

I think it is. Similarly to your game, in classic Mastermind, the player has to guess a combination of 4 non-unique coloured pegs, and each time is given a mark for the number of correct position and colour combinations.

64 = 1,296 possible combinations

However, with your game (to borrow the nomenclature), there are 10 possible pegs, and as they're unique the combinations are:

10 x 9 x 8 x 7 = 5,040 possible combinations

Which is an increase of 288.8%, furthermore you're only giving the user 5 attempts to guess, as opposed to Mastermind's 6, 8 or 12. The reason Mastermind plays with a minimum of six, is that you get to test every combination, e.g:

Mastermind guessing

However, when you increase the number of possibilities to 10, and reduce the guesses to 5, you can no longer do this, and the chances of never guessing a correct number increase dramatically, e.g.:

Question guessing

Therefore, I think you need to either increase the number of guesses to 10 (at the least), or chose a smaller set of things to guess, like A-F, 1-6, or something GUI based like Mastermind.

share|improve this answer
    
great. with 0.001 probability of winning, perhaps I should just make this as a gamble game. –  Immer Allein Jan 24 at 11:51
7  
-1 Mastermind is completely different from what you just described. In Mastermind, the player is told how many pegs are the correct color but in the wrong position, and how many are the correct color and position. Thus, in your first example 1234 would be given 3 white pegs, giving the player significantly more information than what you show here. Also, Mastermind can always be beaten in 5 moves. –  BlueRaja - Danny Pflughoeft Jan 24 at 19:56

Just focusing on one specific point for the moment:

int temp = randy.nextInt(9);

This will never generate the digit 9.

If you want to be generating digits 0 through 9 inclusive (that is 10 different digits) you need to do:

int temp = randy.nextInt(10);

If you want to exclude 0 (which, in this case may make sense....):

int temp = randy.nextInt(9) + 1;
share|improve this answer
    
ouch, i really miss-look at this! thanks. 0 is included, so i'll use nextInt(10) –  Immer Allein Jan 24 at 7:12
    
You also don't need a loop. You can put the digits 0-9 into a list, shuffle the list, and then take the first four entries from the list. –  David Conrad Jan 27 at 16:49

Less focused on functionality and more on nomenclature: I wouldn't call your new Random() the name randy. While the name is pretty clear in what it does, the word randy also has a more immature meaning. You never know who will see your source code, so if any of your customers ever sees code with somewhat immature variable names or comments, they could think your immaturity extends to functionality, even if it doesn't.

So instead of

Random randy = new Random();

write

Random rand = new Random();

In general, avoid using immature words, the names of coworkers, companies or products or words in foreign languages, except if the word you want to use is relevant to what the variable is intended for. EPAComplianceCheck() is acceptable. ScrewYouGinaMcCarthy() less so.

share|improve this answer
    
LOL. Okay, actually I found this funny too. I just forgot to change all those variables name before posting. thanks anyway! –  Immer Allein Jan 24 at 11:45
4  
It's amazing how many little jokes people put in code then forget to take out...until someone who shouldn't see them does see them. –  Tim B Jan 24 at 13:17
1  
+1 for mentioning this. This is how I lost a $40/hr contract before. –  Thebluefish Jan 24 at 16:42

In getGuess(),

Integer.parseInt(String.valueOf(input.charAt(i)))

could be written as Character.digit(input.charAt(i), 10).

In getGuess(), recursing on validation failure is inappropriate. (Hold down the Enter key to overflow the stack!) Use a loop instead:

public static int[] getGuess(){
    Scanner keyboard = new Scanner(System.in);
    do {
        System.out.print("Please enter your guess: ");
        String input = keyboard.nextLine();
        if (input.matches("\\d{4}")) break;
        System.out.println("Invalid number. You must enter 4 digits between 0-9 only.");
    } while (true);
    int[] guess = new int[4];
    for (int i = 0; i < guess.length; i++) {
        guess[i] = Character.digit(input.charAt(i), 10);
    }
    return guess;
}

In numberGenerator(), change your while-loop to a do-while.

share|improve this answer
    
cool. Character.digit() is a new thing for me. And about the while, urmm..yeah I guess you're right –  Immer Allein Jan 24 at 7:34
    
How to validate the user input using do-while? I thought recursion is the simple one. –  Immer Allein Jan 24 at 8:30
2  
@RafaEl conceptually it's do{prompt();input=getInput();while(!isValid(input)); –  ratchet freak Jan 24 at 8:44
    
okay that was really make sense. and thanks for the regex ("\\d{4}") much more shorter. –  Immer Allein Jan 24 at 8:53
1  
@Tibos I don't know of any implementation of Java that performs tail call optimization. –  200_success Jan 24 at 15:47

First off:

public class Game {

Really? What game is this? Is this like, tetris or something? Make it something like CodeGuessGame, and add a javadoc comment to the class explaining exactly what it is, and what it does.

System.out.println("The computer has generate a unique 4 digit number.\n"
                 + "You can try to guess the 4 digits number in 5 attempts.\n");

Besides spelling errors, you shouldn't have the \n at the end of that line, because you are using a println. Personally, I always put my \ns at the beginning of each continuation line, but that isn't completely nessisary.

        System.out.println("_______________________________________________________\n");

Again, extra \n. Personally I would suggest using hyphens (-) or equals signs (=) instead, because those are more common for creating horizontal rules. Underscores suggest a blank that the user is supposed to type into.

        int[] random = numberGenerator();
        int maxTry = 5;
        int indexMatch = 0;
        int match = 0;

You should combine the declarations for maxTry, indexMatch, and match into one statement, for conciseness.

        while(maxTry>0 && indexMatch!=4){
            int[] guess=getGuess();
            indexMatch=0;
            match=0;
            for(int i=0;i<guess.length;i++){
                if(guess[i]==random[i]){
                    indexMatch++;
                }
                else if(guess[i]==random[0] || guess[i]==random[1] || guess[i]==random[2] || guess[i]==random[3]){
                    match++;
                }
            }
            if(indexMatch==4){
                System.out.print("Well done! Your guess is Correct! The number is: ");
                for(int i=0;i<guess.length;i++){
                    System.out.print(guess[i]);
                }
            }
            else{
                maxTry--;
                if(maxTry>1){
                    System.out.println("You have guess "+indexMatch+" correct number in correct position,"+
                    " and "+match+" correct number in incorrect position. \n"+maxTry+" attempt remaining.");
                }
                else if(maxTry==1){
                    System.out.println("You have guess "+indexMatch+" correct number in correct position,"+
                    " and "+match+" correct number in incorrect position. \nLast attempt!. Good luck");
                }
                else{
                    System.out.println("Sorry, you failed to guess the number in 5 attempts.");
                    System.out.print("The number is: ");
                    for(int i=0;i<random.length;i++){
                        System.out.print(random[i]);
                }
            }
        }
    }   
}

Whut us thish shpaghetti?? Please, learn to use System.out.format and ternary operations (condition?valueIfTrue:valueIfFalse).

At this point, I can no longer bear to read your code any more. Please, just write a new program, and write it with configurability and reusability in mind this time.

The game should be run as separate instances. The main method should not contain any logic having to do with the actual game. All it should do is (1) instintantiate Game, (2) set configurable options, and (3) call an instance method of Game (called something like play) to run the game.

Ideally, your main method should end up looking like this:

public static void main(String [] args){
    new MastermindGame()
        .setLanes(4)
        .setColors("0", "1", "2", "3", "4", "5", "6", "7", "8", "9")
        .setGuesses(5)
        .setStartFlavor("The computer has generated a 4 digit number."
                   + "\nYou must guess this 4 digit number in 5 attempt(s).")
        .setPrompt("Enter guess:")
        .setResponse("Correct digit in correct position count = %1"
                 + "\nCorrect digit in incorrect position count = %2"
                 + "\nGuesses remaining = %3")
        .setLose("Sorry, you failed to guess the number (%1) in 5 attempts.");
        .setWin("Congratulations, you win!");
        .play();
}
share|improve this answer
    
I defiantly wouldn't recommend that main method. That requires all methods returning void to return this instead. Plus debugging that will be a chore. I would consider that to be very bad practice. –  sixtyfootersdude Jan 24 at 19:34
    
Thought about this a bit more and maybe it is ok. The term seems to be a "fluent interface" or "method chaining". Dealing with stack traces from this kind of thing is going to be messy but it is somewhat interesting. See this question for a further discussion of them: stackoverflow.com/questions/1345001/… –  sixtyfootersdude Jan 24 at 19:47
1  
That won't affect stack traces, and the only thing messy about debugging it is that you need breakpoints on subexpressions (or just put them on the functions themselves instead of the call site). –  Ben Voigt Jan 24 at 19:54

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.