current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 3 down vote favorite
1

I am a Java beginner and have made a simple Compare Game. It has done well but I am not sure about my solution, especially about my thread. Can you give some advice on making it better?

public class CompareGame {
    static short first=0;
    static short second=0;
    static JLabel lblFirst = new JLabel("First");
    static JLabel lblSecond = new JLabel("Second");
    private JFrame frame;
    static String math = null;
    JButton btnReset = new JButton("Reset");
    JButton btnGo = new JButton("Go");

    public static void main(String[] args) {
        EventQueue.invokeLater(new Runnable() {
            public void run() {
                try {
                    CompareGame window = new CompareGame();
                    window.frame.setVisible(true);
                } catch (Exception e) {
                    e.printStackTrace();
                }
            }
        });
    }

Main frame:

public CompareGame() {
        initialize();
    }

    private void initialize() {
        frame = new JFrame("CompareGame");
        frame.setBounds(100, 100, 432, 279);
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.getContentPane().setLayout(null);

        JLabel lblWelcome = new JLabel("Welcome");
        lblWelcome.setHorizontalAlignment(SwingConstants.CENTER);
        lblWelcome.setBounds(161, 30, 81, 14);
        frame.getContentPane().add(lblWelcome);

        lblFirst.setBounds(88, 75, 46, 14);
        frame.getContentPane().add(lblFirst);

        JLabel lblMath = new JLabel("Math");
        lblMath.setHorizontalAlignment(SwingConstants.CENTER);
        lblMath.setBounds(178, 75, 46, 14);
        frame.getContentPane().add(lblMath);

        lblSecond.setBounds(285, 75, 46, 14);
        frame.getContentPane().add(lblSecond);

        JButton btnLeft = new JButton("<");
        btnLeft.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {lblMath.setText("<");
            }
        });
        btnLeft.setBounds(38, 114, 89, 23);
        frame.getContentPane().add(btnLeft);

        JButton btnMid = new JButton("=");
        btnMid.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {lblMath.setText("=");
            }
        });
        btnMid.setBounds(166, 114, 89, 23);
        frame.getContentPane().add(btnMid);

        JButton btnRight = new JButton(">");
        btnRight.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent e) {lblMath.setText(">");
            }
        });
        btnRight.setBounds(285, 114, 89, 23);
        frame.getContentPane().add(btnRight);

        class Countdown implements Runnable{
                public void run(){
                long time=System.currentTimeMillis();       
                long future=time+2*1000;            //2 seconds countdown time
                while(time<future){time=System.currentTimeMillis();}
                btnGo.doClick();
        }
        }

            btnReset.addActionListener(new ActionListener() {
            public void actionPerformed(ActionEvent arg0) {
            lblWelcome.setText(null);
            lblMath.setText("?");
            btnLeft.setEnabled(true);btnMid.setEnabled(true);
            btnRight.setEnabled(true);
            math();
            Countdown Time=new Countdown();
            new Thread(Time).start();
            }
        });
        btnReset.setBounds(166, 192, 89, 23);
        frame.getContentPane().add(btnReset);

        btnGo.addActionListener(new ActionListener() {  // I used button to do job
            public void actionPerformed(ActionEvent e) {
                String result=lblMath.getText();
                if(result==math)
                {lblMath.setText("?");
                math();
                Countdown Time=new Countdown();
                new Thread(Time).start();}
                else
                {lblWelcome.setText("Game Over");
              btnLeft.setEnabled(false);btnMid.setEnabled(false);
              btnRight.setEnabled(false);}
            }
        });
        btnGo.setBounds(1, 1, 1, 1);
        frame.getContentPane().add(btnGo);
        btnGo.setVisible(false);
                }
    private static String math(){        // creat number
        Random random=new Random();
        first=randomShort(1, 100, random );
        lblFirst.setText(String.valueOf(first));
        second=randomShort(1,100,random);
        lblSecond.setText(String.valueOf(second));
        if (first<second) math="<";
        if (first==second)math="=";
        if (first>second) math=">";
        return math;
    }
        private static short randomShort(int i, int j, Random random) { //random number
        short range = (short) (j - i + 1);
        short fraction = (short) (range * random.nextDouble());
        short randomNumber = (short) (fraction + i);
        return randomNumber;}   
}
share|improve this question

2 Answers 2

up vote 6 down vote accepted

  1. Your code formatting looks broken(indentation is inconsistent, opening/closing curly brackets are located in strange places, multiple statements in one line, etc). It makes your code very hard to read.

  2. Operators should be surrounded by whitspaces. For instance,

    long future = time + 2 * 1000;

    is more readable than 

    long future=time+2*1000;

  3. Variables and methods names are not self-descriptive in your code. For example, a name math() for a method is bad: it does not reflect what this method actually does. You could call it generateNumbers() or something like that. The same goes for variables: for instance, math is pretty a strange name for a variable(and again, it does show the meaning of this variable). Moreover, there is no point in mangling words: buttonLeft is a better name than btnLeft, labelFirst or firstLabel is more comprehensive than lblFirst and so on.

  4. Creating your own class that works like a timer(Countdown) is a bad idea. A timer is already present in Java standard library: java.swing.Timer, so you can use it.

  5. You can make you code more concise by using lambda-expressions if you have Java8. For example, you can add a listener to a button like this:

    button.addActionListener((ActionEvent e) -> {
    // do something
});
share|improve this answer
    
Thank you so much. I'll write it to my note. –  Nongdan Xitrum Jan 6 at 14:49
    
I would consider long future = time + 2*1000; more readable then either example you have. It nudges your brain to evaluate with correct precedence. Not all places that allow whitespace deserve it. –  Shade Jan 6 at 20:27
1  
@Shade To me, any binary operator not wrapped around with whitespaces looks worse than with them. Moreover, according to Oracle Java coding conventions(and other style guides like Google code style) whitespaces should be there. –  ILoveCoding Jan 6 at 20:40
up vote 5 down vote

You use a busy loop for a timeout. There is a better method with javax.swing.Timer. This removes the need for btnGo and the thread.

private Timer timer;// as field

timer = new Timer(2*1000, new ActionListener(){
    public void actionPerformed(ActionEvent e) {

        String result=lblMath.getText();
        if(result==math) {
            lblMath.setText("?");
            math();
        }else{
             lblWelcome.setText("Game Over");
             btnLeft.setEnabled(false);
             btnMid.setEnabled(false);
             btnRight.setEnabled(false);
             timer.stop();
        }
    }
});
timer.setRepeats(true);//auto restarts the timer after it triggers

Then when you need to start it you just call timer.start();.

Other than that please keep a consistent format, especially the indentation and brace style. It will vastly increase readability. Compare the listener I provided to your btnGo listener.

Also the fixed layout of the components will do odd things when the user tries to resize the window, I suggest using a layout manager.

share|improve this answer
    
Thank you. I'll learn javax.swing.Timer. and add it to my app. –  Nongdan Xitrum Jan 6 at 14:51
    
timer.stop() can not put in that field, I can not fix it. Please help me. –  Nongdan Xitrum 2 days ago

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.