7
\$\begingroup\$

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

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

        top.setBackground(Color.BLACK);
        bot.setBackground(Color.BLACK);

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

        goToRoom = new JButton("Go to room", new ImageIcon("door_closed.jpg"));
        goToRoom.addActionListener(this);
        goToRoom.setRolloverIcon(new ImageIcon("door_open.jpg"));

        bot.add(goToRoom);

        card1.add(top, BorderLayout.CENTER);
        card1.add(bot, BorderLayout.SOUTH);
    }

    public void initCard2()
    {
        JPanel top = new JPanel(new GridLayout(1, 0));
        JPanel bot = new JPanel(new FlowLayout(FlowLayout.CENTER));

        for(int i=0; i<bulbPans.length; i++)
        {
            bulbPans[i] = new JPanel();
            bulbPans[i].setBackground(Color.BLACK);
        }

        card2 = new JPanel(new BorderLayout());

        for(int i=0; i<bulbs.length; i++)
            bulbs[i] = new JLabel(new ImageIcon("bulb.jpg", "Image of a bulb"));

        bot.setBackground(Color.BLACK);

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

        paintStuff();

        back = new JButton("Back to switches",new ImageIcon("door_closed.jpg"));
        back.addActionListener(this);
        back.setRolloverIcon(new ImageIcon("door_open.jpg"));

        bot.add(back);

        card2.add(top, BorderLayout.CENTER);
        card2.add(bot, BorderLayout.SOUTH);
    }

    public void setCards()
    {
        cards = new JPanel(new CardLayout()); 
        cards.add(card1, "CARD1");
        cards.add(card2, "CARD2");

    }

    public void paintStuff()
    {
        for(int i=0; i<switchstate.length; i++)
        {
            if(switchstate[i])
                bulbPans[i].add(bulbs[i]);
            else
                bulbPans[i].remove(bulbs[i]);
            switches[i].setIcon(switchstate[i]?switchonIMG:switchoffIMG);
        }
        repaint();
    }

    public void actionPerformed(ActionEvent evt) {
        CardLayout c = (CardLayout)(cards.getLayout());
        c.next(cards);
    }

}

interface Constants
{
    final static Rectangle OFF_RECTANGLE = new Rectangle(30, 75, 35, 40); 
    final static Rectangle ON_RECTANGLE  = new Rectangle(25, 35, 35, 30);
}

Output:

GIF_OUTPUT

Is there any room for improvement?

\$\endgroup\$
8
  • \$\begingroup\$ How come turning the switch "off" turns the bulb "on"? \$\endgroup\$
    – JS1
    Jun 13, 2015 at 13:30
  • \$\begingroup\$ @JS1 , huh? Where? \$\endgroup\$
    – Spikatrix
    Jun 13, 2015 at 14:11
  • \$\begingroup\$ @CoolGuy When something is "down" it's typically off \$\endgroup\$
    – anon
    Jun 13, 2015 at 14:30
  • \$\begingroup\$ @QPaysTaxes , Do you mean that when the program starts, all switches are in the "on" position? \$\endgroup\$
    – Spikatrix
    Jun 13, 2015 at 14:32
  • 1
    \$\begingroup\$ @JS1 and QPaysTaxes, It is the opposite where I live. Looks fine to me. \$\endgroup\$
    – Spikatrix
    Jun 13, 2015 at 14:42

1 Answer 1

7
\$\begingroup\$

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();
            }
        });
    }
\$\endgroup\$
7
  • \$\begingroup\$ Thanks for taking the time to review my program. What do you suggest a name for the constant needed to clear "magic numbers"? And I don't like enhanced for loop much which is why I'm using normal loops. As for naming, I really don't care. \$\endgroup\$
    – Spikatrix
    Jun 13, 2015 at 14:17
  • \$\begingroup\$ "I don't like enhanced for loop much" This is worrying. They're a very important part of the language and help readability a ton (at least for people who know Java). Learn to love them. It's gonna be a lot easier in the end. "As for naming, I really don't care" This is incredibly worrying. You should care. Always care about naming. You may get it now and never touch this again, but get into good habits early and you won't have to break them later, when it'll be a billion times harder. CARE ABOUT NAMING. \$\endgroup\$
    – anon
    Jun 13, 2015 at 14:45
  • \$\begingroup\$ @QPaysTaxes , I usually program in C and there aren't any for-each loops in C (which explains why I don't like for-each loops). I'll try to care about the naming.... Thanks anyway. \$\endgroup\$
    – Spikatrix
    Jun 13, 2015 at 14:49
  • \$\begingroup\$ Hey @CoolGuy, thanks for the suggested edit, the guys were a bit overly cautious, but it was mostly legit and I applied your suggestions. About for-each, it's a natural way of iterating over elements, without needing to worry about indexes too. Another great benefit is that it works with any kind of collection, for example a linked list, where indexing would be costly. Naming is crucial for future readability. Keep in mind that code is read far more often than written, so it pays to spend time for read-time convenience \$\endgroup\$
    – janos
    Jun 13, 2015 at 15:03
  • \$\begingroup\$ @janos , Never mind about the rejected edit. Even I think that is wasn't very good. Ok. I'll try to use for-each loops and better names. Can you suggest a good name for the constant needed to clear "magic numbers"? The first name that sprang to my mind was "number". Doesn't look to good.... \$\endgroup\$
    – Spikatrix
    Jun 13, 2015 at 15:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.