HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

"I'm Listening"

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
listeningstackoverflowprogramming

Problem

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:

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);
}

Solution

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...!

Code Snippets

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;
}
b1.addActionListener(e -> {
    if (buttonOn) {
        b1.setText("Stop");
        b1.setBackground(Color.RED);
    } else {
        b1.setText("Go");
        b1.setBackground(Color.GREEN);
    }
});
slider.addChangeListener(e -> {
    JSlider s = (JSlider) e.getSource();
    label.setText(Integer.toString(s.getValue()));
});

Context

StackExchange Code Review Q#119685, answer score: 4

Revisions (0)

No revisions yet.