7
\$\begingroup\$

I'd like my code to be solid and good. That's why I want someone a little more experienced to give me some feedback so I can improve. Here is the code for a simple stop watch that I created. I feel the code is a bit clumpy and big so that's why I want to get feedback on how to make it more efficient.

Compiled (runnable) .jar file download

Main Class:

    public static JTextField field;
    public static String time = "00:00:00:00";

    private static String imgIcon = "data/icon.png";
    private static Log log = new Log("StopWatch");
    private static Thread t = new Thread(new ClockTimer());
    private static Thread t1 = new Thread(new lolee());
    private static Thread t2 = new Thread(new StopWatch());
    private static JButton button;
    private static JButton button2;
    private static JButton button3;
    private static Action1 act1;
    private static Action2 act2;
    private static Action3 act3;
    private static keyPress1 key1;
    public JLabel statusBar;
    public static JFrame frame;
    private static final int WIDTH = 300;
    private static final int HEIGHT = 150;

    public static void main(String[] args) {
        log.info("Loading frame utilities");
        frame = new JFrame("Stop Watch V 1.2");
        frame.setDefaultCloseOperation(EXIT_ON_CLOSE);
        frame.setIconImage(new ImageIcon(imgIcon).getImage());
        frame.setSize(WIDTH, HEIGHT);
        frame.setResizable(false);
        frame.setLocation((getResulutionWidth() - WIDTH) / 2, (getResulutionHeight() - HEIGHT) / 2);
        frame.setLayout(new BorderLayout());
        frame.setVisible(true);
        log.info("Rendering and configuring frame elements");
        t2.start();
    }

    public void run() {
        loadUI();
        loadTextBox();
        t1.start();
        t.start();
    }

    private static void loadUI() {
        button = new JButton("    Start    ");
        button2 = new JButton("Pause");
        button3 = new JButton("    Reset    ");
        button.setFocusable(false);
        button2.setFocusable(false);
        button3.setFocusable(false);
        frame.add(button, BorderLayout.WEST);
        frame.add(button2, BorderLayout.CENTER);
        frame.add(button3, BorderLayout.EAST);
        key1 = new keyPress1();
        act1 = new Action1();
        act2 = new Action2();
        act3 = new Action3();
        button.addActionListener(act1);
        frame.addKeyListener(key1);
    }

    private static void loadTextBox() {
        field = new JTextField(time);
        frame.add(field, BorderLayout.NORTH);
    }

    public void setTime(String t) {
        time = t;
    }

    private static int getResulutionHeight() {
        Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize();
        return (int) screenSize.getHeight();
    }

    private static int getResulutionWidth() {
        Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize();
        return (int) screenSize.getWidth();
    }

    // Start Button

    static class Action1 implements ActionListener {
        public void actionPerformed(ActionEvent e) {
            perform();
        }

        public void perform() {
            ClockTimer.setRunningState(true);
            button.removeActionListener(this);
            button2.addActionListener(act2);
            button3.removeActionListener(act3);
        }
    }

    static class Action2 implements ActionListener {
        public void actionPerformed(ActionEvent arg0) {
            perform();
        }

        public void perform() {
            ClockTimer.setRunningState(false);
            button.addActionListener(act1);
            button2.removeActionListener(this);
            button3.addActionListener(act3);
        }

    }

    static class Action3 implements ActionListener {
        public void actionPerformed(ActionEvent e) {
            perform();
        }

        public void perform() {
        button3.removeActionListener(this);
        ClockTimer.reset();
        }
    }

    static class keyPress1 implements KeyListener {

        public void keyPressed(KeyEvent e) {
            int k = e.getKeyCode();
            if(k == 32 || k == 83) {
                if(ClockTimer.getRunningState()) {
                    act2.perform();
                } else if(!ClockTimer.getRunningState()) {
                    act1.perform();
                }
            }else if(k == 82 && !ClockTimer.getRunningState()) {
                act2.perform();
                act3.perform();
            }
        }

        public void keyReleased(KeyEvent e) {

        }

        public void keyTyped(KeyEvent e) {

        }
    }
}

