6
\$\begingroup\$

enter image description here

Also posted at rgcsm.info/blog

//Number Guessing Game in Java Swing
//Also posted at 

import javax.swing.*;
import java.awt.*;
import java.awt.event.*;

class Guess extends JFrame
{
    JTextField t1, t2, t3, t4;
    JLabel j4; 
    ButtonListener bl1;
    ButtonListener2 bl2;
    ButtonListener3 bl3;    

    //setting random number in rand variable
    int rand=(int) (Math.random()*100); 
    int count=0;

    public Guess()
    {
        //Get the container
        Container c = getContentPane();

        //Set absolute layout        
        c.setLayout(null);   

        //Set Background Color
        c.setBackground(Color.WHITE); 

        //Creating image 
        JLabel lblpic = new JLabel("");
        lblpic.setIcon(new ImageIcon("images.png"));
        lblpic.setBounds(0,0,500,350);

        //Creating label Guess my number text
        JLabel j=new JLabel("Guess my number game");
        j.setForeground(Color.RED);
        j.setFont(new Font("tunga",Font.BOLD,24));
        j.setSize(270,20);
        j.setLocation(300,35);

        //Creating label Enter a number..... 
        JLabel j1=new JLabel("Enter a number b/w 1-100");
        j1.setFont(new Font("tunga",Font.PLAIN,17));
        j1.setSize(270,20);
        j1.setLocation(300,60);

        //Creating TextField for input guess
        t1=new JTextField(10);
        t1.setSize(50,30);
        t1.setLocation(350,80);

        //Creating Label for Display message        
        j4=new JLabel("Try and guess my number");
        j4.setFont(new Font("tunga",Font.PLAIN,17));
        j4.setSize(270,20);
        j4.setLocation(290,130);

        //Creating Text field for best score
        t2=new JTextField(10);
        t2.setSize(40,20);
        t2.setLocation(10,10);        

        //Creating Label for best score
        JLabel j5=new JLabel("Best Score");
        j5.setFont(new Font("tunga",Font.PLAIN,17));
        j5.setSize(270,20);
        j5.setLocation(60,10);

        //Creating guess text field
        t3=new JTextField(10);
        t3.setSize(40,20);
        t3.setLocation(160,10);

        //Creating guess Label
        JLabel j6=new JLabel("Guesses");
        j6.setFont(new Font("tunga",Font.PLAIN,17));
        j6.setSize(270,20);
        j6.setLocation(210,10);

        //creating 3 buttons
        JButton b1=new JButton("Guess");
        b1.setSize(150,40);
        b1.setLocation(260,250);
        bl1=new ButtonListener();        
        b1.addActionListener(bl1);


        JButton b2=new JButton("Give up!");
        b2.setSize(100,30);
        b2.setLocation(180,200);
        bl2=new ButtonListener2();
        b2.addActionListener(bl2);        

        JButton b3=new JButton("Play Again");    
        b3.setSize(120,30);
        b3.setLocation(330,200);    
        bl3=new ButtonListener3();        
        b3.addActionListener(bl3);


        //Place the components in the pane
        c.add(j5);    
        c.add(j4);    
        c.add(lblpic);
        c.add(j);    
        c.add(j1);
        c.add(t1);
        c.add(t2);
        c.add(t3);
        c.add(b1);    
        c.add(b2);
        c.add(b3);        
        c.add(j6);     

        //Changing TextFields to UnEditable
        t2.setEditable(false);
        t3.setEditable(false);    

        //Set the title of the window
        setTitle("GUESS MY NUMBER");        

        //Set the size of the window and display it
        setSize(550,350);
        setVisible(true);
        setDefaultCloseOperation(EXIT_ON_CLOSE);
    }

    private class ButtonListener implements ActionListener
    {
        int bestScore=100;
        public void actionPerformed(ActionEvent e)
        {
            int a = Integer.parseInt(t1.getText());
            count=count+1;
            if(a<rand)
            {
                j4.setText(a+" is too low, try again!!");
            }    
            else if(a>rand)
            {
                j4.setText(a+" is too high, try again!!");
            }
            else
            {
                j4.setText("CORRECT, YOU WIN!!!!");    
                if(count<bestScore)
                {    
                    bestScore=count;
                    t2.setText(bestScore+"");
                }
                else
                    t2.setText(""+bestScore);
                t1.setEditable(false);
            }

            //setting focus to input guess text field
            t1.requestFocus();
            t1.selectAll();

            t3.setText(count+"");
        }
    }

