1
\$\begingroup\$

The below code is placed in a method . But it seems too large and I would like to shorten it. Is there a better way to do what it does?

ImageIcon switchoffIMG = new ImageIcon("switch1.jpg");
ImageIcon switchonIMG  = new ImageIcon("switch2.jpg");

JLabel switch1 = new JLabel(switchoffIMG);
JLabel switch2 = new JLabel(switchoffIMG);
JLabel switch3 = new JLabel(switchoffIMG); /* Will add these into different JPanels */

boolean switch1state, switch2state, switch3state;
switch1state = switch2state = switch3state = false;

switch1.addMouseListener(new MouseAdapter(){
    public void mouseClicked(MouseEvent e)
    {
        if(switch1state == false)
        {
            if((e.getX() >= OFFBUTTONLEFT && e.getX() <= OFFBUTTONRIGHT) && (e.getY() >= OFFBUTTONTOP && e.getY() <= OFFBUTTONDOWN))
                switch1state = true;
        }else
        {
            if((e.getX() >= ONBUTTONLEFT && e.getX() <= ONBUTTONRIGHT) && (e.getY() >= ONBUTTONTOP && e.getY() <= ONBUTTONDOWN))
                switch1state = false;
        }

        paintStuff();
    }
});

switch2.addMouseListener(new MouseAdapter(){
    public void mouseClicked(MouseEvent e)
    {
        if(switch2state == false)
        {
            if((e.getX() >= OFFBUTTONLEFT && e.getX() <= OFFBUTTONRIGHT) && (e.getY() >= OFFBUTTONTOP && e.getY() <= OFFBUTTONDOWN))
                switch2state = true;
        }else
        {
            if((e.getX() >= ONBUTTONLEFT && e.getX() <= ONBUTTONRIGHT) && (e.getY() >= ONBUTTONTOP && e.getY() <= ONBUTTONDOWN))
                switch2state = false;
        }

        paintStuff();
    }
});

switch3.addMouseListener(new MouseAdapter(){
    public void mouseClicked(MouseEvent e)
    {
        if(switch3state == false)
        {
            if((e.getX() >= OFFBUTTONLEFT && e.getX() <= OFFBUTTONRIGHT) && (e.getY() >= OFFBUTTONTOP && e.getY() <= OFFBUTTONDOWN))
                switch3state = true;
        }else
        {
            if((e.getX() >= ONBUTTONLEFT && e.getX() <= ONBUTTONRIGHT) && (e.getY() >= ONBUTTONTOP && e.getY() <= ONBUTTONDOWN))
                switch3state = false;
        }

        paintStuff();
    }
});

Additional information (if required):

public void paintStuff()
{
    if(switch1state)
    {
        switch1.setIcon(switchonIMG);
        left.add(bulb1);              //Add image to JPanel
    }else
    {
        switch1.setIcon(switchoffIMG);
        left.remove(bulb1);           //Remove image from JPanel
    }
    if(switch2state)
    {
        switch2.setIcon(switchonIMG);
        mid.add(bulb2);               //Add image to JPanel
    }else
    {
        switch2.setIcon(switchoffIMG);
        mid.remove(bulb2);            //Remove image from JPanel
    }
    if(switch3state)
    {
        switch3.setIcon(switchonIMG);
        right.add(bulb3);             //Add image to JPanel
    }else
    {
        switch3.setIcon(switchoffIMG);
        right.remove(bulb3);          //Remove image from JPanel
    }
    repaint();
}

and

final static int OFFBUTTONTOP   = 75;
final static int OFFBUTTONLEFT  = 30;
final static int OFFBUTTONRIGHT = 65;
final static int OFFBUTTONDOWN  = 115;

final static int ONBUTTONTOP   = 35;
final static int ONBUTTONLEFT  = 25;
final static int ONBUTTONRIGHT = 60;
final static int ONBUTTONDOWN  = 75;
\$\endgroup\$
0

1 Answer 1

Reset to default
2
\$\begingroup\$ 
boolean switch1state, switch2state, switch3state;

These three variables are a good start to write everything three times. But then it gets too long... Numbered variables are usually a code smell. Wouldn't an array do?

switch1state = switch2state = switch3state = false;

Useless, just drop it.

        if(switch1state == false)
        {
            if((e.getX() >= OFFBUTTONLEFT && e.getX() <= OFFBUTTONRIGHT) && (e.getY() >= OFFBUTTONTOP && e.getY() <= OFFBUTTONDOWN))
                switch1state = true;
        }else
        {
            if((e.getX() >= ONBUTTONLEFT && e.getX() <= ONBUTTONRIGHT) && (e.getY() >= ONBUTTONTOP && e.getY() <= ONBUTTONDOWN))
                switch1state = false;
        }

