2
\$\begingroup\$

A friend asked me to write a pen-and-paper dice roller, so I put this together. I'm still pretty new to Java Swing, so I was hoping I could have some advice on how I handled the layouts and everything, and also just that everything is clean and efficient. He wanted to be able to type the dice roll in DnD format (ex. 4d20), so I parse that out and make sure everything's valid. I also wanted to throw in some nice shortcuts where you can just hit enter to roll, or if you want both parties to have the same roll you can just put it in the opposing party field. You can also switch between War and Party mode, where in war mode the totals also detract from the army's size.

Sample Screenshot

enter image description here enter image description here

DnDRoller.java

package dndroller;

import java.awt.Dimension;
import javax.swing.JFrame;

public class DnDRoller {

    public static void main(String[] args) {
        JFrame frame = new JFrame("DnD Roller");
        frame.setDefaultCloseOperation(JFrame.EXIT_ON_CLOSE);


        RollerPanel rollerPanel = new RollerPanel();
        frame.getContentPane().add(rollerPanel);
        frame.getRootPane().setDefaultButton(rollerPanel.getDefaultButton());

        Dimension minSize = new Dimension(650, 650);
        frame.setMinimumSize(minSize);
        frame.setPreferredSize(minSize);
        frame.pack();
        frame.setVisible(true);
    }

}

RollerPanel.java

package dndroller;

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

class RollerPanel extends JPanel {

    private static final Font INPUT_FONT  = new Font("Arial", Font.PLAIN, 12);
    private static final Font OUTPUT_FONT = new Font("Monospaced", Font.PLAIN, 12);

    private static final int MAIN_GROUP = 0, OPPOSING_GROUP = 1;
    private static final int NUM_DICE = 0, DIE_VALUE = 1;

    private Box armiesBox;
    private JTextField[] rollFields;
    private JTextField[] warFields;
    private JTextArea outputArea;
    private JRadioButton warMode, partyMode;
    private JLabel[] groupLabels;
    private JButton rollButton, clearButton;

