5
\$\begingroup\$

I have made a UI that guesses the number that the user is thinking of(0 - 100), and I am wondering if it can be improved(I guess this is simple AI). I tried to shorten it as best I could, but it still seems a bit "fluffed." I would greatly appreciate anyone willing to help me shorten and/or improve it. It has 3 buttons, a "Too High", "Too Low", and "Just Right" button, and the guess is displayed in the center of the window. I have posted an image below also.

import java.awt.BorderLayout;
import java.awt.Color;
import java.awt.EventQueue;
import java.awt.Font;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.util.Random;

import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.UIManager;

public class NumberGuesser extends JFrame {
    private static final long serialVersionUID = 1L;
    private Random rand = new Random();
    private int min = 0;
    private int max = 100;
    private int guess = rand.nextInt(max);
    private JLabel guesses;
    private Font font = new Font("Times New Roman", Font.BOLD, 30);

    public NumberGuesser() {
        initGUI();
        setTitle("Number Guesser");
        pack();
        setLocationRelativeTo(null);
        setVisible(true);
        setResizable(false);
        setDefaultCloseOperation(EXIT_ON_CLOSE);
    }

    private void initGUI() {
        JLabel title = new JLabel(" I Guess Your Number(0-100) ");
        title.setFont(font);
        title.setHorizontalAlignment(JLabel.CENTER);
        title.setBackground(Color.BLACK);
        title.setForeground(Color.WHITE);
        title.setOpaque(true);
        add(title, BorderLayout.NORTH);

        guesses = new JLabel(Integer.toString(guess));
        guesses.setFont(font);
        guesses.setHorizontalAlignment(JLabel.CENTER);
        add(guesses, BorderLayout.CENTER);

        JPanel buttons = new JPanel();
        add(buttons, BorderLayout.SOUTH);

        JButton higher = new JButton("Too High");
        higher.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                max = guess;
                guessAgain(min, max);
            }
        });
        buttons.add(higher);

        JButton correct = new JButton("Just Right");
        correct.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                guesses.setText("I WIN!");
            }
        });
        buttons.add(correct);

        JButton lower = new JButton("Too Low");
        lower.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {
                min = guess;
                guessAgain(min, max);
            }
        });
        buttons.add(lower);
    }

    private void guessAgain(int min, int max) {
        guess = min + (int)(Math.random() * ((max - min) + 1));
        guesses.setText(Integer.toString(guess));

    }

    public static void main(String[] args) {
        try {
            String laf = UIManager.getCrossPlatformLookAndFeelClassName();
            UIManager.setLookAndFeel(laf);
        } 
        catch (Exception e) {}

        EventQueue.invokeLater(new Runnable(){
            public void run() {
                new NumberGuesser();
            }
        });
    }
}

The output looks like this:

enter image description here

\$\endgroup\$
  • \$\begingroup\$ Sorry, but this site is about the code as is. For tips on fixing that you should post it on stackoverflow.com \$\endgroup\$ – Timothy Truckle Jul 3 '17 at 6:43
1
\$\begingroup\$

Thanks for sharing the code!

pros

Naming

Finding good names is the hardest part in programming. You did a good job here. Especially I'd like to mention:

Visibility

You declared all your members and methods private. This is good since it serves both, the information hiding/encapsulation principle and the interface segregation principle.

cons

Magic numbers

Your code contains some literal values.

You should create constants with descriptive names for them. e.g.:

private static final int INITIAL_MINIMUM = 0;
private static final int INITIAL_MAXIMUM = 100;
private static final int TITLE_FONT_SIZE = 30;
private static final String TITLE_FONT_NAME = "Times New Roman";

Inheritance

Your NumberGuesser extends JFrame. I know that many examples out there do this but it is wrong. Your class does not add or change any behavior of its base class. It only configures it. This could also be done with an explicitly created JFrame instance.

Only extend a base class when you have to add or change the base classes behavior.

Call of JFrame.setVisible()

This call should always be the last after completely configuring the frame.

Separation of Concerns / Single Responsibility Pattern

Each class or method in a program should have a clear responsibility. The instantiation of objects (using the new operator) is a responsibility of its own as well as the configuration of the GUI elements.

Work in constructor

In general constructors should only initialize (final) object members with values passed in as parameters.

Yes, many online resources and even official tutorials do initialization in constructors, but it is still a bad practice

Members

Member variables define the objects state. But some of your members will never change. That means they will never get a new value during life time of the object.

You should document that by making them final.

\$\endgroup\$
  • \$\begingroup\$ Thank you so much, I really appreciate it. This is all so helpful! \$\endgroup\$ – CodingNinja Jul 3 '17 at 16:23

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.