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 have created this program and it works fine. It's just that its too wonky and huge so I was wondering if anyone knew how I could shorten the program so I didn't have to put all of these into each button

if(guess == number){
    l1.setText("You have won the number was " + number);
}
if(guess > number){
    l1.setText("Too high");
}
if(guess < number){
    l1.setText("Too low");
}

Here is the full program

public class Main {
    public static int number, guess;
    public static Random rand;
    public static Scanner scan;
    public static JButton b1 = new JButton("1");
    public static JButton b2 = new JButton("2");
    public static JButton b3 = new JButton("3");
    public static JButton b4 = new JButton("4");
    public static JButton b5 = new JButton("5");
    public static JButton b6 = new JButton("6");
    public static JButton b7 = new JButton("7");
    public static JButton b8 = new JButton("8");
    public static JButton b9 = new JButton("9");
    public static JButton b10 = new JButton("10");
    public static JLabel l1 = new JLabel("Guess a number between 1 and 10!");


    public Main(){
        frame();
    }

    public void frame(){
        rand = new Random();
        number = rand.nextInt(10);

        JFrame f = new JFrame();
        f.setResizable(false);
        f.setSize(500,500);
        f.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        f.setVisible(true);
        JPanel p  = new JPanel();
        f.add(p);
        p.add(b1);
        p.add(b2);
        p.add(b3);
        p.add(b4);
        p.add(b5);
        p.add(b6);
        p.add(b7);
        p.add(b8);
        p.add(b9);
        p.add(b10);
        p.add(l1);

        b1.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent e){
                guess = 1;
                if(guess == number){
                    l1.setText("You have won the number was " + number);
                }
                if(guess > number){
                    l1.setText("Too high");
                }
                if(guess < number){
                    l1.setText("Too low");
                }
            }
        });

        b2.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent e){
                guess = 2;

                if(guess == number){
                    l1.setText("You have won the number was " + number);
                }
                if(guess > number){
                    l1.setText("Too high");
                }

                if(guess < number){
                    l1.setText("Too low");
                }
            }
        });

        b3.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent e){
                guess = 3;

                if(guess == number){
                    l1.setText("You have won the number was " + number);
                }
                if(guess > number){
                    l1.setText("Too high");
                }

                if(guess < number){
                    l1.setText("Too low");
                }
            }
        });

        b4.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent e){
                guess = 4;

                if(guess == number){
                    l1.setText("You have won the number was " + number);
                }
                if(guess > number){
                    l1.setText("Too high");
                }

                if(guess < number){
                    l1.setText("Too low");
                }
            }
        });

        b5.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent e){
                guess = 5;

                if(guess == number){
                    l1.setText("You have won the number was " + number);
                }
                if(guess > number){
                    l1.setText("Too high");
                }

                if(guess < number){
                    l1.setText("Too low");
                }
            }
        });

        b6.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent e){
                guess = 6;

                if(guess == number){
                    l1.setText("You have won the number was " + number);
                }
                if(guess > number){
                    l1.setText("Too high");
                }

                if(guess < number){
                    l1.setText("Too low");
                }
            }
        });

        b7.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent e){
                guess = 7;


                if(guess == number){
                    l1.setText("You have won the number was " + number);
                }
                if(guess > number){
                    l1.setText("Too high");
                }

                if(guess < number){
                    l1.setText("Too low");
                }
            }
        });

        b8.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent e){
                guess = 8;

                if(guess == number){
                    l1.setText("You have won the number was " + number);
                }
                if(guess > number){
                    l1.setText("Too high");
                }

                if(guess < number){
                    l1.setText("Too low");
                }
            }
        });

        b9.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent e){
                guess = 9;

                if(guess == number){
                    l1.setText("You have won the number was " + number);
                }
                if(guess > number){
                    l1.setText("Too high");
                }

                if(guess < number){
                    l1.setText("Too low");
                }
            }
        });

        b10.addActionListener(new ActionListener(){
            public void actionPerformed(ActionEvent e){
                guess = 10;


                if(guess == number){
                    l1.setText("You have won the number was " + number);
                }
                if(guess > number){
                    l1.setText("Too high");
                }

                if(guess < number){
                    l1.setText("Too low");
                }
            }
        });
    }   

    public static void main(String[] args){
        new Main();
    }
}
share|improve this question

