Digital Clock in Java

Question

I'm just looking for some feedback:

import javax.swing.JFrame;
import java.awt.Color;
import javax.swing.JComboBox;
import javax.swing.JPanel;

public class Clock extends JFrame {


    public Clock () {
        setContentPane(new ScreenSaver());
    }

    public static void main(String[] args) {

        Clock clock = new Clock();

        clock.setSize(685, 80);
        clock.setUndecorated(true);
        clock.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        clock.setBackground(Color.BLACK);
        clock.setResizable(false);
        clock.setVisible(true);
        clock.toFront();

    }
}

Second class:

public class ScreenSaver extends JPanel implements ActionListener {

    public String stringDate;
    public JComboBox colorList;
    public Color userColor = Color.RED;
    public JPanel panel = new JPanel();
    public JButton button = new JButton("Alarm Clock");
    public static String time = "";
    public static Date date;
    public static DateFormat df;

    public ScreenSaver () {
        date();
        setLayout(new BorderLayout());
        Timer timer = new Timer(1000, this); 
        timer.start();


        panel.setBackground(new Color(0,0,0,0));
        panel.setLayout(new BorderLayout());

        add(panel, BorderLayout.PAGE_START);

        String [] colorName = {"Red", "Blue", "Yellow", "Green", "Pink", "Grey"}; 
        colorList = new JComboBox(colorName);
        colorList.setBackground(Color.BLACK);
        panel.add(colorList, BorderLayout.LINE_START);

    }

    public void date() {
        df = new SimpleDateFormat("EEE,MMM d yyyy - h:mm:ss a");
        date = new Date();
        stringDate = df.format(date);

    }


    public void actionPerformed(ActionEvent e) {
        if (colorList.getSelectedItem().equals("Red"))
        userColor = Color.RED;
        else if (colorList.getSelectedItem().equals("Blue"))
        userColor = Color.BLUE;
        else if (colorList.getSelectedItem().equals("Yellow"))
        userColor = Color.YELLOW;
        else if (colorList.getSelectedItem().equals("Green"))
        userColor = new Color(5, 200, 51);
        else if (colorList.getSelectedItem().equals("Pink"))
        userColor = new Color(249, 100, 178);
        else if (colorList.getSelectedItem().equals("Grey"))
        userColor = Color.GRAY;

        colorList.setForeground(userColor);

    }

    public void paintComponent(Graphics g) { 
       date();
       super.paintComponent(g);
       g.setColor(Color.BLACK);
       g.fillRect(0, 0, 685, 100);

        Font font = null;
        try {
            font = Font.createFont(Font.TRUETYPE_FONT, getClass().getResource("/DIGITALDREAMFAT.ttf").openStream());
        } catch(FontFormatException e) {
            e.printStackTrace();
        } catch(IOException e) {
            e.printStackTrace();
        }

        GraphicsEnvironment gen = GraphicsEnvironment.getLocalGraphicsEnvironment();
        gen.registerFont(font);
        font = font.deriveFont(30f);
        g.setFont(font);

       g.setColor(userColor);
       g.drawString(stringDate, 10, 60);
       repaint();

    }
}

Answer 1

CPU Load Bug: Wrong Usage of Timer and repaint()

repaint() calls paintComponent(). The way your code is written, your Java program is repainting all the time, consuming a CPU core completely. Instead it should repaint only when a change happens, and that is if the user changed the color or the timer was triggered. The call to repaint() should not be in paintComponent() but in actionPerformed().

It also means that the call to date() is in the wrong method. While it works being in paintComponent(), it should actually be in actionPerformed() - the timestamp shall be updated when the timer expires, not when the paint happens, because painting might happen much more frequently.

Remove unused symbols

Your fields button and time are not used and their initializations involve no (relevant) side-effects, you can remove them completely.

Fields should be private

All your fields are public. Fields should be private. Technically you can make public fields, but that's considered bad design: It violates data hiding, one of the fundamental principles of object-oriented design. The consequence would be a breach of encapsulation and cohesion with too tight coupling to internals.

