5
\$\begingroup\$

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);
        MapFrame.add(grid[i]);

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

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

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);
    mapFrame.add(button);   
    
  • 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);
    addButtons();
    mapFrame.setVisible(true);
}

private void addButtons() {
    int leftOffset = 0;
    int topOffset = 0;
    for (int i = 0; i < NUMBER_OF_BUTTONS; i++) {
        if (isBeyondEndOfLine(leftOffset)) {
            leftOffset = 0;
            topOffset += BUTTON_SIDE;
        }
        addButtonAt(leftOffset, topOffset);
        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);
    mapFrame.add(button);
}
\$\endgroup\$
3
  • \$\begingroup\$ 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. \$\endgroup\$ – Sulthan Sep 11 '12 at 11:45
  • \$\begingroup\$ @codesparkle, A very well written answer, it might even be the best I've received on all the StackExchange sites! (If you read the section I deleted, never mind.) \$\endgroup\$ – Russell Sep 11 '12 at 12:33
  • 1
    \$\begingroup\$ @codesparkle, If I wouldn't be banned for sockpuppeting, I would upvote everything you've written. The book you recommended is amazing, and your code is an example of what the book considers perfect clean code. I have bookmarked this answer and set it as my background. I shall live and code by this standard. Long Live Codesparkle! \$\endgroup\$ – Russell Oct 11 '12 at 12:36

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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