migrated from stackoverflow.com Aug 27 at 8:52

This question came from our site for professional and enthusiast programmers.

    
I don't see that this code has anything to do with eclipse (even if you happened to use Eclipse to develop it), so I have retagged the question accordingly. –  200_success Aug 27 at 11:11

7 Answers 7

Create a new Actionlistener class and add it to all Buttons:

b1.addActionListener(new MyActionListener(1))
b1.addActionListener(new MyActionListener(2))
...

--

class MyActionListener implements ActionListener {

    private int guess ;
    public MyActionListener(int guess){
        this.guess = guess;
    }
    @Override
    public void actionPerformed(ActionEvent e) {
        if(guess == number){
            l1.setText("You have won the number was " + number);
        }
        if(guess > number){
            l1.setText("Too high");
        }

        if(guess < number){
            l1.setText("Too low");
        }

    }

}
share|improve this answer

A few things:

  • You are overusing public static, all those variables should instead be private and not static. Whenever you can, avoid static. Also whenever you can, use private.

  • The variable guess is not needed on the class, it can be a local variable inside each method. But if you use the answer by Jens you can just remove it from where it is now.

  • You should consider using an array for your JButtons. Technically, they also don't need to be class variables. They can be local variables inside the frame method. You should just use a for-loop to create each button and add it to the panel, then you don't even need to store it in an array.

  • number = rand.nextInt(10); does not give you a random number from 1 to 10, it gives you a random number from 0 to 9. Imagine a user clicking '1' and getting the message "Too high". What will the user do then? Change it to number = rand.nextInt(10) + 1; and you have fixed that issue.

Remember these rules for how to get a random number in a specific range:

  • The parameter to rand.nextInt is how many different numbers it should be possible to get
  • Use + x where x is the minimum possible value.

So if you for example want a number from 10 to 20, that's 11 different numbers to choose from (count them if you don't believe me), and the minimum number is 10 so that's rand.nextInt(11) + 10

share|improve this answer
4  
I think the point about arrays is key. It should make you think about developing clean code that scales easily with the number of buttons. Any time you are copying and pasting code you should stop and think. –  brain Aug 27 at 13:48

There are a lot of problems with your code, but for this particular problem, you can just create a function:

public static void guess(int guessNumber) {
    if (guessNumber == number) {
        l1.setText("You have won the number was " + number);
    }
    if (guessNumber > number) {
        l1.setText("Too high");
    }

    if (guessNumber < number) {
        l1.setText("Too low");
    }
}

And call it like that in your action listeners:

    b10.addActionListener(new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent e) {
            guess(10);
        }
    });
share|improve this answer
    
Thanks that made it a lot smaller and more readible. –  SwordOfDoom Aug 27 at 7:33

You can use just one actionListener for all buttons:

ActionListener listner = new ActionListener(){
    @Override
    public void actionPerformed(ActionEvent e){
        switch(e.getSource()){
        case b1:
            guess(1);
        break;
        case b2:
            guess(2);
        break;
        ...
        }
    }
}

and just call .addActionListener(listener); for all buttons.

To remove the redundance of writing an if clause for each actionListener, make the method guess(int guess) I've already used in the code above:

private void guess(int guess){
    if(guess == number){
        l1.setText("You have won the number was " + number);
    }
    if(guess > number){
        l1.setText("Too high");
    }
    if(guess < number){
        l1.setText("Too low");
    }
}
share|improve this answer

As noted in the Concurrency in Swing Java tutorial, Swing code must run in the Event Dispatch Thread, not the main thread. Therefore, you need to wrap all of the GUI initialization code (namely frame()) inside SwingUtilities.invokeLater():

public class Main {
    public Main() {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                frame();
            }
        });
    }
}

For a simple program like this, you probably won't observe any ill effects from running Swing code in the main thread. However, in more complex situations, you could trigger deadlocks if you don't do things according to the documentation.

As others have mentioned, the class should be renamed to something more descriptive, like GuessingGame.

share|improve this answer
    