Class with the timer:

import java.util.Random;
import javax.swing.JFrame;
public class ClockTimer implements Runnable {

    private static final Log log = new Log("tick");
    private static boolean isRunning = false;
    private static int ms = 00;
    private static int s = 00;
    private static int m = 00;
    private static int h = 00;
    private static StopWatch l = new StopWatch();
    private String t;
    private long startTime;

    public void run() {
        log.info("Loading timer method");
        while (true) {
        startTime = System.nanoTime();
        while (isRunning) {
            if (System.nanoTime() - startTime >= 10000000) {
                startTime = System.nanoTime();
                ms++;
                if (ms == 100) {
                    s++;
                    ms = 0;
                    if (s == 60) {
                        m++;
                        s = 0;
                        if (m == 60) {
                            h++;
                            m = 0;
                        }
                    }
                }
                t = String.format("%02d:%02d:%02d:%02d\n", h, m, s, ms);
                l.setTime(t);
                //System.out.printf("%02d:%02d:%02d:%02d\n", h, m, s, ms);
            }
        }
        }
    }

    public static boolean getRunningState() {
        return isRunning;
    }

    public static void setRunningState(boolean b) {
        isRunning = b;
        log.debug("Clock timer running:"+isRunning);
    }

    public static void reset() {
        log.info("Resseting values of timer");
        ms = 0;
        s = 0;
        m = 0;
        h = 0;
        l.setTime("00:00:00:00");
    }
}