    private class ButtonListener2 implements ActionListener
    {
        public void actionPerformed(ActionEvent e)
        {
            t3.setText("");
            j4.setText(rand+" is the answer!");
            t1.setText("");
            t1.setEditable(false);
        }
    }        

    private class ButtonListener3 implements ActionListener
    {
        public void actionPerformed(ActionEvent e)
        {
            rand=(int) (Math.random()*100);
            t1.setText("");
            t3.setText("");
            j4.setText("Try and guess my number");
            t3.setText("");
            count=0;
            t1.setEditable(true);    
            t1.requestFocus();
        }
    }

    public static void main(String[] args)
    {
        new Guess();
    }
}
\$\endgroup\$
2
  • \$\begingroup\$ Welcome to Code Review! Because links tend to go away over time, please also copy the description from your blog to this post itself. \$\endgroup\$ Commented May 3, 2014 at 12:31
  • \$\begingroup\$ Seems like someone needs to read about the EDT. Your code create and mutates Swing components off of the EDT, this is in complete violation of Swings threading policy. \$\endgroup\$ Commented May 3, 2014 at 18:07

4 Answers 4

7
\$\begingroup\$

Layoutmanagers

You're using absolute positioning for all your elements which won't scale well when you enlarge or shrink the application. For this reason you should use a layoutmanager that will handle the resizing for you (and it's a lot easier to extend your application with new elements).

For more information I'll refer you to the documentation.

Comments

//Set the title of the window
setTitle("GUESS MY NUMBER"); 

The comment doesn't tell me anything I don't know yet. Comments should explain why I do something, not what I'm doing.

Naming

Your class is named Guess, which isn't very descriptive of what it means. GuessingGame would be a lot better and clarifies that it is a game we're talking about.

Spacing

Add a space before and after your binary operators like *, &&, =, etc. Right now the code gives a very compressed feeling.

Braces

I see this line of code:

else
    t2.setText(""+bestScore);
t1.setEditable(false);

Always use braces so I can distinguish easily between what should be inside that if statement. Right now I can't tell for sure if that is intended or if it is a logical error. It will also prevent you from introducing errors when you add an expression to your else body without thinking about the braces.

If anything, it's also a matter of staying consistent.

Main

I prefer to put my main method in a separate class so I always have instant knowledge of where my application starts without having to dig into my separate classes. It's also more comfortable to work with when you have to setup certain things like the "Look and Feel" or logging.

\$\endgroup\$
1
  • \$\begingroup\$ Man, I had to scroll all the way down to find an answer mentioning the absolute positioning. As someone who has to maintain several legacy GUIs with enormous complexity, this is one of the most terrible things I have to deal with and I find myself rewriting one class after another to get rid of it. \$\endgroup\$
    – Ingo Bürk
    Commented May 3, 2014 at 21:06
11
\$\begingroup\$

Vogel612's answer already covered naming. I'd like to highlight some other things: some because of how Swing works, and some about initialisation.

Initialisation

Initialisation and reset take mostly the same actions but are in two different places: ButtonListener3.actionPerformed and Guess.<init>. Try taking these actions out and creating methods that clarify their intent:

private static final int MAX_NUMBER = 100;
public void start() {
    rand = (int) (Math.random() * MAX_NUMBER) + 1;
    count = 0;
    // set labels here
}

public void restart() {
    // any cleanup goes here
    start();
}

Keep it on the EDT

Swing is a multi-threaded environment, and any manipulation of GUI elements should happen on the Event Dispatch Thread (EDT). For most of an application's lifetime, this is not a direct concern because it primarily runs inside this EDT, but bootstrapping is a special case and requires your intervention.

public static void main(String[] args)
{
    SwingUtilities.invokeLater(
        new Runnable() {
            public void run() {
                Guess guess = new Guess();
                guess.start();
            }
        }
    };
}

Or, since Java 8:

public static void main(String[] args)
{
    SwingUtilities.invokeLater(
        () -> {
            Guess guess = new Guess();
            guess.start();
        }
    );
}

Yes, even running a single method or constructor requires this, and you may get away with it most of the time but, as I learned to my shame, not all of the time. ;)