Prefer delegation over inheritance (and follow LSP)

It's often misunderstood that inheritance is at the core of OOP. It's not. What's at the core of OOP is polymorphic abstraction. Inheritance is the cream on top of it, and just as too much cream isn't healthy for the body, too much inheritance is not healthy for a design.

Using inheritance can also lead to a violation of the LSP - Liskov Substitution Principle. Subclasses are to be able to replace their superclasses in all cases. But your Clock class cannot be used or extended by yet another class to get an even more specialized version of a Window (or Frame or JFrame) in a meaningful way.

Another sign of wrong inheritance is that your class Clock does not add any behavior to its base class JFrame.

So, instead, you should not inherit from JFrame but just use a JFrame in your main method.

Also, pick the right superclass: Use JComponent instead of JPanel.

Your ScreenSaver is actually not a JPanel. A JPanel is a component designed to be a container with a layout for reuse. What you want as superclass is JComponent. You have multiple components, and that's possible with JComponent, but you don't want anyone to take your ScreenSaver and mess around with its LayoutManager and its contained components.

Think of data structures for replacing switch case - OCP

switch case you might ask? You have a long chain of if else which is just a switch case in disguise. In this case, it could easily be replaced by a Map<String, Color>. You could even use that map to initialize the JComboBox.

This is also related to a principle known as OCP - Open Closed Principle. It was first described by Bertrand Meyer and is one of the first five principles of object-oriented class design collected by Robert C. Martin. It says that, when a new feature needs to be implemented, a class (or actually any software entity on its level of abstraction) should be open for extension but closed for modification. It can also be seen as a special case of the SRP - Single Responsibility Principle: a class (or actually any software entity on its level of abstraction) should have only one reason to change. Which is interesting, as in a side track that defines responsibility as reason to change.

Now what is that theoratical babble all about? Imagine you want to add a new color: In your program you have to make a change in two (actually three) places:

• The list of items given to the JComboBox,
• and the color names actionPerformed() method that selects the color,
• and the actual colors in the actionPerformed() method.

That can easily get out of sync. But even worse: There could also be more subtile errors, like in the JComboBox grey being written as "Grey", and in the actionPerformed() method grey being written as "Gray". Such errors can be very difficult to spot.

When you use a single central Map<String, Color>, then in your system there will be only one authority over the information of colors and their display names. Nothing can get out of sync. Much easier to maintain, much less error prone. You could even later easily load this stuff from a file then.

Use verbs for method names

Your method date is a noun. Okay, it can be argued that date actually works very very well as a verb, too. Just maybe not in this context, in this context date is thought of as a noun. So you could use updateDate instead.

Avoid mutable static fields

Mutable static fields are a dangerous source of error. They prevent multiple instances of the class to work properly independent of each other.

You might argue that in this case, there would never be two instances with different values for date and df. It would still be wrong, for two reasons:

• You might want to run two instances in the same VM on different clocks (see below for Java 8 datetime API), for example to show different time zones, or just for testing. Then sharing the data between the instances would be wrong.

• While the fields are global (once), there still are multiple Timers (one for each instance of Clock).

Reduce scope as much as possible - use local variables.

The variables panel, date and df do not need to be global, you can turn them into local variables of the respective methods.

Put the right amount of code inside the try-Block.

That you initialize font with null, then catch exceptions, which means font might still be null, and then continue using font afterwards, means that the remaining code eventually uses null. null should be avoided as much as possible, and those situations where null cannot be avoided, should be contained and controlled as much as possible.

Load and Initialize Only Once.

You do not need to load the font and register it with the graphics environment every time the component is repainted. It's sufficient to load and register the font once during application startup.

Similarly you do not need to create a new DateFormat every time the Timer calls your actionPerformed() method. It's sufficient to create only one global DateFormat object.

Collapse catch blocks

You can use Java 7 multicatch to collapse multiple catch blocks into one when the code executed for the catch blocks is identical.

