Problem with Java GUI implementation

Question

I have a problem with the code I am currently trying to run - I am trying to make 3 buttons, put them on a GUI, and then have the first buttons colour be changed to orange, and the buttons next to that colour change to white and green. Every click thereafter will result in the colours moving one button to the right. My code thus far is as follows, it is skipping colours in places and is not behaving at all as I expected. Can anyone offer some help/guidance please ?

import java.awt.*;
import javax.swing.*;
import java.awt.event.*;

public class ButtonJava extends JButton implements ActionListener  {
  private int currentColor=-1;
  private int clicks=0;
  private static final Color[] COLORS = {
    Color.ORANGE,
    Color.WHITE,
    Color.GREEN };
  private static ButtonJava[] buttons;

  public ButtonJava( ){
    setBackground( Color.YELLOW );
    setText( "Pick ME" );
    this.addActionListener( this );
  }

  public static void main(String[] args) {
    JFrame frame = new JFrame ("JFrame");
    JPanel panel = new JPanel( );
    frame.setDefaultCloseOperation( JFrame.EXIT_ON_CLOSE);
    buttons = new ButtonJava[3];
    for(int i = 0;i<buttons.length ; i++){
      buttons[i] = new ButtonJava(); 
      panel.add(buttons[i]);
    }
    frame.getContentPane( ).add( panel );
    frame.setSize( 500, 500);
    frame.setVisible( true );
  }

  private void updateButton() {
     clicks++;
    changeColors();
//    setText( );
  }

private void changeColors( ) {
  for (int i=buttons.length-1;i>=0;i--){
    buttons[i].currentColor = nextColor(currentColor);
    buttons[i].setBackground(COLORS[buttons[i].currentColor]);
    buttons[i].setText(("# of clicks = " + buttons[i].getClicks() ) );
  }
}

private Integer getClicks() {
 return clicks;
}

private int nextColor( int curCol ) {
  final int colLen = COLORS.length;
  curCol--;
  curCol = (colLen + curCol % colLen) % colLen;
  return curCol;
}

private void firstClick( ActionEvent event ) {
  int curCol = 0;
  for (int i=buttons.length-1;i>=0;i--){
    if ( buttons[i] == event.getSource() ) {
      buttons[i].currentColor = curCol;
      curCol++;
      currentColor++;
    }
  }}

  @Override
  public void actionPerformed( ActionEvent event ) {
    if ( -1 == currentColor ) {
      firstClick( event );
    }
    updateButton( );   
  }


}

Thank you very much for the help :)

The OP already has 4 suggestions from posting this question yesterday: stackoverflow.com/questions/4938149/… — camickr
– camickr, Commented Feb 9, 2011 at 21:23
@camickr: same code, different problem. Agreed though that its better off as an edit to yesterday's question. — Mac
– Mac, Commented Feb 9, 2011 at 21:34
@camickr I fixed the suggestions that were made there, it still doesn't work, hence I am looking for more help... — user476033
– user476033, Commented Feb 9, 2011 at 21:35
@user476033: it's often better to update the original question with the details of your new problem. That way, the people who are already familiar with your code will have easier access, and there's less volume of questions for people to sort through. — Mac
– Mac, Commented Feb 9, 2011 at 21:40
@Mac: Thanks for the constructive feedback, I will bear that in mind from now on ! :) — user476033
– user476033, Commented Feb 9, 2011 at 21:46

unholysampler · Accepted Answer · 2011-02-10 22:43:13Z

3

You have a couple issues with the code you posted, but they generally boil down to being clear about what is a member of the class(static) and what is a member of the instance.

For starters, you buttons array only exists inside your main method and can't be accessed by changeColors(). Along those same lines, since changeColors() is an instance method, setBackground() needs to be called directly on the button in your array. as written you are setting the color for one button 3 times.

Additionally, the logic in changeColors() is not properly rotating the currentColor index. You need to both increase the counter and ensure is wraps for the length of the color array. If the arrays are the same size, you need to make sure there is an extra addition to make the colors cycle.

private static void changeColors( ) {
  for (int i=0;i<buttons.length;i++){
    buttons[i].setBackground(COLORS[currentColor]);
    currentColor = nextColor(currentColor);
  }
  if (buttons.length == COLORS.length) {
    currentColor = nextColor(currentColor);
  }
}

private static int nextColor(int currentColor) {
  return (currentColor+1)% COLORS.length;
}

Edit for new code:

I'm not sure why you re-wrote nextColor(), as what I posted worked. But in general, I feel like you are running into issues because your code is not well partitioned for the tasks you are trying to achieve. You have code related to the specific button instance and code related to controlling all the buttons mixing together.

With the following implementation, the issue of how many times a button was clicked is clearly self-contained in the button class. Then every button press also calls the one method in the owning panel. This method knows how many buttons there are and the color of the first button. And each subsequent button will contain the next color in the list, wrapping when necessary.