This is wrong. Swing code is executed in a separate Event Dispatch Thread, but that does NOT mean this anything with Swing must run in the EDT ! You can use as much Swing GUI generation and initialization as you like without ill effects. What the problem is if you are delegating tasks with considerable amount of execution time into the EDT. If that happens, the EDT hogs and the App finally freezes (what you call "deadlock"). But in this simple case the comparison code executed in the EDT is so fast that this will not happen even if you use 10 000 buttons. –  Thorsten S. Aug 27 at 21:28
    
@ThorstenS. Read the first two pages of the linked tutorial. Also see this example of a failure case if you don't do things by the book. –  200_success Aug 27 at 21:32
    
I have read it and I seriously disagree (to be polite). (Mis)using invokeLater in this way has only disadvantages: You open another thread (no, the EDT is not always there, it only appears when you call pack()/setVisible(true) in the frame) and you do not know the execution order because EDT and main are running parallel. In fact, the given failure case disproves your argument, because invokeLater did not prevent the hang, the solution was to create the JFrame first or move MIDI creation into a separate thread. –  Thorsten S. Aug 27 at 22:13
    
Anyway, I think I will read about it and very likely question this "wisdom" in stackoverflow. I will inform you about the results. –  Thorsten S. Aug 27 at 22:14
1  
@ThorstenS. If the execution order matters, then call SwingUtilities.invokeAndWait(). –  200_success Aug 27 at 22:23

I added comments to show what can be improved. Most of the oddities are already mentioned and explained in detail by other answers.

import javax.swing.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.Random;

/**
 * Main. Your class here now implements the interface ActionListener.
 * You do not need to create anonymous ActionListeners for each button.
 */
public class Main implements ActionListener {
  /** Use constants to change the length of the given options and avoid magical constants. By changing this constant you can have as much buttons as you like. */
  private static final transient int NUMBER_OF_BUTTONS = 10;

  /** We need the label as member to set its text, a better name is not bad. */  
  private JLabel  descriptionLabel;

  /** We need also the number to guess. */
  private int numberToGuess;

  /**
   * Constructor.
   */
  public Main() {
    // Always initialize your members before building up the user interface.
    Random rand = new Random();
    // See ? With the constant you automatically adjust the random number generation.
    // By the way, the number must be increased by one
    numberToGuess = 1 + rand.nextInt(NUMBER_OF_BUTTONS);
    // Tell the reader that you are setting up the GUI
    createFrameAndGUI();
  }

  /**
   * By moving creating your GUI into a seperate function
   * you did well.
   */
  private void createFrameAndGUI() {    
    // So far ok, nothing to be changed
    JFrame f = new JFrame();
    f.setResizable(false);
    f.setSize(500, 500);
    f.setDefaultCloseOperation(WindowConstants.EXIT_ON_CLOSE);
    JPanel p = new JPanel();

    // Now, simply generate the buttons on the fly. As the panel holds the buttons,
    // you do not need them later.
    for (int number = 1; number <= NUMBER_OF_BUTTONS; number++) {
      // Create your number string from the number. 
      String numberString = Integer.toString(number);
      // You have a button
      JButton numberButton = new JButton(numberString);
      // Write the number string into your button
      numberButton.setActionCommand(numberString);
      // Please, listen to me if I am pressed...
      numberButton.addActionListener(this);
      // Add to panel
      p.add(numberButton);
    }
    // Now we create the label
    descriptionLabel = new JLabel("Guess a number between 1 and 10!");
    // And add it
    p.add(descriptionLabel);

    // In Swing it is better to start with the lowest components
    // and add them to container. These containers are added to super
    // containers and finally to JWindows (JFrame is a JWindow).
    f.add(p);

    // We are ready, show yourself
    f.pack();
    f.setVisible(true);     
  }   

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

  public void actionPerformed(ActionEvent e){
    // Get the number String of the button which was pressed...
    String numberString = e.getActionCommand();
    // Convert it into an integer
    int number = Integer.parseInt(numberString);

    // Use if / else if / else for options which cannot be true for each branch
    if (numberToGuess == number) {
        descriptionLabel.setText("You have won the number was " + number);
    }
    else if (numberToGuess > number){
        descriptionLabel.setText("Too low");
    }
    else {
        descriptionLabel.setText("Too high");
    }
  }
}
share|improve this answer