try {
    // some code
} catch (Exception1 e) {
    e.printStackTrace();
} catch (Exception2 e) {
    e.printStackTrace();
}

can be replaced with

try {
    // some code
} catch (Exception1 | Exception2 e) {
    e.printStackTrace();
}

Make smaller methods

Your methods are quite big. Ideal methods do only one thing, they do it well and they do it only. For example, loading the font (including registering) is one thing, handling the exceptions in case the font couldn't be loaded, and when it was loaded successfully, setting it for the component, is another thing.

Make the change of color immediate

It feels a bit weird for a user to change the color and not seeing anything happening (until the second expires). For me, a second is a pretty long time. I can type 10 characters in a second, or shoot 3 people in Unreal Tournament when I'm lucky. You could change your program in a way that changes to the color become immediate.

Let Swing keep track of what it can

Swing can keep track of a lot of stuff. Explore the API. Your ScreenSaver is a JPanel. A JPanel is a JComponent. As such, it has font, foreground color and background color. You can just set the font and the color, and the Graphics object for the paintComponent() method will be initialized accordingly.

You might also want to consider simply involving a JLabel.

Add @Override annotation when implementing or overriding methods

This way the tools can help you when you misspell your method name and for example override paintcomponent(Graphics g) instead of paintComponent(Graphics g).

Consider import static

Often, it's plain obvious where static methods and constants are coming from. For example, in your context, it's plain obvious that RED is coming from class Color. In cases where it is plain obvious, import static can help making the source code more terse without losing clarity.

The Timer should only be running while the component is visible

So you should have a ComponentListener which starts the timer in componentShown() and stops the timer in componentHidden().

Refer to static symbols via the declaring class.

It's better to refer to symbols via the declaring class and not via any of the subclasses. For example, EXIT_ON_CLOSE is declared in interface WindowConstants. So it should be referred to as WindowConstants.EXIT_ON_CLOSE and not JFrame.EXIT_ON_CLOSE.

Prefer DISPOSE_ON_CLOSE over EXIT_ON_CLOSE.

It's better to use DISPOSE_ON_CLOSE then EXIT_ON_CLOSE. Yes, it is harder to program correctly with DISPOSE_ON_CLOSE, but that's exactly the intention: When your application doesn't properly stop and exit with DISPOSE_ON_CLOSE, there's a flaw (usually in the threading model). Using EXIT_ON_CLOSE instead of DISPOSE_ON_CLOSE just bypasses that flaw. That can even be error-prone if there is another thread in the application that's still busy doing important stuff (like saving a user's file). That thread would be terminated in the middle of its work.

Prefer main(String... args) over main(String[] args)

That's not really big and important. It's just easier for testing:

• With main(String[] args) a no args test-call is main(new String[0]).
• With main(String... args) a no args test-call is simply main().

Consider using final for variables as much as possible

It helps reducing programming mistakes and drives you towards the functional programming paradigm.

Split ScreenSaver in 2-3 classes

Actually ScreenSaver is doing more than one thing. It's displaying the date in a color of the user's liking, and it's enabling the user to pick the color of their liking. While initially it requires some thought of how to split this, in the end the code will become simpler if you achieve this.

Names: Is ScreenSaver really a screen saver? And is the Clock really a Clock?

Also, I would avoid using names that are in the API already, and there already is a class called Clock in the API. It isn't always possible (and sometimes not even intended) to avoid name-clashes, obviously. Just a thought.

Use Java 8 Date API

The classes Date, Calendar and such are actually horrible, and code that uses them is difficult to test. You may consider using the new Java 8 API for datetimes, java.time. It is really awesome.

Calling deriveFont() the right way

In case your font couldn't be loaded, the default font will have the default size instead of 30pt. I think that's not intended. It's better to call setFont(...) and then derive the font by setting it again using setFont(getFont().deriveFont(30f)).

Use the correct thread for the job

