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

Switch-Bulb GUI program

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

Problem

Here is my switch bulb GUI program:

```
import javax.swing.JFrame;
import javax.swing.JPanel;
import javax.swing.JButton;
import javax.swing.JLabel;
import javax.swing.ImageIcon;
import javax.swing.SwingUtilities;

import java.awt.Color;
import java.awt.Rectangle;
import java.awt.CardLayout;
import java.awt.FlowLayout;
import java.awt.GridLayout;
import java.awt.BorderLayout;

import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;
import java.awt.event.MouseEvent;
import java.awt.event.MouseAdapter;

public class SeventhProgram
{
public static void main(String[] args)
{
SwingUtilities.invokeLater(new Runnable() {
public void run() {
GUI program = new GUI("Switches and bulbs");
program.setSize(400,300);
program.setLocationRelativeTo(null);
program.setVisible(true);
}
});
}
}

class GUI extends JFrame implements Constants, ActionListener
{
JPanel cards;
JPanel card1, card2;
JPanel[] bulbPans = new JPanel[3];

JButton goToRoom, back;
JLabel[] switches = new JLabel[3];
ImageIcon switchonIMG, switchoffIMG;
JLabel[] bulbs = new JLabel[3];

boolean[] switchstate = new boolean[3];

public GUI(String arg)
{
super(arg);
setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
setResizable(false);

init();

add(cards);
}

public void init()
{
initCard1();
initCard2();
setCards();
}

public void initCard1()
{
JPanel top = new JPanel(new FlowLayout(FlowLayout.CENTER));
JPanel bot = new JPanel(new FlowLayout(FlowLayout.CENTER));

card1 = new JPanel(new BorderLayout());

switchoffIMG = new ImageIcon("switch1.jpg", "OFF switch");
switchonIMG = new ImageIcon("switch2.jpg", "ON switch ");

for(int i=0; i<switches.length; i++)
switches[i] = new JLabel(switchoffIMG);

Solution

Avoid magic numbers

Number 3 appears in many places, for example:

JPanel[] bulbPans = new JPanel[3];
JLabel[] switches = new JLabel[3];
JLabel[] bulbs = new JLabel[3];


But what's even worse is that it's critical that it's the same number in all these places:
if you change only one of them and forget the others (including many others I didn't include in the above list),
you risk breaking the program.

Give this number a descriptive name by turning it into a constant.

Use interfaces only to define types

The Constants interface is a classic example of what not to do.
The constants you put in here should be moved to the class where they are used.
Interfaces are designed to be implemented,
and to define the contract of behaviors.
Using an interface just to keep some constants is a classic poor use.

Use more enhanced for-each loops

Replace counting loops with enhanced for-each loop whenever possible.
For example instead of this:

for (int i = 0; i < switches.length; i++) {
    top.add(switches[i]);
}


Write like this:

for (JLabel bulbSwitch : switches) {
    top.add(bulbSwitch);
}


Naming

You really need better names.
SeventhProgram, GUI, paintStuff are "classic" bad names.

Code style

You're not formatting the common style encouraged by modern IDEs,
and commonly seen in open source projects.
This is how my IDE reformats your code:

for (int i = 0; i < switchstate.length; i++) {
        final int j = i;
        switches[j].addMouseListener(new MouseAdapter() {
            public void mouseClicked(MouseEvent e) {
                if (switchstate[j]) {
                    if (ON_RECTANGLE.contains(e.getX(), e.getY())) {
                        switchstate[j] = false;
                    }
                } else {
                    if (OFF_RECTANGLE.contains(e.getX(), e.getY())) {
                        switchstate[j] = true;
                    }
                }
                paintStuff();
            }
        });
    }

Code Snippets

JPanel[] bulbPans = new JPanel[3];
JLabel[] switches = new JLabel[3];
JLabel[] bulbs = new JLabel[3];
for (int i = 0; i < switches.length; i++) {
    top.add(switches[i]);
}
for (JLabel bulbSwitch : switches) {
    top.add(bulbSwitch);
}
for (int i = 0; i < switchstate.length; i++) {
        final int j = i;
        switches[j].addMouseListener(new MouseAdapter() {
            public void mouseClicked(MouseEvent e) {
                if (switchstate[j]) {
                    if (ON_RECTANGLE.contains(e.getX(), e.getY())) {
                        switchstate[j] = false;
                    }
                } else {
                    if (OFF_RECTANGLE.contains(e.getX(), e.getY())) {
                        switchstate[j] = true;
                    }
                }
                paintStuff();
            }
        });
    }

Context

StackExchange Code Review Q#93500, answer score: 7

Revisions (0)

No revisions yet.