After splitting the "if-else" into two "if"-s and a simple reordering the conditions gets clearer:

        if((e.getX() >= OFFBUTTONLEFT && e.getX() <= OFFBUTTONRIGHT) && (e.getY() >= OFFBUTTONTOP && e.getY() <= OFFBUTTONDOWN))
            if(switch1state == false) {
                switch1state = true;
            }
        if((e.getX() >= ONBUTTONLEFT && e.getX() <= ONBUTTONRIGHT) && (e.getY() >= ONBUTTONTOP && e.getY() <= ONBUTTONDOWN))
            if(switch1state == true) {
                switch1state = false;
            }
        }

and we see that's repetitive without any reason. Moreover, spacing is wrong and things like switch1state == false should be written as !switch1state. But we don't need it, anyway.

        if ((e.getX() >= OFFBUTTONLEFT && e.getX() <= OFFBUTTONRIGHT) && (e.getY() >= OFFBUTTONTOP && e.getY() <= OFFBUTTONDOWN)) {
            switch1state = true;                
        } else if ((e.getX() >= ONBUTTONLEFT && e.getX() <= ONBUTTONRIGHT) && (e.getY() >= ONBUTTONTOP && e.getY() <= ONBUTTONDOWN))
            switch1state = false;
        }

But wait... your rectangles seem to overlap, so it's wrong. Then let's resort to my basic rules:

  • never copy stuff
  • if you ever do it, never copy long stuff

So what about

if (!switch1state && isInRectangle(ON_RECTANGLE, e)) {
    switch1state = true;
} else if (switch1state && isInRectangle(OFF_RECTANGLE, e)) {
    switch1state = false;
}

There's a class Rectangle and it has a method to determine if a Point is in it, which you can use.

With an array you can do simply this (the annotation come from Lombok and does exactly what it states; just in case you want to do it manually)

@RequiredArgsConstructor class MyMouseListener extends MouseAdapter {
    private final int index;

    public void mouseClicked(MouseEvent e) {
        if (!switchstate[index] && isInRectangle(OFF_RECTANGLE, e)) {
                switch1state = true;
        } else if (switchstate[index] && isInRectangle(ON_RECTANGLE, e)) {
                switch1state = false;
        }
        paintStuff();
    }
}

paintStuff

if(switch1state)
{
    switch1.setIcon(switchonIMG);
    left.add(bulb1);              //Add image to JPanel
}else
{
    switch1.setIcon(switchoffIMG);
    left.remove(bulb1);           //Remove image from JPanel
}

This can be shortened a bit like

switches[0].setIcon(switchstate[0] ? switchonIMG : switchoffIMG);
if (switchstate[0]) {
    leftMidRight[0].add(bulb[0]);              //Add image to JPanel
} else {
    leftMidRight[0].remove(bulb[0]);           //Remove image from JPanel
}

using three more arrays

Button[] switches[] = {switch1, switch2, switch3};
JPanel[] leftMidRight = {left, mid, right};
Icon[] bulb = {bulb1, bulb2, bulb2};

and now it's ready for a loop.

constants

 final static int OFFBUTTONTOP   = 75;

Wedonotwriteconstantslikethis. Ihopethereasonisclearnow. Use

 private final static Rectangle OFF_RECTANGLE = ...
\$\endgroup\$
5
  • \$\begingroup\$ Awesome answer. I feel so dumb not thinking about arrays. Thanks a lot! One doubt. Why is switch1state = switch2state = switch3state = false; useless? (In my original program, switch1state, switch2state, switch3state,switch1,switch2,switch3,switchoffIMG,switchonIMG are fields of my class) \$\endgroup\$ Commented Jun 12, 2015 at 9:26
  • \$\begingroup\$ @CoolGuy In Java all fields get initialized to some form of zero (0, 0.0, false, or null). \$\endgroup\$ Commented Jun 12, 2015 at 9:46
  • \$\begingroup\$ Can ImageIcon or Icon be directly added to a JPanel? Or is it better to stick with the JLabels as in my code? \$\endgroup\$ Commented Jun 12, 2015 at 10:46
  • \$\begingroup\$ And here is my full code. I hope it looks good. \$\endgroup\$ Commented Jun 15, 2015 at 4:25
  • \$\begingroup\$ @CoolGuy I wasn't here. You can't add icons directly, so I guess using labels is fine. I'd probably handle the bulbs just like the switches, i.e. a label with bulbOn or bulbOff (empty) icon. You've got already a review on your new code and I agree with janos' objections, but in any case, it's a huge improvement. \$\endgroup\$ Commented Jun 18, 2015 at 22:28

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.