5
\$\begingroup\$

For this next assignment, my job was to create a GUI with a button that changes text and color when clicked, and a slider that updates a label (with a TitledBorder) based on its value. Right now it looks like this:

enter image description here

I've made GUI's before, but it's been a few years and I know that the language has updated since then. Any suggestions for improvements? Perhaps ways to make it look more visually appealing?


package listeners;

import java.awt.Color;
import java.awt.Dimension;
import java.awt.FlowLayout;
import java.awt.Font;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import javax.swing.BorderFactory;
import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JSlider;
import javax.swing.UIManager;
import javax.swing.event.ChangeEvent;
import javax.swing.event.ChangeListener;

/**
 *  This class will give you practice writing GUI components and listeners.
 *  @author syb0rg
 */
public class Listeners
{

    public Listeners() 
    {
        JFrame frame = new JFrame("Listeners Lab");
        frame.setPreferredSize(new Dimension(450, 150));
        frame.setLayout(new FlowLayout());

        // cause my Mac was rendering things weird
        try 
        {
            UIManager.setLookAndFeel( UIManager.getCrossPlatformLookAndFeelClassName() );
        } catch (Exception e) 
        {
            e.printStackTrace();
        }

        final JButton b1 = new JButton("Stop");
        b1.setPreferredSize(new Dimension(100, 50));
        b1.setFont(Font.getFont(Font.SANS_SERIF));
        b1.setBackground(Color.RED);
        b1.setVisible(true);
        frame.add(b1);

        b1.addActionListener(new ActionListener() 
        {
            @Override
            public void actionPerformed(ActionEvent e) 
            {
                if (b1.getText().equals("Stop"))
                {
                    b1.setText("Go");
                    b1.setBackground(Color.GREEN);
                }
                else
                {
                    b1.setText("Stop");
                    b1.setBackground(Color.RED);
                }

            }
        });

        final JSlider slider = new JSlider();
        final JLabel label = new JLabel("50");
        label.setPreferredSize(new Dimension(60, 50));
        label.setFont(Font.getFont(Font.SANS_SERIF));
        label.setForeground(Color.BLUE);
        label.setBorder(BorderFactory.createTitledBorder("Value"));
        slider.addChangeListener(new ChangeListener() 
        {
            @Override
            public void stateChanged(ChangeEvent e) 
            {
                JSlider s = (JSlider) e.getSource();
                label.setText(Integer.toString(s.getValue()));
            }
        });
        frame.add(slider);
        frame.add(label);


        frame.pack();
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
        frame.setVisible(true);
    }

    /**
     * @param args the command line arguments -- not used
     */
    public static void main(String... args) 
    {
        new Listeners();
    }
}
\$\endgroup\$
2
\$\begingroup\$

Lazy intialization / factory methods

You currently separate the component creation process by immediately instantiating the component at the variable declaration and configuring the component elsewhere.

Component creation and configuration should be encapsulated in lazy getters like this:

private JButton stopButton;

private JButton getStopButton() {
    if (stopButton == null) {
        stopButton = new JButton("Stop");
        stopButton.setPreferredSize(new Dimension(100, 50));
        stopButton.setFont(Font.getFont(Font.SANS_SERIF));
        stopButton.setBackground(Color.RED);
        stopButton.setVisible(true);
        stopButton.addActionListener(new ActionListener() {
            @Override
            public void actionPerformed(ActionEvent e) {
                if (stopButton.getText().equals("Stop")) {
                    stopButton.setText("Go");
                    stopButton.setBackground(Color.GREEN);
                } else {
                    stopButton.setText("Stop");
                    stopButton.setBackground(Color.RED);
                }

            }
        });
    }
    return stopButton;
}

If you do not use the variable anywhere else you may consider to only have a factory method:

private JButton createStopButton() {
    final JButton b1 = new JButton("Stop");
    b1.setPreferredSize(new Dimension(100, 50));
    b1.setFont(Font.getFont(Font.SANS_SERIF));
    b1.setBackground(Color.RED);
    b1.setVisible(true);
    b1.addActionListener(new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent e) {
            if (b1.getText().equals("Stop")) {
                b1.setText("Go");
                b1.setBackground(Color.GREEN);
            } else {
                b1.setText("Stop");
                b1.setBackground(Color.RED);
            }

        }
    });
    return b1;
}

There are at least two main points this procedure addresses.

  1. Having the the whole construction at one place
  2. If you use the lazy getter only you do not have to care about the creation time. You are more flexible when rearranging the components

I recommend this because of the the points mentioned AND the following argument:

I saw several GUI editors that generated code like this. As the code must be EASILY parseable by the GUI editor this seems to be an easy to parse structure. And what are we developers doing when we read code? We are parsing it!

Shutdown strategy

Because you are using Swing you currently define following to close your application:

setDefaultCloseOperation(EXIT_ON_CLOSE);

I do not recommend this. Any pending or working thread will be aborted. If this is your intention then you have no well-defined shutdown as the threads may be in an arbitrary state (writing to harddisk, communicating with remote services etc.).

You will also hide programming errors if some threads have corrupt shutdown mechanisms.

So make sure to notify running threads to shutdown if you want to exit the program. The following Swing configuration for a frame will disassamble the UI and exits the JVM if no other frame exists:

setDefaultCloseOperation(DISPOSE_ON_CLOSE);