    protected RollerPanel() {
        setLayout(new BorderLayout());

        Box topPanel = Box.createVerticalBox();

        //Set up radio options for mode
        ButtonGroup modeGroup = new ButtonGroup();
        Box modeBox = Box.createHorizontalBox();

        warMode = new JRadioButton("War Mode");
        warMode.addActionListener(new ModeListener());
        modeGroup.add(warMode);

        partyMode = new JRadioButton("Party Mode");
        partyMode.addActionListener(new ModeListener());
        partyMode.setSelected(true);
        modeGroup.add(partyMode);

        modeBox.add(Box.createHorizontalGlue());
        modeBox.add(partyMode);
        modeBox.add(Box.createHorizontalGlue());
        modeBox.add(warMode);
        modeBox.add(Box.createHorizontalGlue());

        topPanel.add(modeBox);

        //Set up input panel
        Box inputBox = Box.createHorizontalBox();

        final Dimension BOX_DIMENSION = new Dimension(0, 30);
        rollFields = new JTextField[2];
        rollFields[MAIN_GROUP] = new JTextField(10);
        rollFields[MAIN_GROUP].setMaximumSize(BOX_DIMENSION);
        rollFields[MAIN_GROUP].setFont(INPUT_FONT);
        rollFields[OPPOSING_GROUP] = new JTextField(10);
        rollFields[OPPOSING_GROUP].setMaximumSize(BOX_DIMENSION);
        rollFields[OPPOSING_GROUP].setFont(INPUT_FONT);

        groupLabels = new JLabel[2];
        groupLabels[MAIN_GROUP] = new JLabel("Main Party");
        groupLabels[OPPOSING_GROUP] = new JLabel("Opposing Party");

        inputBox.add(Box.createHorizontalGlue());
        inputBox.add(groupLabels[MAIN_GROUP]);
        inputBox.add(Box.createHorizontalStrut(5));
        inputBox.add(rollFields[MAIN_GROUP]);
        inputBox.add(Box.createHorizontalStrut(20));
        inputBox.add(groupLabels[OPPOSING_GROUP]);
        inputBox.add(Box.createHorizontalStrut(5));
        inputBox.add(rollFields[OPPOSING_GROUP]);
        inputBox.add(Box.createHorizontalGlue());

        topPanel.add(inputBox);

        armiesBox = Box.createHorizontalBox();

        warFields = new JTextField[2];
        warFields[MAIN_GROUP] = new JTextField(5);
        warFields[MAIN_GROUP].setName("Main Army");
        warFields[MAIN_GROUP].setMaximumSize(BOX_DIMENSION);
        warFields[MAIN_GROUP].setFont(INPUT_FONT);
        warFields[OPPOSING_GROUP] = new JTextField(5);
        warFields[OPPOSING_GROUP].setName("Opposing Army");
        warFields[OPPOSING_GROUP].setMaximumSize(BOX_DIMENSION);
        warFields[OPPOSING_GROUP].setFont(INPUT_FONT);

        armiesBox.add(Box.createHorizontalGlue());
        armiesBox.add(new JLabel("Main Army Size"));
        armiesBox.add(Box.createHorizontalStrut(5));
        armiesBox.add(warFields[MAIN_GROUP]);
        armiesBox.add(Box.createHorizontalStrut(20));
        armiesBox.add(new JLabel("Opposing Army Size"));
        armiesBox.add(Box.createHorizontalStrut(5));
        armiesBox.add(warFields[OPPOSING_GROUP]);
        armiesBox.add(Box.createHorizontalGlue());

        armiesBox.setVisible(false);
        topPanel.add(armiesBox);

        add(topPanel, BorderLayout.NORTH);

        //Create output panel
        JPanel outputPanel = new JPanel();
        outputPanel.setLayout(new BoxLayout(outputPanel, BoxLayout.PAGE_AXIS));

        outputArea = new JTextArea();
        outputArea.setFont(OUTPUT_FONT);
        outputArea.setColumns(75);
        outputArea.setRows(30);
        outputArea.setEditable(false);
        outputArea.setMinimumSize(new Dimension(250, 400));

        JScrollPane outputPane = new JScrollPane(outputArea);
        outputPane.setMinimumSize(new Dimension(250, 400));
        outputPane.setHorizontalScrollBarPolicy(ScrollPaneConstants.HORIZONTAL_SCROLLBAR_AS_NEEDED);
        outputPane.setVerticalScrollBarPolicy(ScrollPaneConstants.VERTICAL_SCROLLBAR_ALWAYS);

        JLabel resultsLabel = new JLabel("Roll results");
        resultsLabel.setAlignmentX(CENTER_ALIGNMENT);

        outputPanel.setMinimumSize(new Dimension(250, 600));
        outputPanel.add(Box.createHorizontalStrut(10));
        outputPanel.add(resultsLabel);
        outputPanel.add(outputPane);
        outputPanel.add(Box.createHorizontalStrut(10));

        add(outputPanel, BorderLayout.CENTER);

        //Set up action button panel
        Box actionBox = Box.createHorizontalBox();
        rollButton  = new JButton("Roll");
        clearButton = new JButton("Clear");

        ActionListener buttonListener = new ButtonListener();
        rollButton.addActionListener(buttonListener);
        clearButton.addActionListener(buttonListener);

        actionBox.add(Box.createHorizontalGlue());
        actionBox.add(rollButton);
        actionBox.add(Box.createHorizontalStrut(10));
        actionBox.add(clearButton);
        actionBox.add(Box.createHorizontalGlue());

        add(actionBox, BorderLayout.SOUTH);
    }

    protected JButton getDefaultButton() {
        return rollButton;
    }

    private void decreaseArmy(JTextField armyField, int decreaseBy) {
        int armySize = -1;

        try {
            armySize = Integer.parseInt(armyField.getText());
        } catch (NumberFormatException e) {
            JOptionPane.showMessageDialog(armyField, armyField.getText() + " isn't a valid army size.");
        }

        if (armySize > 0) {
            if (armySize <= decreaseBy) {
                armyField.setText("DEAD");
                JOptionPane.showMessageDialog(armyField, armyField.getName() + " has been destroyed!",
                        "Army died", JOptionPane.PLAIN_MESSAGE);
            } else {
                armyField.setText(Integer.toString(armySize - decreaseBy));
            }
        }
    }

    private void clearOutput() {
        outputArea.setText("");

        if (armiesBox.isVisible()) {
            warFields[MAIN_GROUP].setText("");
            warFields[OPPOSING_GROUP].setText("");
        }
    }

    private class ButtonListener implements ActionListener {

        @Override
        public void actionPerformed(ActionEvent e) {
            Object source = e.getSource();

            if (source == rollButton) {
                handleRoll();
            } else if (source == clearButton) {
                clearOutput();
            } else {
                throw new UnsupportedOperationException("Unsupported button: " + source);
            }
        }