Class that updates the textbox (don't mind the name):

import java.awt.BorderLayout;
import java.awt.Font;

public class lolee extends StopWatch implements Runnable{

    public void run() {
        clock();
    }

    public static void clock() {
        Log l = new Log("clock");
        l.info("Loading method for frame update");
        Font bigFont = field.getFont().deriveFont(Font.PLAIN, 50f);
        field.setFont(bigFont);
        frame.pack();
        while(true) {
        field.setText(time);
        frame.add(field, BorderLayout.NORTH);
        frame.repaint();
        }
    }
}

Log class (Nothing important, made it just for fun)

import java.util.Date;


public class Log {

    private String Class;
    private Date time;

    public Log(String Class) {
        this.Class = Class;
    }

    @SuppressWarnings("deprecation")
    private String getTime() {
        time = new Date();
        return String.format("%02d:%02d:%02d", time.getHours(), time.getMinutes(), time.getSeconds());
    }

    public void info(String m) {
        System.out.printf("[%s][INFO][%s.class]%s\n", getTime() ,Class, m);
    }

    public void debug(String m) {
        System.out.printf("[%s][DEBUG][%s.class]%s\n", getTime() ,Class, m);
    }
}
\$\endgroup\$
10
\$\begingroup\$

Hmmm.... this is a big one. Right, let's break this review in to five sections:

  1. Code Style
  2. Threads
  3. Class Hierarchy
  4. Swing
  5. Performance....

Code Style

This is a laundry-list of things. I'm not going to explain them all, but:

  • Class names should be CamelCase, lolee is not a good name. Nor is class keyPressed.

  • Class names like Action1 etc. are also useless for knowing what they do.

  • button, button2 and button3 should be called playButton, pauseButton, and resetButton, or something.

  • act1, act2, and act3 should be similarly named.

  • t, t1, and t2 are bad names for threads.

  • indentation should be consistent. The while-block is not correctly indented:

        while(true) {
        field.setText(time);
        frame.add(field, BorderLayout.NORTH);
        frame.repaint();
        }
    
  • deprecation. If something is marked as deprecated, you should not be using it. It means there is a better alternative (in this case, Gregorian Calendar if you are on Java7, or Instant if you are on Java8).

Threads

your system has 5 threads:

  1. the main thread
  2. the Event Dispatch Thread
  3. The Stopwatch Thread
  4. The ClockTimer Thread
  5. The lolee thread

You only need three:

  1. main thread.
  2. Event Dispatch Thread
  3. a Timer thread

Also, your lolee thread is in a 'tight loop' updating the UI continuously, and that is putting the CPU at 100%. It only needs to update the UI 100 times a second, so that is a waste.

Your Threads should be set to be Daemon threads so they are better behaved on system exit.

With all those threads running, there are a lot of concurrency problems, They are all accessing the same fields, but you have not established any synchronization. It is conceivable (though you don't notice it because it happens fast), that the stopwatch 'time' value does not change when you think it does. The UI may in fact have a very 'stale' time, and the updates may not be as regular as you think.

Classes

You have a very unusual class structure. This is because your fields are all statics. You Have two Runnable instances that are actually the same class, and it only works because you have a single static 'time' variable that is shared. The lolee class has two run methods (one in lolee the other in StopWatch). This is .... confusing.

Swing

    while(true) {
    field.setText(time);
    frame.add(field, BorderLayout.NORTH);
    frame.repaint();
    }

This code should not be adding the field every time it loops...... and, it should be setText and repainting on the (EDT Event Dispatch Thread)

The way you set up your frame is unconventional. You should have a Container class for your application with private variables (not static). One of them should be a JFrame. You should construct this class from the main thread, and populate and pack the frame. Then you should show() it. You have it all mixed up in the main method, and in static variables... it's ... a mess.

You add and remove action listeners to your buttons as you need them, but you should simply be enabling and disabling the buttons instead.

Performance

The tight loop in the GUI update is poor form. It is unnecessary. There are better ways.

Alternative ....

I'm messing with your code at the moment... essentially rewriting it. Hope you don't mind. In a bit I will update this answer with that code.... and here it is...

Key features:

  • Locking for the variables that change in different threads
  • Swing data changes are made in the EventDispatchThread.
  • Uses Actions to control the ... action.
  • Uses a ScheduledTimerExecutorService for scheduling things.
  • CPU usage is much less....
  • better class structures
  • only updates GUI when things change.
  • uses better layout managers to ensure equal-sized buttons (instead of using space padding in the button names).

There may be a few things that are left over..... This is substantially different to your implementation. This is both a good thing, and a bad thing. Look through the code, identify why it is different, decide which approach is better. Learn..... but, I am not saying the following code is perfect.....

package stopwatch;

import java.awt.BorderLayout;
import java.awt.Dimension;
import java.awt.Font;
import java.awt.Toolkit;
import java.awt.event.ActionEvent;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReentrantLock;

import javax.swing.AbstractAction;
import javax.swing.Action;
import javax.swing.BoxLayout;
import javax.swing.ImageIcon;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JTextField;
import javax.swing.SwingUtilities;

@SuppressWarnings("serial")
public class StopWatch {

    private static final String imgIcon = "data/icon.png";
    private static final int WIDTH = 300;
    private static final int HEIGHT = 150;

    private static final String RESETTIME = buildTime(0);

    private static int getResulutionHeight() {
        Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize();
        return (int) screenSize.getHeight();
    }

    private static int getResulutionWidth() {
        Dimension screenSize = Toolkit.getDefaultToolkit().getScreenSize();
        return (int) screenSize.getWidth();
    }

    public static void main(String[] args) {
        StopWatch sw = new StopWatch();
        sw.setVisible();
    }

    private static final String buildTime(final long elapsed) {
        long hundredths = elapsed / 10;
        long seconds = hundredths / 100;
        long minutes = seconds / 60;
        long hours = minutes / 60;

        return String.format("%02d:%02d:%02d:%02d", hours, minutes % 60, seconds % 60, hundredths % 100);
    }

    private final Log log = new Log("StopWatch");
    private final JTextField fieldTime;
    private final JButton buttonPlay;
    private final JButton buttonPause;
    private final Action actionPlay;
    private final Action actionPause;
    private final Action actionReset;

    private final JFrame frame;

    private final ReentrantLock lock = new ReentrantLock();

    private long lasttick = 0L;
    private boolean paused = false;
    private long elapsed = 0L;
    private ScheduledFuture<?> running = null;

    private final ScheduledExecutorService executor = Executors.newScheduledThreadPool(1);

    private final Runnable ticker = new Runnable() {
        public void run() {
            tick();
        }
    };

    public StopWatch() {
        log.info("Building StopWatch constructor");
        frame = new JFrame("Stop Watch V 1.2");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setIconImage(new ImageIcon(imgIcon).getImage());
        frame.setSize(WIDTH, HEIGHT);
        frame.setResizable(false);
        frame.setLocation((getResulutionWidth() - WIDTH) / 2, (getResulutionHeight() - HEIGHT) / 2);
        JPanel mainPanel = new JPanel();
        mainPanel.setLayout(new BoxLayout(mainPanel, BoxLayout.PAGE_AXIS));
        frame.setContentPane(mainPanel);

        fieldTime = new JTextField(RESETTIME);
        fieldTime.setEditable(false);
        Font bigFont = fieldTime.getFont().deriveFont(Font.PLAIN, 50f);
        fieldTime.setFont(bigFont);
        mainPanel.add(fieldTime);

        JPanel buttonPanel = new JPanel();
        buttonPanel.setLayout(new BoxLayout(buttonPanel, BoxLayout.LINE_AXIS));
        mainPanel.add(buttonPanel, BorderLayout.SOUTH);

        actionPlay = new AbstractAction("Start") {

            @Override
            public void actionPerformed(ActionEvent e) {
                start();
            }
        };

        actionPause = new AbstractAction("Pause") {
            @Override
            public void actionPerformed(ActionEvent e) {
                pause();
            }
        };

        actionReset = new AbstractAction("Reset") {
            @Override
            public void actionPerformed(ActionEvent e) {
                reset();
            }
        };

        buttonPlay = new JButton(actionPlay);
        buttonPanel.add(buttonPlay);
        buttonPause = new JButton(actionPause);
        buttonPanel.add(buttonPause);
        buttonPanel.add(new JButton(actionReset));

        log.info("Rendering and configuring frame elements");
    }

    private final void setVisible() {
        frame.setVisible(true);
        buttonPlay.requestFocusInWindow();
    }

    private void start() {
        lock.lock();
        try {
            if (running != null) {
                return;
            }

            paused = false;
            lasttick = System.currentTimeMillis();
            running = executor.scheduleAtFixedRate(ticker, 10, 10, TimeUnit.MILLISECONDS);

            actionPlay.setEnabled(false);
            actionPause.setEnabled(true);
            actionReset.setEnabled(false);
            buttonPause.requestFocusInWindow();
        } finally {
            lock.unlock();
        }
    }

    private void pause() {
        lock.lock();
        try {
            if (running == null) {
                return;
            }
            running.cancel(false);
            paused = true;
            running = null;

            actionPlay.setEnabled(true);
            actionPause.setEnabled(false);
            actionReset.setEnabled(true);
            buttonPlay.requestFocusInWindow();
        } finally {
            lock.unlock();
        }
    }

    private void reset() {
        lock.lock();
        try {
            if (running != null) {
                return;
            }
            elapsed = 0;
            displayTime(RESETTIME);

            actionPlay.setEnabled(true);
            actionPause.setEnabled(false);
            actionReset.setEnabled(true);
            buttonPlay.requestFocusInWindow();
        } finally {
            lock.unlock();
        }
    }

    private void tick() {
        lock.lock();
        try {
            long now = System.currentTimeMillis();
            long delta = now - lasttick;
            lasttick = now;
            if (!paused) {
                elapsed += delta;
                displayTime(buildTime(elapsed));
            }
        } finally {
            lock.unlock();
        }
    }

    private void displayTime(final String todisplay) {
        SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                fieldTime.setText(todisplay);
            }
        });
    }


}
\$\endgroup\$

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.