Missing model

You are totally missing a model. I don't know if you have the intention to introduce one. But setting the text and the background color of "b1" directly in the ActionListener is across country.

I'd expect the ActionListener call a model that will change. The change will be populated to "b1" again through another listener mechanism so the ActionListener isn't aware of what has to be changed in the UI.

You really see the missing concept of a model by looking at following statement:

if (stopButton.getText().equals("Stop")) { ...

You are asking the UI component for a state. This state should be encoded in a model.

As the code provided seems to be incomplete in the context of all requirements of your customer it is hard to argue for a model because currently you will not see the big advantage for this procedure.

That why I provide an example where the model drives the UI in a little more complex way.

Main.java

public class Main {

    public static void main(String[] args) {
        MyModel myModel = new MyModel();
        MyFrame myFrame = new MyFrame(myModel);
        myFrame.setVisible(true);
    }

}

MyFrame.java

import java.awt.BorderLayout;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

import javax.swing.JButton;
import javax.swing.JFrame;
import javax.swing.JTextField;

public class MyFrame extends JFrame {

    private final MyModel myModel;

    private JButton nextWordButton;
    private WordTextField wordTextField;


    public MyFrame(MyModel myModel) {
        this.myModel = myModel;
        setDefaultCloseOperation(DISPOSE_ON_CLOSE);
        getContentPane().setLayout(new BorderLayout());
        getContentPane().add(getNextWordButton(), BorderLayout.WEST);
        getContentPane().add(getWordTextField(), BorderLayout.CENTER);
        setSize(400, 200);
    }


    private JButton getNextWordButton() {
        if (nextWordButton == null) {
            nextWordButton = new JButton("Next");
            nextWordButton.addActionListener(new ActionListener() {
                @Override
                public void actionPerformed(ActionEvent e) {
                    myModel.execute();
                }
            });
        }
        return nextWordButton;
    }


    private WordTextField getWordTextField() {
        if (wordTextField == null) {
            wordTextField = new WordTextField();
            myModel.add(wordTextField);
        }
        return wordTextField;
    }

}

MyModel.java

import java.util.HashSet;
import java.util.Set;

public class MyModel {


    private static final String[] INFINITE_SENTENCE = new String[] {"This", "sentence", "will", "never", "end", "because ..."};


    private int index;
    private Set<MyModelListener> listeners;

    public MyModel() {
        this.index = 0;
        this.listeners = new HashSet<>();
    }

    public void execute() {
        this.index++;
        notifyOnExecute(getWord());
    }

    private String getWord() {
        return INFINITE_SENTENCE[this.index % INFINITE_SENTENCE.length];
    }

    private void notifyOnExecute(String word) {

        for (MyModelListener listener: this.listeners) {
            listener.onExecute(word);
        }

    }

    public boolean add(MyModelListener l) {
        l.onListenerRegistration(getWord());
        return listeners.add(l);
    }

    public boolean remove(MyModelListener l) {
        return listeners.remove(l);
    } 


}

MyModelListener.java

public interface MyModelListener {

    void onExecute(String word);

    void onListenerRegistration(String word);

}

WordTextField

public class WordTextField extends JTextField implements MyModelListener {

    @Override
    public void onExecute(String word) {
        setText(word);
    }


    @Override
    public void onListenerRegistration(String word) {
        setText(word);
    }

}
\$\endgroup\$
  • \$\begingroup\$ Could you go a bit more in depth about the modeling aspect of your review? You make a compelling argument that I should use one, but don't give any hints on how to use one "well". \$\endgroup\$ – syb0rg Feb 11 '16 at 23:24
  • \$\begingroup\$ Separating model from UI is the way to keep your logic independent of technology. This will help you in future tec decisions and in testing as well. As I said: The provided code does nothing than a flop. So I provided an example that shows the interaction of model an UI. Using a model will make the UI technology interchangeable so my example. Throw away swing, JavaFX wil work as well. Testing the model is easy as well. \$\endgroup\$ – oopexpert Feb 12 '16 at 0:42
4
\$\begingroup\$

There is only one thing that I could point out: you are relying on the label of the button to determine whether it is "on" or "off". Instead, it would be better to introduce a boolean for this, like buttonOn.

This way, you could have the following, which I find more robust:

private boolean buttonOn = true;

// ...

@Override
public void actionPerformed(ActionEvent e) 
{
    if (buttonOn)
    {
        b1.setText("Stop");
        b1.setBackground(Color.RED);
    }
    else
    {
        b1.setText("Go");
        b1.setBackground(Color.GREEN);
    }
    buttonOn = !buttonOn;
}

Of course, if you are using Java 8, you could write this easier using lambda expressions:

b1.addActionListener(e -> {
    if (buttonOn) {
        b1.setText("Stop");
        b1.setBackground(Color.RED);
    } else {
        b1.setText("Go");
        b1.setBackground(Color.GREEN);
    }
});

and

slider.addChangeListener(e -> {
    JSlider s = (JSlider) e.getSource();
    label.setText(Integer.toString(s.getValue()));
});

As a side-note, I'm not a big fan of the style of curly braces you are using and prefer the K&R style but what's important is that it is consistent through your code.


Concerning the issue of making it more visually appealing... I'll leave that for someone else to review, nice graphics are not my strong point...!

\$\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.