Layout and Resizing

Not having a layout manager makes it easier to have components right where you want them, but it makes your container ignore resizing. If you've already picked the (only) size you're going to support, you may want to constrain it:

setMinimumSize(getSize());

Randoms in Java

Math.random() returns a value \$\in [0,1[\$, so if you want an integer between min and max, inclusive, you'll need to change a bit:

rand = (int) (Math.random() * (max - min + 1)) + min;

...which, for 1..100 would make:

rand = (int) (Math.random() * 100) + 1;

Because I got 0 on my first run of your game. :p


Seperation of Concerns

Your GUI and your game are intimately connected. Separation could help you maintain your code and make it more robust to changes.

The guessing part can be extracted into a class of its own, which will also take care of your magic numbers:

class GuessingGame {
    int value;
    int tries;
    int topScore = Integer.MAX_VALUE;

    Random random = new Random();

    public void reset() {
        tries = 0;
        value = random.nextInt(getMaximum() - getMinimum() + 1) + getMinimum();
    }

    public Result guess(int guess) {
        tries++;

        if ( guess == value ) {
            if ( topScore > tries ) {
                topScore = tries;
            }
            return Result.EXACT;
        }

        return guess < value ? Result.TOO_HIGH : Result.TOO_LOW;
    }

    public int getNumberOfTries() {
        return tries;
    }

    public int getTopScore() {
        return topScore;
    }

    public int getMinimum() { return 1; }
    public int getMaximum() { return 100; }
}

enum Result {
    TOO_HIGH, TOO_LOW, EXACT;
}
\$\endgroup\$
0
7
\$\begingroup\$

Naming:

You're naming your things terribly:

JTextField t1, t2, t3, t4;
JLabel j4;
ButtonListener bl1;
ButtonListener2 bl2;
ButtonListener3 bl3;

Better would definitely be:

JTextField guessInput, bestScore, guessText; 

You should already be able to see: t4 was unused, guessInput (taken from the comments at initialization) and guessText (also take from the comment at init) are somewhat doubled.

ButtonListener, ButtonListener2, ButtonListener3

Wait what??? you have 3 different action listeners and call them ButtonListener 1, 2 and 3?
Shame on you. Better names would be:

GuessSubmittedListener
SurrenderListener
NewGameListener

Move this thorough your whole code.

Starting the game:

public static void main(String[] args)
{
    new Guess();
}

Conventionally you'd have something like the following instead:

public static void main(String[] args){
    Guess game = new Guess();
    game.start(); // game.run(); game.begin(); game.whatever();
}

The constructor of game is supposed to initialize it. But it never should be responsible for starting the game.

Braces:

The convention for Java is "egyptian braces". The opening curly brace is on the line with the block expression, the closing brace is separate. You are using the C# conventions...

Magic Numbers:

Your whole code is littered with magic numbers, numbers that have no explanation whatsoever. Move these numbers to constants:

int rand = (int) (Math.random()*100);

Nonononon...

private static final int RANDOM_BORDER = 101;

Random rnd = new Random();
int randNumber = rnd.nextInt(MAX_NUMBER);

Same goes for the whole initialization stuff... There's so many magic numbers there I'm almost having a breakdown right now.

\$\endgroup\$
2
  • 1
    \$\begingroup\$ (int) (Math.random()*100); does not do the same as rnd.nextInt(101);. The first does not include the possibility for the number 100, the second does. Other than that, +1. \$\endgroup\$ Commented May 3, 2014 at 12:28
  • \$\begingroup\$ You don't mention that lack of Swing threading. This is the biggest issue. The game runs itself on the main thread. This is a massive bug waiting to happen. Otherwise +1 \$\endgroup\$ Commented May 3, 2014 at 18:06
2
\$\begingroup\$

"Give up!" and "Play again!" should be the same button. If you've just won the game, it should be "Play again!", and if you tried and failed and want to reset the game, it should be "Give Up!". If you don't want to change the label every time, "Reset" will work for both scenarios.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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