First, ditch the public static nonsense you have around all your fields. Those modifiers are not needed, and static is in fact, wrong. What if you need to later have multiple instances of this running at the same time?

Rename your class from Main to something more descriptive, like GuessingGame.

Use more descriptive names for fields and local variables. Things like label, panel, and button, not l1, p, or b.

Also, move some of the initialization out of the frame() method. If your buttons and labels are going to be fields, your panel and frame should probably both be fields too.

Then, use an array to store your buttons. You can use an initializer block to automatically initialize the array, like this:

final int buttonCount = 10;
JButton[] buttons = new JButton[buttonCount];{
    for(int i = 1; i <= buttonCount; i++)
        buttons[i] = new JButton(String.valueOf(i));
}

You can use initializers and initializer blocks to initialize everything else, too.

JFrame frame = new JFrame();{
    frame.setResizable(false);
    frame.setSize(500,500);
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
}

JPanel panel = new JPanel();{
    frame.add(panel);
}

JLabel label = new JLabel("Guess a number between 1 and 10!");{
    panel.add(label);
}

int targetValue = ThreadLocalRandom.current().nextInt(1, buttonCount);

This then lets you simplify adding them all to the panel:

f.add(p);
for(JButton b: buttons) // this is the ONE case where one-letter variable names are OK --
    p.add(b); // -- when they are part of a `for` expression.
p.add(label);

And it also simplifies adding the listener, especially when combined with @Gumbo's suggestion to move the code into a guess method, and writing a short little class:

class GuessListener implements ActionListener {
    final GuessingGame parent;
    final int guessValue;

    public GuessListener(GuessingGame parent, int guessValue){
        this.parent = parent;
        this.guessValue = guessValue;
    }

    public void actionPerformed(ActionEvent e){
        parent.guess(guessValue)
    }
}

Assigning the listeners can then be done at the same time you assign the text. You can also move the part where you add them all to the panel to here. Replace the first initializer block from earlier with:

final int buttonCount = 10;
JButton[] buttons = new JButton[buttonCount];{
    for(int i = 1; i <= buttonCount; i++){
        buttons[i] = new JButton(String.valueOf(i));
        buttons[i].addActionListener(new GuessListener(this, i));
        panel.add(buttons[i]);
    }
}

Oh, and one more thing: You should NOT be running the actual game as part of the game's constructor. The constructor should be purely for setting up the game, with its configurations and all that. Instead, make a new method, play, that actually triggers the start of the game.

Since the frame method should be empty now, you can get rid of it altogether, along with the constructor - just use the default no-args constructor.

The play method is then just:

public void play() { frame.setVisible(true); }

To recap, here is the modified program:

public class GuessingGame {

    public static void main(String[] args){
        new GuessingGame().play();
    }

    class GuessListener implements ActionListener {
        final GuessingGame parent;
        final int guessValue;

        public GuessListener(GuessingGame parent, int guessValue){
            this.parent = parent;
            this.guessValue = guessValue;
        }

        public void actionPerformed(ActionEvent e){
            parent.guess(guessValue);
        }
    }

    JFrame frame = new JFrame();{
        frame.setResizable(false);
        frame.setSize(500,500);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    }

    JPanel panel = new JPanel();{
        frame.add(panel);
    }

    final int buttonCount = 10;
    JButton[] buttons = new JButton[buttonCount];{
        for(int i = 1; i <= buttonCount; i++){
            buttons[i-1] = new JButton(String.valueOf(i));
            buttons[i-1].addActionListener(new GuessListener(this, i));
            panel.add(buttons[i-1]);
        }
    }

    int targetValue = ThreadLocalRandom.current().nextInt(1, buttonCount + 1);

    JLabel label = new JLabel("Guess a number between 1 and 10!");{
        panel.add(label);
    }

    public void play() {
        frame.setVisible(true);
    }

    private void guess(int guessValue){
        if(guessValue == targetValue){
            label.setText("You have won the number was " + targetValue);
        }
        if(guessValue > targetValue){
            label.setText("Too high");
        }
        if(guessValue < targetValue){
            label.setText("Too low");
        }
    }
}
share|improve this answer
    
Just updated it with a few edits needed to make it work correctly. –  AJMansfield Aug 27 at 23:33

Your Answer

 
discard

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