        private void handleRoll() {
            String[][] resultLines;
            StringBuilder appendText = new StringBuilder();

            if (rollFields[DIE_VALUE].getText().isEmpty()) {
                resultLines = new String[1][];
            } else {
                if (rollFields[NUM_DICE].getText().isEmpty()) {
                    rollFields[NUM_DICE].setText(rollFields[1].getText());
                }
                resultLines = new String[2][];
            }

            for (int i = 0; i < resultLines.length; i++) {
                try {
                    resultLines[i] = Roller.rollResult(groupLabels[i].getText(), rollFields[i].getText());
                } catch (IllegalArgumentException e) {
                    JOptionPane.showMessageDialog(outputArea, e.getMessage());
                }
            }

            int maxLines = (resultLines.length == 1 || resultLines[0].length > resultLines[1].length)
                                ? resultLines[MAIN_GROUP].length : resultLines[OPPOSING_GROUP].length;

            for (int lineNum = 0; lineNum < maxLines - 1; lineNum++) {
                for (String[] resultLine : resultLines) {
                    if (lineNum >= resultLine.length - 1) {
                        appendText.append(String.format("     %-30s", " "));
                    } else {
                        appendText.append(String.format("     %-30s", resultLine[lineNum]));
                    }
                }
                appendText.append("\n");
            }
            appendText.append(String.format("     %-30s",
                    resultLines[MAIN_GROUP][resultLines[MAIN_GROUP].length - 1]));

            if (resultLines.length == 2) {
                appendText.append(String.format("     %-30s%n",
                        resultLines[OPPOSING_GROUP][resultLines[OPPOSING_GROUP].length - 1]));

                int partyValue = Integer.parseInt(resultLines[MAIN_GROUP][resultLines[MAIN_GROUP].length - 1]
                                                        .substring(7));
                int opposingValue = Integer.parseInt(resultLines[OPPOSING_GROUP][resultLines[OPPOSING_GROUP].length - 1]
                                                        .substring(7));

                if (armiesBox.isVisible()) {
                    decreaseArmy(warFields[MAIN_GROUP], partyValue);
                    decreaseArmy(warFields[OPPOSING_GROUP], opposingValue);
                }

                if (partyValue == opposingValue) {
                    appendText.append("     They tied!");
                } else if (partyValue > opposingValue) {
                    appendText.append("     The ").append(armyOrParty()).append(" won!");
                } else {
                    appendText.append("     The opposing ").append(armyOrParty()).append(" won.");
                }
            }

            appendText.append("\n\n");

            outputArea.append(appendText.toString());
        }
    }

    private String armyOrParty() {
        if (warMode.isSelected()) {
            return "army";
        } else {
            return "party";
        }
    }

    private class ModeListener implements ActionListener {

        @Override
        public void actionPerformed(ActionEvent e) {
            Object source = e.getSource();

            if (source == warMode && !armiesBox.isVisible()) {
                armiesBox.setVisible(true);
                groupLabels[MAIN_GROUP].setText("Main Army");
                groupLabels[OPPOSING_GROUP].setText("Opposing Army");
                clearOutput();
            } else if (source == partyMode && armiesBox.isVisible()) {
                armiesBox.setVisible(false);
                groupLabels[MAIN_GROUP].setText("Main Party");
                groupLabels[OPPOSING_GROUP].setText("Opposing Party");
                clearOutput();
            }
        }

    }
}

Roller.java

package dndroller;

import java.util.Random;

public class Roller {
    private static final Random random = new Random();

    private static final int NUM_DICE = 0, DIE_VALUE = 1;

    public static int[] parseRollEntry(String entry) {
        int[] entries = new int[2];
        String[] entryStrs = entry.split("d");

        if (entryStrs.length != 2) {
            throw new IllegalArgumentException("Sorry, " + entry + " is not a valid dice roll!");
        }

        for (int i = 0; i < 2; i++) {
            if (!entryStrs[i].isEmpty()) {
                try {
                    entries[i] = Integer.parseInt(entryStrs[i]);

                    if (entries[i] <= 0) {
                        entries[i] = 1;
                    }
                } catch (NumberFormatException e) {
                    throw new IllegalArgumentException("Sorry, " + entry
                            + " is not a valid dice roll!");
                }
            }
        }

        return entries;
    }

    public static String[] rollResult(String partyName, String rollEntry) {
        String[] out;
        int[] roll = parseRollEntry(rollEntry);

        final int VERBOSE_CUTOFF = 20;

        if (roll[NUM_DICE] == 0) {
            roll[NUM_DICE] = 1;
        }

        int rounds;
        if (roll[NUM_DICE] > VERBOSE_CUTOFF) {
            out = new String[2];
            rounds = roll[NUM_DICE];
        } else {
            out = new String[roll[NUM_DICE] + 2];
            rounds = roll[NUM_DICE] + 1;
        }

        out[0] = partyName + " rolled " + rollEntry;
        int total = 0;
        for (int i = 1; i < rounds; i++) {
            int nextRoll = rollInt(roll[1]);
            if (roll[NUM_DICE] <= VERBOSE_CUTOFF) {
                out[i] = String.format("Roll %d: d%d -> %d", i, roll[DIE_VALUE], nextRoll);
            }
            total += nextRoll;
        }

        out[out.length - 1] = "Total: " + total;

        return out;
    }