The setup of your application runs in the main thread. Ideally, modifications (and even reads) on Swing components happen only on the AWT Event Thread, unless the method that's called is explicitely documented as being thread-safe.

In your case it will probably not matter. But it's still safer and best practice to even create the UI not in the main thread but on the AWT Event Thread. The class SwingUtilities provides the methods invokeAndWait() and invokeLater() for that.

Your main() method could look like this:

public static void main(String... args) {
    SwingUtilities.invokeLater(() -> {
        JFrame frame = new JFrame();
        frame.setContentPane(new ScreenSaver());
        frame.setSize(685, 80);
        frame.setUndecorated(true);
        frame.setDefaultCloseOperation(DISPOSE_ON_CLOSE);
        frame.setBackground(Color.BLACK);
        frame.setResizable(false);
        frame.setVisible(true);
        frame.toFront();
    });
}

Or, putting the code for setting up the UI in a separate method, like this:

public static void main(String... args) {
    SwingUtilities.invokeLater(Clock::setupUI);
}

private static void setupUI() {
    JFrame frame = new JFrame();
    frame.setContentPane(new ScreenSaver());
    frame.setSize(685, 80);
    frame.setUndecorated(true);
    frame.setDefaultCloseOperation(DISPOSE_ON_CLOSE);
    frame.setBackground(Color.BLACK);
    frame.setResizable(false);
    frame.setVisible(true);
    frame.toFront();
}

Last but not least

Consider testing your application. The most popular test frameworks (test runners) for Java probably are JUnit, Cucumber and TestNG.

Answer 2

I'm guessing the ScreenSaver.java code you posted is missing some imports, so I'd suggest adding those in so we can see them. I'm also not sure what "DIGITALDREAMFAT.ttf" is, but I had to cut that whole section out to get the code to work properly, so I guess I'm missing out on some cool font. And then there's the JButton button that is declared and initialized, but never used again. Not sure if that's just a feature you haven't added yet or is just in there and was forgotten about. You also import JComboBox and JPanel in your Clock class but since that class does not use them but ScreenSaver does, I'd suggest moving those two lines over to that class.

The first thing that I notice about the program is that since you set your frame as undecorated, it makes it a little harder to close and I've yet to find a way to move it. I'm not sure if your intention is to only have it in the top left corner where you have to go out of your way to close it, but if not, or if you plan on having others use this, I'd suggest editing that.

One thing that Eclipse is yelling about with your code is the use of JComboBox. Your compiler should be giving you a warning or an error since you don't use any parameters. You can read about it here. I'd suggest you do this:

public JComboBox<String> colorList;

and

colorList = new JComboBox<>(colorName);

You should also limit your scope on your DateFormat and Date. Right now, they're static variables for the whole class, but you only use them in one function. I'd suggest just declaring your DateFormat and Date in the public void date() function rather than the entire class. This still works with your paintComponent(), as stringDate is still available to the whole class.

Another thing you could do that would cut down on lines and not affect performance is changing Clock from a Clock object to just a JFrame.

Instead of this:

public class Clock extends JFrame {

    public Clock () {
        setContentPane(new ScreenSaver());
    }

    public static void main(String[] args) {

        Clock clock = new Clock();
        //the rest of your code

All you need is this:

public class Clock{

    public static void main(String[] args) {

        JFrame clock = new JFrame();
        clock.getContentPane().add(new ScreenSaver());
        //the rest of your code

Both have the same effect and do what you want.

Just from messing around with it, it doesn't look like you need this line in your Clock class:

clock.setBackground(Color.BLACK);

I tested it with and without that line and it doesn't look like it made a difference. I'm guessing with all of the other setBackgrounds you have in ScreenSaver, it wasn't needed.

And this is just a nitpicky thing, but almost every time you set something's background color, you use setBackground(Color.BLACK) except for once when you set the background of panel. Just for consistency (unless (0, 0, 0, 0) is different from Color.BLACK and I just didn't see the difference), I'd use the same one every time.