7
\$\begingroup\$

This was a learning experience. I have looked around and found some better ways to write some things but wanted a second opinion to look at what I've written and kind of analyze it as well.

I just finished my first "Text based game" sort of deal. I believe that it's poorly written and is pretty long for the short amount of game there is. I used a lot of unnecessary code such as pausing in between strings and such, although that's not what I'm asking.

I'm asking if you could find anything that can be done either more simply or more efficient.

The game goal is to basically guess a number and if you get it right, you win. As the levels progress the random numbers to guess and the ranges get higher, thus giving more points.

I tried to comment as much as I could for anyone to read. There are no errors with this game. It runs and works.

This is all 1 file. Here is a Pastebin to the code in case you want it.

import java.util.Scanner;
import java.util.Random;
public class GuessingGame {
//First text based game.  Guess the random number that will be generated, and gather points! 
    public static void main(String[] args) {

    try (Scanner store = new Scanner(System.in)){  //This < is used to create a new scanner variable . Used the try method because Eclipse was giving a 'resource leak' 
    Random rnd = new Random();


    //Only 4 rounds so 4 random number ints. 
    int guess;   //Will store the number that you will guess.
    int totalpoints = 0;
    char readytoplay;
    int randomnumber1 = rnd.nextInt(1)+1;      //First level of random numbers
    int randomnumber2 = rnd.nextInt(2)+1;
    int randomnumber3 = rnd.nextInt(4)+1;
    int randomnumber4 = rnd.nextInt(9)+1;


    System.out.println("Welcome to Guessing game!");
    System.out.println("In this game, you will guess a number to see how lucky you are.");
    System.out.println("As you progress, it gets harder and harder. ");
    System.out.println("You will gain points as you go. ");
    System.out.println("Are you ready to play? Y/N");
    readytoplay = store.next().charAt(0);          //This will store a CHAR
    System.out.println("You chose:  " + readytoplay);

    while(true){
        if(readytoplay == 'y' ||readytoplay == 'Y'){ //Possible response types for Yes. 
            System.out.println("Okay! Great! Lets get started.");
            break;
        }
        else if(readytoplay == 'n' ||readytoplay == 'N'){ //Possible response types for No.
            System.out.println("If you don't want to play, please exit.");
            readytoplay = store.next().charAt(0);
            break;
        }
        else{
            System.out.println("You must answer EITHER (Y)es or (n)o"); //Letting them know they didn't answer either Y, y, N, n  so it was a invalid response. 
            readytoplay = store.next().charAt(0); // This will store their answer for them to re submit a valid response.

        continue;

        }
    }
        System.out.println("Okay. So let's explain how this works. ");
        try {                                        // This will delay the Strings being typed. 1500 = 1.5 secs. 
            Thread.sleep(900);                      //
        }                                           //   I added these to make the text easy to read for the user
        catch (InterruptedException ex)              //  To not spam them with everything to read all at once. 
        {                                             //   Not sure of an easier way to do this so maybe a TODO: getting a easier method..
                                                        // 
        }                                                //
        System.out.println("You will be given a 'Range' of numbers to guess for the answer.");
        try {
            Thread.sleep(900);
        }
        catch (InterruptedException ex)
        {

        }
        System.out.println("FOR EXAMPLE:");
        try {
            Thread.sleep(900);
        }
        catch (InterruptedException ex)
        {

        }
        System.out.println("1-3");
        try {
            Thread.sleep(900);
        }
        catch (InterruptedException ex)
        {

        }
        System.out.println("You can guess either 1, 2, or 3.");
        try {
            Thread.sleep(900);
        }
        catch (InterruptedException ex)
        {

        }
        System.out.println("The machine will generate a random number between either of those, if yours is correct, you advance!");
        try {
            Thread.sleep(900);
        }
        catch (InterruptedException ex)
        {

        }
        System.out.println("Let's try this out.");
        try {
            Thread.sleep(900);
        }
        catch (InterruptedException ex)
        {

    }
        System.out.println("The first set is 1-2. Pick 1, or 2.");
        guess = store.nextInt();


                    //ROUND 1
            if (guess == randomnumber1)     {
            System.out.println("You were correct.");
            System.out.println("You guessed "+guess);
            System.out.println("The random number was "+randomnumber1);
            totalpoints = +1;                                       //This will add points to their total points. 
            System.out.println("You gained 1 point, Your total points are now: " +totalpoints);       //Obviously letting them know how many points they now have. 

            }
            else {
            System.out.println("You didn't guess correctly.");

            System.out.println("Your number:" +guess);
            System.out.println("The random number:" +randomnumber1);
            System.exit(0);     //Exiting the game, they did not have any points at the time so not displaying any points . 
            }
    {

                //ROUND 2 
            System.out.println("Good job , by the way! You got lucky! Literally!");
            try {                                     //
                Thread.sleep(1000);                   //
            }                                         // 
            catch (InterruptedException ex)          //Pausing for a second to not spam.. Letting them know round 2 is starting.
            {                                         // 
                                                         //
            }                                            //
            System.out.println("Next round. Your numbers are 1-3. 1,2, or 3 ");
            System.out.println("Pick your number:");
            guess = store.nextInt();

            if (guess == randomnumber2) {
                System.out.println("You were correct. ");
                System.out.println("You guessed "+guess);
                System.out.println("The random number was " +randomnumber2);
                totalpoints = +2;
                System.out.println("You gain 2 points for that one! you now have: " +totalpoints+ "points!");
            }
                else {
                    System.out.println("You were not correct, sorry..");
                    System.out.println("You guessed: " +guess);
                    System.out.println("The random number was " +randomnumber2);
                    System.out.println("Game over. You ended with: " +totalpoints+ " point");
                    System.exit(0);
                }
            try {                                    //
                Thread.sleep(1000);                    // 
            }                                        //
            catch (InterruptedException ex)          //Pausing for a second to not spam.. Letting them know round 3 is starting.
            {                                        //

            }                                        //

                    //ROUND 3
            System.out.println("Round 3.  You will need to guess 1-5. Good luck on this one...");
            System.out.println("Guess now. 1,2,3,4, or 5.");
            guess = store.nextInt();

            if (guess == randomnumber3) {
                System.out.println("You were correct. ");
                System.out.println("You guessed "+guess);
                System.out.println("The random number was " +randomnumber3);
                totalpoints = +3;
                System.out.println("You gain 3 points for that one! you now have: " +totalpoints+ " points!");
            }
                else {
                    System.out.println("You were not correct, sorry..");
                    System.out.println("You guessed: " +guess);
                    System.out.println("The random number was " +randomnumber3);
                    System.out.println("Game over. You ended with: " +totalpoints+ " point");
                    System.exit(0);
                }
            try {                                   //
                Thread.sleep(1000);                     //
            }
            catch (InterruptedException ex)          //Pausing for a second to not spam.. Letting them know round 4 is starting.
            {                                       //
                                                    //
            }                                       //

                                        //ROUND 4
            System.out.println("You are really lucky. Maybe you should play the lottery");
            System.out.println("Although, I believe this will be a little more difficult.");
            System.out.println("You will be given a random number 1-10. 1,2,3,4,5,6,7,8,9, or 10.");
            System.out.println("You have a 10% chance. Goodluck.);");
            System.out.println("Guess now:");
            guess = store.nextInt();

            if (guess == randomnumber4) {
                System.out.println("You were correct. ");
                System.out.println("You guessed " +guess);
                System.out.println("The random number was " +randomnumber4);
                totalpoints = + 4;
                System.out.println("You gain 4 points. You now have: " +totalpoints+ "points!");

            }
            else {
                System.out.println("You were wrong. You guessed " +guess);
                System.out.println("The random number was: " +randomnumber4);
                System.out.println("You ended the game with " +totalpoints+ "points.");
                System.exit(0);
            }
            try {                                   //
                Thread.sleep(500);                  //
            }                                       //
            catch (InterruptedException ex)          //Pausing for a second to not spam.. 
            {                                       //
                                                    //
            }                                       //
            System.out.println("CONGRATULATIONS! You beat all the odds, somehow..");
            System.out.println("You ended the game with: " +totalpoints);
            System.exit(0);

    }
    }           
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review! I hope you get some great answers! \$\endgroup\$ – Phrancis Jan 22 '16 at 20:10
7
\$\begingroup\$

I would try to split your code into multiple methods to make things easier to understand. Having everything in one huge main method is difficult to read and difficult to maintain, and generally considered a bad practice.

Having multiple methods also allows you to reuse blocks of code - one of the main principles of software development is "Don't repeat yourself."

Let's take a look at one of your main pieces of logic: checking their guess.

        if (guess == randomnumber1)     {
        System.out.println("You were correct.");
        System.out.println("You guessed "+guess);
        System.out.println("The random number was "+randomnumber1);
        totalpoints = +1;                                       //This will add points to their total points. 
        System.out.println("You gained 1 point, Your total points are now: " +totalpoints);       //Obviously letting them know how many points they now have. 

        }
        else {
        System.out.println("You didn't guess correctly.");

        System.out.println("Your number:" +guess);
        System.out.println("The random number:" +randomnumber1);
        System.exit(0);     //Exiting the game, they did not have any points at the time so not displaying any points . 
        }

Side note: you should aim to indent each block of logic deeper than the level above it. Most editors will do this for you with a keyboard shortcut. E.g.

        if (guess == randomnumber1) {
             System.out.println("You were correct.");
             ...

Anyway, looking at the above, the code checks the value, then if it is correct, prints a few lines and adds to your score, and if not, prints a few other lines. Compare this to the code in Round 2 (starting around line 138), and Round 3 (line 165), and Round 4 (195). Look familiar? They're practically identical. By refactoring these into one method and calling it several times, you gain several benefits:

  • your code will be more succinct and easier to read
  • if you have to update the code, you only have to update it once instead of four times
  • if you want to expand it to add a 5th, 6th, ...nth round, it will take much less time and effort.

Here's an example:

private int validateGuess(int currentScore, int pointsToAdd, int guess, int correctValue){
    if (guess == correctValue)     {
        System.out.println("You were correct.");
        System.out.println("You guessed "+guess);
        System.out.println("The random number was "+correctValue);
        int totalpoints = currentScore+pointsToAdd;    

        System.out.println("You gained "+pointsToAdd+" point(s), Your total points are now: " +totalpoints);       

    }
    else {
        System.out.println("You didn't guess correctly.");
        System.out.println("You guessed:" +guess);
        System.out.println("The random number was:" + correctValue);
        System.out.println("Game over. You ended with: " + currentScore + " point(s)");
        System.exit(0);    
    }
}

Now, you just need to call your method everywhere you had that code block before, e.g.

totalPoints = validateGuess(totalPoints, 1, guess, randomNumber1);
...
totalPoints = validateGuess(currentScore, 2, guess, randomNumber2);
...

Regarding all the Thread.sleep blocks, I would at least try to to compress them so they aren't so verbose:

    try {
       Thread.sleep(900);
    }
    catch (InterruptedException ex)
    {  //Ignoring exception since this is just pauses console output      }

Some people would suggest even putting the try on one line, it all depends on style preference. Also note that in general, an empty exception block is a bad practice (you should either handle the exception or throw it), but at the minimum you should at least comment any situations where you aren't handling an exception (which I think is fine in this situation given the context of your program and the exception).

Lastly, just as you could refactor your common output code to a method, you can probably use a loop to minimize the repetition of your input code, making it easier to read and extend. It could be something as simple as:

int numRounds = 4;
for(int i=0, i<numRounds, i++){
   // Output your prompt for a guess
   // read in guess from input console
   // call validateGuess
}

Since the value range for the guess changes each round, you might need to add some logic for that.

Some other tips:

  • A common recommendation is to write the guess validation statement as (randomNumber1 == guess). That way if you accidentally remove an '=', you don't assign "guess" the value of "randomNumber". But what about the danger now of assigning "randomNumber" the value of "guess"? There's a solution for that:
  • Make your randomNumbers final. Then you won't risk accidentally changing their values: final int randomnumber1 = rnd.nextInt(1)+1;

That's all for now. Good luck!

\$\endgroup\$
  • 2
    \$\begingroup\$ Welcome to Code Review! Good job on your first answer \$\endgroup\$ – SirPython Jan 22 '16 at 22:03
4
\$\begingroup\$

Before I go into details, let me say that your current code is quite good overall!


The first remark that I have is that all of your code is inside the main method, which makes reading a bit difficult. It is better to factor code into small methods: the name of the method gives a clear intent as to what the code is doing. For example, your game can be split into multiple distinct parts:

  • Print general information
  • Ask the user if they want to play and proceed
  • Print instructions
  • Make round 1, then 2, then 3 and 4.

Each of those parts can be split into dedicated methods to make understanding the code easier. It is easier because each method will be shorter and will have a clear intent.

The second remark is that you have duplicated code: that is to say, the same logic written at different places. Code duplication is something you want to avoid. The reason is the following: let's say you have the same logic written at two differents places of your program; one day, you will want to modify that logic because of new requirements and when that happens, you will have to remember to modify the two places where that logic was written. And, unfortunately, you might forget to modify each of the places where that logic is. This is solved by factoring out that logic into a single method and then use that method elsewhere. The trick is that, by doing that, when that logic needs to be updated, you only need to modify that particular method and not search through all of your code.

In this case, the duplication comes from the "round" methods. To see how we could factor that into a common method, we first need to analyze what a round is:

  • It has a target number: this is the number that the player has to guess.
  • It will ask the user for a number.
  • If the guess was correct, it will print out information to the user that their guess was correct.
  • If not, the program will terminate, it will print what was the number to guess.
  • This method will return whether or not the guess was correct: this enable the rest of code to decide what to do based on a correct guess or not.

As you can see, this can be written as a common method that would take 2 parameters: the scanner object to retrieve the user guess and the target number. If you want to see a example of implementation, refer to the end of this post.

There is also duplication in your way to wait for printing messages. Instead of calling Thread.sleep(millis); (wrapped in try-catch) each time, make a method that does it: it would take as parameter the number of milliseconds to wait and handle the potential InterruptedException.


Now, on to the existing problems of your code:

  • rnd.nextInt(n) returns a random integer between 0 and n exclusive. This means that the random integer is selected from 0 to n - 1 include. As such, rnd.nextInt(5) does not return a random integer between 0 and 5. It will return a random integer that will be 0, 1, 2, 3 or 4 (with equal probability). So when your program does rnd.nextInt(1) + 1 for the first round, it means the number returned will actually always be 0 + 1 = 1!
  • You are incrementing the total points of the player with the following code: totalpoints = + 4; (for example). This code actually assigns 4 to the variable totalpoints, it does not increment it. Putting unnecessary parentheses, the code actually is totalpoints = (+ 4); which is equivalent to totalpoints = 4;. What you want instead is to use the += operator, which will increment the variable by the given amount. The code would be totalpoints += 4;.
  • You are correctly using a try-with-resources statement to handle the automatic closure of the Scanner object, this is good! Resource leaks means that you acquire a handle to a resource (like opening a file) but you never close it. This can lead to unexpected problems. In this case, with the try-with-resources statement, the resource is guaranteed to be closed when the application finishes. However, you are exiting the application with System.exit(0), and in that case, it actually becomes useless: the resource won't be closed. As such, instead of exiting the application with System.exit, consider restructing a little your code: you can just return nothing instead.

To point you to how to refactor your code with methods, I will consider the example of waiting before printing a message. Since you might want to wait for a different amount of milliseconds each time, this method should take this number as parameter. Since all it does is wait, it isn't supposed to return something, so we will declare it void. Its purpose will be to wait for an amount of time so a good name for it would be waitFor. A simple implementation would be the following:

private static void waitFor(int millis) {
    try {
        Thread.sleep(millis);
    } catch (InterruptedException ex) {
        ex.printStackTrace();
    }
}

In case of an exception, it is just written to the console and the rest of the code continues.

So to create a method, what you need is:

  • A good name that describe what it does.
  • Define what it should take as parameter.
  • Define what it should return.
  • Make it small, concise, with the possibility of being reused through your code.

I think all the tips I gave you should be enough to improve your code!


Sample code of the round method:

private static boolean round(Scanner store, int randomnumber) {
    System.out.println("Guess now: ");
    int guess = store.nextInt();
    if (guess == randomnumber) {
        System.out.println("You were correct.");
        System.out.println("You guessed " + guess);
        System.out.println("The random number was " + randomnumber);
        return true;
    } else {
        System.out.println("You didn't guess correctly.");
        System.out.println("Your number:" + guess);
        System.out.println("The random number:" + randomnumber);
        return false;
    }
}
\$\endgroup\$
1
\$\begingroup\$

Since nobody else has said it, don't declare all your variables at once.

It may feel like annoucing 'I will be using these variables' is a good idea, but by the time someone has started reading all the other code and made it to half way down the page they're going to start thinking "wait, what was that 4th variable again?".

It's much better to keep variables local to the area where they're being used. Variables have scope for a reason. A variable should only be introduced when it's needed and it should be forgotten about when it is no longer needed. This serves two main purposes.

From a compiler's point of view, this lets the compiler 'cheat' by reallocating the memory used for a variable that is no longer needed for a different variable. If it knows two variables will never exist at the same time there is nothing to stop them sharing a chunk of memory.

From a programmer's point of view, only having to remember the existance of a variable around the time it is being used means one less thing to remember, thus reducing the programmer's cognitive load. The less effort a programmer has to spend remembering various aspects of a problem, the more effort they can spend actually working on the problem.

This also applies to what @GJCode said about breaking the code up into functions. By breaking the program up into smaller pieces, you can focus on one small piece at a time, which makes code both easier to write and easier to read. Think of it like a production line. Each person/machine in a production line does a single job and does it well. When different people/machines with different jobs are chained together, they form a larger process. In programming the functions are the people/machines and the program is the larger process.

\$\endgroup\$
  • \$\begingroup\$ The "don't declare your variables all at once" should be a non-issue if you're using methods / functions to break the program up. Plus a main method that has as least amount of code as possible. Then, of course, you'll be able to see the variable declarations and the code all in one page ;) \$\endgroup\$ – Insane Jan 23 '16 at 4:54
  • \$\begingroup\$ @Insane Using functions has no bearing on where the variables are. There will still be variables. Perhaps there will be less variables but they will still exist. Also 'one page' is relative. \$\endgroup\$ – Pharap Jan 23 '16 at 6:24
  • \$\begingroup\$ True, but not using functions will result in more variables declared at the same time and more code below them, was my point \$\endgroup\$ – Insane Jan 23 '16 at 6:26
  • 1
    \$\begingroup\$ @Insane I grant you 'more variables' and 'more code' being highly probable, it's the 'at the same time' bit I have issues with. \$\endgroup\$ – Pharap Jan 23 '16 at 6:43
0
\$\begingroup\$
int randomnumber1 = rnd.nextInt(1)+1;      //First level of random numbers
int randomnumber2 = rnd.nextInt(2)+1;
int randomnumber3 = rnd.nextInt(4)+1;
int randomnumber4 = rnd.nextInt(9)+1;

Consider using an array.

    System.out.println("You will be given a 'Range' of numbers to guess for the answer.");
    try {
        Thread.sleep(900);
    }
    catch (InterruptedException ex)
    {

Try using a for loop or an array that prints each message and then sleeps 900, since the code you currently have is redundant.

    try {
        Thread.sleep(900);
    }
    catch (InterruptedException ex)
    {

    }

My own preference for code like this would be to one-line it:

try {Thread.sleep(900);} catch (InterruptedException ex) {}

but I may be in the minority here.

\$\endgroup\$
  • \$\begingroup\$ You pointed out some pretty interesting things . I understand creating an array would be easier and I would've done that but I don't understand how can you create an array if each int has a different value of a random number? \$\endgroup\$ – LifezJoke Jan 22 '16 at 20:58
  • \$\begingroup\$ Something like for i ({1,2,4,9}) (arr[] = rnd.nextInt(i)+1;} (this is psuedo-code, not Java, but hopefully helps) \$\endgroup\$ – Barry Carter Jan 22 '16 at 21:04
  • \$\begingroup\$ @LifezJoke Arrays aren't read-only, you can write to them too. int[] randoms = new int[4]; randoms[0] = rnd.nextInt(1)+1; randoms[1] = rnd.nextInt(2)+2; ... \$\endgroup\$ – Pharap Jan 23 '16 at 2:32

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.