public class RotateButtons extends JPanel {
  private static final Color[] COLORS = { Color.ORANGE, Color.WHITE, Color.GREEN };
  private static final int BUTTON_COUNT = 3;
  private JButton[] _buttons;
  private int _currentColor = 0;

  public static void main(String[] args)
  {
    JFrame frame = new JFrame("JFrame");
    frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);
    frame.getContentPane().add(new RotateButtons());
    frame.setSize(500, 500);
    frame.setVisible(true);
  }

  public RotateButtons()
  {
    _buttons = new JButton[BUTTON_COUNT];
    for (int i = 0; i < _buttons.length; i++) {
      _buttons[i] = new CountButton();
      add(_buttons[i]);
    }
  }

  private void rotateButtons()
  {
    for (JButton button : _buttons) {
      button.setBackground(COLORS[_currentColor]);
      _currentColor = nextColor(_currentColor);
    }
    if (_buttons.length == COLORS.length) {
      _currentColor = nextColor(_currentColor);
    }
  }

  private int nextColor(int currentColor)
  {
    return (currentColor + 1) % COLORS.length;
  }

  private class CountButton extends JButton {
    private int _count = 0;

    public CountButton()
    {
      setBackground(Color.YELLOW);
      setText("Pick ME");
      addActionListener(new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent arg0)
        {
          _count++;
          setText("# of clicks = " + _count);
          rotateButtons();
        }
      });
    }
  }
}

2nd Edit:

Shows just the changes to shift _currentColor by the necessary amount on the first click.

public class RotateButtons extends JPanel {
  ...
  private boolean _firstClick = true;
  ...
  private void rotateButtons(CountButton source)
  {
    if (_firstClick) {
      _firstClick = false;
      boolean foundSource = false;
      for (int i = 0; i < _buttons.length; i++) {
        if (foundSource) {
          _currentColor = nextColor(_currentColor);
        } else {
          foundSource = _buttons[i] == source;
        }
      }
    }
    ...
  }

  private class CountButton extends JButton {
    ...

    public CountButton()
    {
      ...
      addActionListener(new ActionListener() {
        @Override
        public void actionPerformed(ActionEvent arg0)
        {
          ...
          rotateButtons(CountButton.this);
        }
      });
    }
  }

edited Feb 10, 2011 at 22:43

answered Feb 9, 2011 at 21:25

unholysampler

17.4k7 gold badges51 silver badges65 bronze badges

Sign up to request clarification or add additional context in comments.

9 Comments

Mac Over a year ago

+1: I should have realised that buttons was local to main. Well spotted.

user476033 Over a year ago

Thanks for this response ! One thing I am having trouble with though, I need to make the buttons array visible in the changeColours() method...what is the cleanest way to go about doing this ? Thank you very much ! :)

unholysampler Over a year ago

When I was testing the code, I made the array a static member of the class. However, in a real implementation, it would makes sense to have the class that owns the panel also own the array. In fact I would not extend JButton and put all the code in a subclass of JPanel. This way you have one panel that knows how buttons on it work, rather than a button class that assumes certain things about itself and forces the class that uses it to use it correctly.

unholysampler Over a year ago

@user476033: I updated my answer with a full implementation. If it still isn't what you are looking for, try to specifically say what it is doing and why that is wrong for your end goal.

unholysampler Over a year ago

@user476033: New edit to show how to pass in the button that triggered the event and adjust the current color on the first click.

|

DJ. · Accepted Answer · 2011-02-09 21:21:21Z

1

One thing that I noticed is you are using currentColor to assign the color, but the currentColor is initialized to 0 and the only manipulation is currentColor %= 2 which does not change it.

answered Feb 9, 2011 at 21:21

DJ.

6,7842 gold badges36 silver badges49 bronze badges

Comments

Mac · Accepted Answer · 2011-02-09 21:36:49Z

1

If I'm understanding what you are wanting to achieve, I'm thinking to change currentColor %= 2 to ++currentColor, and setBackground(COLORS[currentColor]); to buttons[i].setBackground(COLORS[(i + currentColor) % 3]);. That way, your colours should cycle around your buttons each time one is clicked.

EDIT: it's probably also worth calling changeColors from within main to initialise your button colours. And, as @unholysampler notes, your array buttons is local to main and should (for example) be refactored as a static member variable, and have changeColors become a static method.

edited Feb 9, 2011 at 21:36

answered Feb 9, 2011 at 21:30

Mac

14.8k11 gold badges65 silver badges83 bronze badges

Collectives™ on Stack Overflow

Problem with Java GUI implementation

3 Answers 3

9 Comments

Comments

Comments

Your Answer

Linked

Hot Network Questions

Collectives™ on Stack Overflow

3 Answers 3

9 Comments

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Linked

Related