# How can I make placing JButtons from an array more elegant?

I am trying to place JButtons from an array onto a JFrame. The way I'm doing it is having it test how many more buttons are left, and if buttons have been placed at the edge of the frame. The end result is an ugly piece of code.

JButton[] grid = new JButton[2501];

JFrame MapFrame = new JFrame();

public void makeMap() {

MapFrame.setBounds(40, 0, 750, 773);

int x = 0;
int y = 0;
for (int i = 0; i < grid.length; i++) {
// grid is the JButton Array

if (x > 749) {
x = 0;
y = y + 15;
}
grid[i] = new JButton();
grid[i].setBounds(x, y, 15, 15);

x = x + 15;
}
MapFrame.setVisible(true);
MapFrame.repaint();
}


The code just looks so bulky with variables being changed in different braces, and with so many braces. How could I make this more elegant? (Please don't recommend layouts, as, none of them fit my requirements.)

• Make 15 a constant, calculate 749 from the MapFrame.bounds and it will be fine. – Sulthan Sep 11 '12 at 9:50
• not to use GridLayout – mKorbel Sep 12 '12 at 10:33

In order to find a more elegant solution, we first need to identify the problems; only then can we solve them:

• Magic Numbers
One of the confusing things about this snippet is that we constantly (pun intended) come across magic values such as 15 and 749. What if the map gets bigger in the future, or the dimensions of the buttons change? Solution: Define constants.
Note: I used the Java naming convention for constants, which is SHOUTY_CASE, although I dislike it, because unified coding standards when sharing code trump personal preferences.

private static final int NUMBER_OF_BUTTONS = 2501;
private static final int BUTTON_SIDE = 15;

• Unnecessary use of array
Why are you copying every button into your grid before adding them to the mapFrame? If you aren't accessing them from the array later on, you can get rid of all the access-by-index complexity.

JButton button = new JButton();
button.setBounds(x, y, BUTTON_SIDE, BUTTON_SIDE);

• Function doing too many things
The empty lines that you have used to split your code into sections is a code smell: it indicates that your function is doing too many different things, and therefore needs to be divided into several functions, each doing one thing. To quote Robert C. Martin, author of Clean Code: A Handbook of Agile Software Craftsmanship:

Functions should do one thing. They should do it well. They should do it only.

• Using complex syntax for simple things
Code like the following two examples from your code snippet

x = x + 15;
y = y + 15;


can be shortened by using the combined += operator.

## Refactored

private static final int NUMBER_OF_BUTTONS = 2501;
private static final int BUTTON_SIDE = 15;
private JFrame mapFrame = new JFrame();

public void createAndShowMap() {
mapFrame.setBounds(40, 0, 750, 773);
mapFrame.setVisible(true);
}

int leftOffset = 0;
int topOffset = 0;
for (int i = 0; i < NUMBER_OF_BUTTONS; i++) {
if (isBeyondEndOfLine(leftOffset)) {
leftOffset = 0;
topOffset += BUTTON_SIDE;
}
leftOffset += BUTTON_SIDE;
}
}

private boolean isBeyondEndOfLine(int x) {
return x >= mapFrame.getBounds().width;
}

private void addButtonAt(int x, int y) {
JButton button = new JButton();
button.setBounds(x, y, BUTTON_SIDE, BUTTON_SIDE);

• My addition would be - make the bounds a parameter for the create method and calculate the number of buttons from the size of frame and size of one button. – Sulthan Sep 11 '12 at 11:45