    public static int roll(int[] roll) {
        if (roll.length != 2) {
            StringBuilder rollStr = new StringBuilder("{ ");
            for (int i = 0; i < roll.length - 1; i++) {
                rollStr.append(roll[i]).append(", ");
            }
            rollStr.append(roll[roll.length - 1]).append(" }");

            throw new IllegalArgumentException("Illegal roll entry: " + rollStr);
        }

        int total = 0;
        for (int i = 0; i < roll[NUM_DICE]; i++) {
            total += random.nextInt(roll[DIE_VALUE]) + 1;
        }

        return total;
    }

    public static int rollInt(int bound) {
        if (bound <= 0) {
            throw new IllegalArgumentException("Negative bound:" + bound);
        }

        return random.nextInt(bound) + 1;
    }

}
\$\endgroup\$

1 Answer 1

2
\$\begingroup\$

I'll be going through this from top down. This means I won't do an overall design critique. From a quick skim this would be a good idea, though. I do hope someone else does that.

Anyways </meta>, and into the review:

Your main-method is clean and simple, I'd personally rewrite it a little though. I prefer to include the JFrame into my customized UI classes to provide better encapsulation and cohesion of things related to one component.

Consider:

public static void main(String[] args) {
    RollerView r = new RollerView();
    r.show();
}

This allows you to keep the JFrame setup separate from main and gets it into RollerPane (which I renamed to RollerView, because it's not a Pane there). When you want to change the minimumSize you'll look in the class you have there and don't have to go all over the codebase to find all JFrames where it's added to the ContentPane, and change those.


class RollerPanel extends JPanel {

Favor composition over inheritance. It allows you to easier change how things work internally. This plays even better when you pull the JFrame into RollerView, because the JPanel is actually something you won't need a reference to after setting it up.


private static final int MAIN_GROUP = 0, OPPOSING_GROUP = 1;
private static final int NUM_DICE = 0, DIE_VALUE = 1;
//[...]
private JTextField[] rollFields;
private JTextField[] warFields;

I like how you've declared named constants for the things you do. I don't like how you've solved the problem of having two named controls for the same thing though...

Instead of an array (which is "magic") it would've IMO been better to just go with:

private final JTextField mainGroupRoll;
private final JTextField opposingGroupRoll;
private final JTextField mainGroupWarField;
private final JTextField opposingGroupWarField;

I find this to be significantly clearer in accessing than: rollFields[MAIN_GROUP] or warFields[OPPOSING_GROUP].
Same applies to groupLabels btw.

In addition to that it's usually considered bad practice to declare multiple variables or fields on one line, see (MAIN_GROUP, OPPOSING_GROUP, warMode, partyMode, ...)


I like how you set up your components in your constructor. I especially like how you've been grouping component setup code by component. I don't like that it's all typed out in the constructor. You're intermingling abstraction levels in a way.

Creating a component and adding it to your content pane are two different levels of abstraction. I strongly suggest extracting the component creation into properly named methods to cut down the number of lines in your constructor.
Additionally you can abuse the fact that those components never will change over the lifetime of your panel (or roller). You should declare the fields as final where possible. This is especially helpful after "pulling apart" the arrays, since a final array is still mutable, wrt. it's content. This doesn't happen when you pull the array apart into separate fields.
Let me demonstrate what I mean:

private final Component[] components = new Component[2];
private final Component mainGroupComponent = new JTextField();
private final Component opposingGroupComponent = new JTextField();

public Ctor() {
    components[0] = mainGroupComponent;
    components[1] = opposingGroupComponent;
}

public void doSomething() {
    components[0] = new JTextField(); // completely legal
    mainGroupComponent = new JTextField(); // compiler error
}

Immutability generally is considered a good thing, since it allows you to reduce mental strain when reading code (stuff doesn't change) and it allows for some Compiler optimizations (both for javac and the JIT compiler), which can improve overall performance.


For now that's it about the GUI. I've got a small point to make about your Roller, though...

You should also handle dice rolls like "5+3d20" correctly. As of now that's not the case, though...

\$\endgroup\$
1
  • \$\begingroup\$ Thanks so much for your review!! I did just have a follow-up question though. You mention about not using arrays, which definitely makes sense in most cases, but I thought an array would be better here because of the amount of duplicate code there'd be otherwise with parsing the fields for handling opposing rolls. Or would you recommend I break that stuff out too into another method as well? (I just thought of that while typing my comment). Also, thanks for the idea about the modifier! I added that in too :) \$\endgroup\$
    – BrainFRZ
    Apr 30, 2016 at 20:42

Your Answer

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

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