6
\$\begingroup\$

I've been working on this utility for a while now, and have since gone through a couple reworks to try and increase the readability and best practices, but since I don't usually work a lot with Swing, or UI design in general, I still have the nagging feeling there's a better way to approach this. Code for both files is below along with a GitHub link in case full context is needed.

Specific Questions

  1. Is GridBagUIPanel a proper abstraction of functionality, or is it too little/too much?
  2. Is there anything I should be doing to clean up init()/update() more in LifePathUI?
  3. Am I using things right with the TextChangeListeners and Button Events in LifePathUI? Defining classes inside method calls still feels very off to me.
  4. Is there any glaring reinventing the wheel or other mistakes going on? I want to cut out any particularly embarrassing things since this is one of my few projects that would be able to be used in portfolios and such.

Additional Edit/Note: There is plans for additional UI/functions for this project, so code was brought outside of LifePathUI for reuse purposes as well. I just realized that would also be important context as to intent.

Main UI (LifePathUI)

import java.awt.*;
import java.awt.event.ActionEvent;
import java.awt.event.ActionListener;

import javax.swing.JFrame;
import javax.swing.JLabel;
import javax.swing.JOptionPane;
import javax.swing.JScrollPane;
import javax.swing.JTextArea;
import javax.swing.event.DocumentEvent;
import javax.swing.event.DocumentListener;

public class LifePathUI implements UI {

     final static boolean shouldWeightX = true;
     final static String DIVIDER_STRING = "\n------------------------------------------\n";

     // we still hardcode some stats like this because the page would break if they were user definable anyways
     final static String[] primStats = {"COG","COO","INT","REF","SAV","SOM","WIL"}; 
     final static String[] secStats = {"DUR","WT","DR","LUC","TT","IR","INIT","SPD","DB"};
     private BorderLayout windowLayout;
     private JFrame mainWindow;
     private GridBagUIPanel mainPanel, statPanel,sideBar;
     private JTextArea mainStatus;
     private LifePathGenerator gen;

    /**
     * @throws HeadlessException
     */
    public LifePathUI() throws HeadlessException {
        DataProc.init("LifepathPackages.dat","internalInfo.dat");
        gen = new LifePathGenerator("",this,true);
        windowLayout = new BorderLayout();
        mainWindow = new JFrame();     
        mainPanel = new GridBagUIPanel();
        statPanel = new GridBagUIPanel();
        mainWindow.setDefaultCloseOperation(javax.swing.WindowConstants.EXIT_ON_CLOSE);
    }

    /* (non-Javadoc)
     * @see UI#promptUser(java.lang.String, java.lang.String)
     */
    @Override
    public String promptUser(String message, String extraContext) {

        String prompt = message + "\n" + extraContext;

        // check for a few things that lets us provide extra info
        String[] result = DataProc.getExtraPromptOptions(message,extraContext);
        if (result != null )
        {
            prompt += result[1];
        }

        String inputValue = "";

        // for longer prompts we use a more proper scrollable UI object
        // for short prompts we use a more compact/prettier/simpler one
        if (prompt.length() > 3000)
        {
            JTextArea textArea = new JTextArea(prompt);
            JScrollPane scrollPane = new JScrollPane(textArea);  
            textArea.setLineWrap(true);  
            textArea.setWrapStyleWord(true); 
            textArea.setEditable(false);
            scrollPane.setPreferredSize( new Dimension( 800, 500 ) );
            inputValue = JOptionPane.showInputDialog(null, scrollPane, "Enter Choice",  
                                                            JOptionPane.QUESTION_MESSAGE);
        }
        else
        {
            inputValue = JOptionPane.showInputDialog(prompt);
        }

        return inputValue;
    }

    /* (non-Javadoc)
     * @see UI#handleError(java.lang.String)
     */
    @Override
    public boolean handleError(String message) {
        int resp = JOptionPane.showConfirmDialog(null,"Error Resulted, redo step?\n"+message, "Error", JOptionPane.YES_NO_OPTION);

        if (resp == JOptionPane.YES_OPTION)
        {
            return true;
        }
        else            
        {
            return false;
        }       
    }

    /* (non-Javadoc)
     * @see UI#statusUpdate(java.lang.String)
     */
    @Override
    public void statusUpdate(String message) {
        this.appendStatusText(message);
    }

    /* (non-Javadoc)
     * @see UI#end()
     */
    @Override
    public void end() {
        // Marks the character gen process as stopped, disabling the buttons that used to advance it
        mainPanel.remove(mainPanel.getComponent("Run Next Step"));
        mainWindow.revalidate();
        mainWindow.repaint();
    }

    /**
     * Main setup method for the UI, creates most UI components and initializes their default state
     */
    public void init()
    {
        mainWindow.setComponentOrientation(ComponentOrientation.LEFT_TO_RIGHT);
        mainWindow.setLayout(windowLayout);

        // to make everything work right we add a mainPanel under the mainWindow
        mainWindow.add(mainPanel);

        // start first row of rows of mixed size
        mainPanel.addMappedTF(0,0,"Character Name",20,new TextChangeListener());
        mainPanel.addMappedFixedTF(2,0,"Morph","",10,true);
        mainPanel.addMappedFixedTF(4,0,"Background","",10,true);
        mainPanel.addMappedFixedTF(6,0,"Natural Language", "",15,true);
        mainPanel.addMappedFixedTF(8,0,"Faction","",10,true);
        mainPanel.endRow(10,0);

        // we init new Panel for the sidebar (skills displays)
        sideBar = new GridBagUIPanel();

        // addC is (component,x,y,height,width)
        mainPanel.addC(sideBar,22,0,GridBagConstraints.REMAINDER,1);

        mainPanel.addC(statPanel,0,1,6,GridBagConstraints.RELATIVE);

        // stats are predictable in format and appearance, so we do them via loops

        // Add first row, with the Base Primary stat values
        int idx = 0;
        for (String key : primStats)
        {
            statPanel.addMappedFixedTF(idx,0,"Base "+key, ""+gen.getPC().getAptitude(key),5,true);
            idx +=2;
        }
        statPanel.endRow(idx,0);


        // add row for bonuses
        idx = 0;
        for (String key : primStats)
        {
            statPanel.addMappedTF(idx, 1, "Bonus", "MorphBonus"+key, 5, new TextChangeListener());
            idx +=2;
        }
        statPanel.endRow(idx,1);

        // add final row for totals.
        idx = 0;
        for (String key : primStats)
        {
            statPanel.addMappedFixedTF(idx,2,"Total "+key, "",5,true);
            idx +=2;
        }
        statPanel.endRow(idx,2);


        // add row for base secondary stats
        idx = 0;
        for (String key : secStats)
        {
            statPanel.addMappedFixedTF(idx,3,key, ""+gen.getPC().getSecStat(key),5,true);
            idx +=2;
        }
        statPanel.endRow(idx,3);

        // now we do bonuses
        idx = 0;
        for (String key : secStats)
        {
            statPanel.addMappedTF(idx, 4, "Bonus", "MorphBonus"+key, 5, new TextChangeListener());
            idx +=2;
        }
        statPanel.endRow(idx,4);

        // now we do the totals
        idx = 0;
        for (String key : secStats)
        {
            statPanel.addMappedFixedTF(idx,5,"Total "+key, "",5,true);
            idx +=2;
        }
        statPanel.endRow(idx,5);

        // a few extra stats get factored in too
        statPanel.addMappedFixedTF(0,6,"Stress", "",5,true);
        statPanel.addMappedFixedTF(2,6,"MOX", "",5,true);
        statPanel.addMappedFixedTF(4,6,"Credits", "",5,true);
        statPanel.addMappedFixedTF(6,6,"Free CP", "",5,true);
        statPanel.endRow(8,6);

        // last bar is Rep values, which can vary based on configuration
        int xIdx = 0;
        for (Rep r : gen.getPC().getAllRep())
        {
            statPanel.addMappedFixedTF(xIdx,7,r.getName()+"-rep", ""+r.getValue(),5,true);
            xIdx += 2;
        }
        statPanel.endRow(xIdx,7);

        // create the main status window        
        mainStatus = new JTextArea(40,160);
        mainStatus.setMinimumSize(mainStatus.getPreferredSize());
        mainStatus.setMaximumSize(mainStatus.getPreferredSize());
        mainStatus.setLineWrap(true);
        mainStatus.setWrapStyleWord(true);
        JScrollPane tempPane = new JScrollPane(mainStatus);
        tempPane.setHorizontalScrollBarPolicy(JScrollPane.HORIZONTAL_SCROLLBAR_NEVER);
        tempPane.setMinimumSize(tempPane.getPreferredSize());

        mainPanel.addC(tempPane,0,9,14,13,GridBagConstraints.BOTH);


        mainPanel.addMappedButton(0,26,"Firewall Events").addActionListener(new ActionListener() {

            public void actionPerformed(ActionEvent e)
            {
                if (gen.getPC().hasVar("{firewall}"))
                {
                    gen.getPC().removeVar("{firewall}");
                    mainPanel.setButtonText("Firewall Events","Firewall Events (Off)");
                }
                else
                {
                    gen.getPC().setVar("{firewall}", "1");
                    mainPanel.setButtonText("Firewall Events","Firewall Events (On)");
                }
            }   
        });

        // set this afterwards so it doesn't change the mapping name
        mainPanel.setButtonText("Firewall Events","Firewall Events (Off)");


        mainPanel.addMappedButton(1,26,"Gatecrashing Events").addActionListener(new ActionListener() {

            public void actionPerformed(ActionEvent e)
            {
                if (gen.getPC().hasVar("{gatecrashing}"))
                {
                    gen.getPC().removeVar("{gatecrashing}");
                    mainPanel.setButtonText("Gatecrashing Events","Gatecrashing Events (Off)");
                }
                else
                {
                    gen.getPC().setVar("{gatecrashing}", "1");
                    mainPanel.setButtonText("Gatecrashing Events","Gatecrashing Events (On)");
                }
            }   
        });

        // set this afterwards so it doesn't change the mapping name
        mainPanel.setButtonText("Gatecrashing Events","Gatecrashing Events (Off)");

        mainPanel.addMappedButton(3,26,"Run Next Step").addActionListener(new ActionListener() {

            public void actionPerformed(ActionEvent e)
            {
                gen.step();
                update();
            }   
        });

        // gives a quick export of the character
        mainPanel.addButton(5,26,"Export to Txt").addActionListener(new ActionListener() {

            public void actionPerformed(ActionEvent e)
            {
                update();             

                JTextArea updateArea= new JTextArea(gen.getPC().toString() + DIVIDER_STRING + mainStatus.getText(),10,120);              
                updateArea.setEditable(true);
                updateArea.setLineWrap(true);
                JScrollPane scroll = new JScrollPane (updateArea, JScrollPane.VERTICAL_SCROLLBAR_ALWAYS, JScrollPane.HORIZONTAL_SCROLLBAR_NEVER);
                JOptionPane.showMessageDialog(null, scroll,"Exported Character", JOptionPane.PLAIN_MESSAGE);

            }   
        });

        // gives a quick export of the character
        mainPanel.addMappedButton(7,26,"Rolling").addActionListener(new ActionListener() {

            public void actionPerformed(ActionEvent e)
            {
                if (gen.isRolling())
                {
                    gen.setRolling(false);
                    mainPanel.setButtonText("Rolling","Choosing Rolls");
                }
                else
                {
                    gen.setRolling(true);
                    mainPanel.setButtonText("Rolling","Rolling");                   
                }
            }   
        });

        mainPanel.endVertical(0,27);

        this.update();

        mainWindow.setSize(1700, 1000);

        mainWindow.setVisible(true);
    }


    /**
     * Adds text to the end of the status text area
     * @param text Text to be added
     */
    private void appendStatusText(String text)
    {
        this.mainStatus.setText(this.mainStatus.getText() + "\n\n" + text);
    }

    /**
     * Updates all relevant display fields for the character
     */
    private void update()
    {
        gen.getPC().setName(mainPanel.getTextF("Character Name").getText());

        gen.getPC().calcStats(); // updates secondaries

        // set morph and background
        String morphName = "";

        if (gen.getPC().getCurrentMorph() != null)
        {
            morphName = gen.getPC().getCurrentMorph().getName();
        }

        mainPanel.getTextF("Morph").setText(morphName);

        if (gen.getPC().hasVar("{background}"))
        {
            mainPanel.setTextF("Background",gen.getPC().getBackground());
        }

        if (gen.getPC().hasVar("NatLang"))
        {
            mainPanel.setTextF("Natural Language",gen.getPC().getVar("NatLang"));
        }

        if (gen.getPC().hasVar("{factionName}"))
        {
            mainPanel.setTextF("Faction",gen.getPC().getVar("{factionName}"));
        }

        int[] stats = new int[16];
        int[] bonuses = new int[16];
        int cnt = 0;

        // fill stats with all the primary and secondary stat values
        for (String key : primStats)
        {
            stats[cnt++] = gen.getPC().getAptitude(key);
        }
        for (String key : secStats)
        {
            stats[cnt++] = gen.getPC().getSecStat(key);
        }

        cnt = 0;

        // update base stats for both
        for (String key : primStats)
        {
            statPanel.setTextF("Base "+key,stats[cnt++]);
        }
        for (String key : secStats)
        {
            statPanel.setTextF(key,stats[cnt++]);
        }

        // get bonus amounts
        cnt = 0;
        for (String key : primStats)
        {
            bonuses[cnt++] = statPanel.getTextFVal("MorphBonus"+key);
        }
        for (String key : secStats)
        {
            bonuses[cnt++] = statPanel.getTextFVal("MorphBonus"+key);
        }

        // build stat totals
        cnt = 0;
        for (String key : primStats)
        {
            statPanel.setTextF("Total "+key,(stats[cnt] + bonuses[cnt]));cnt++;
        }
        for (String key : secStats)
        {
            statPanel.setTextF("Total "+key,(stats[cnt] + bonuses[cnt]));cnt++;
        }
        cnt = 0;

        // update a few more display fields
        statPanel.setTextF("Stress",gen.getPC().getVarInt("{stress}"));
        statPanel.setTextF("MOX",gen.getPC().getSecStat("MOX"));
        statPanel.setTextF("Credits",gen.getPC().getVarInt("{credits}"));
        statPanel.setTextF("Free CP",gen.getPC().getVarInt("{CP}"));

        // update rep
        for (Rep r : gen.getPC().getAllRep())
        {
            statPanel.setTextF(r.getName()+"-rep",+r.getValue());
        }

        // rebuild skills panel
        sideBar.removeAll();

        sideBar.addC(new JLabel("Skills            "),0,0);
        sideBar.addC(new JLabel("                  "),1,0);
        int x = 0, y = 1;
        for(String[] pair : gen.getPC().getSkills())
        {
            sideBar.addMappedFixedTF(x,y,pair[0], pair[1], 5, false);
            if (y <= 32)
            {
                y+=2;
            }
            else
            {
                x += 2;
                y =  0;
            }
        }

        mainPanel.revalidate();
    }

    /**
     * @param args
     */
    public static void main(String[] args) {

        javax.swing.SwingUtilities.invokeLater(new Runnable() {
            public void run() {
                LifePathUI ui = new LifePathUI();
                ui.init();
            }
        });



    }

    // triggers an update if the text field changes
    private class TextChangeListener implements DocumentListener
    {

        @Override
        public void changedUpdate(DocumentEvent e) {
            update();
        }

        @Override
        public void insertUpdate(DocumentEvent e) {
            update();
        }

        @Override
        public void removeUpdate(DocumentEvent e) {
            update();
        }

    }
}

Support Class (GridBagUIPanel)

import java.awt.ComponentOrientation;
import java.awt.GridBagConstraints;
import java.awt.GridBagLayout;
import java.awt.LayoutManager;
import java.util.ArrayList;
import java.util.HashMap;

import javax.swing.JButton;
import javax.swing.JComponent;
import javax.swing.JLabel;
import javax.swing.JPanel;
import javax.swing.JTextArea;
import javax.swing.JTextField;
import javax.swing.event.DocumentListener;

public class GridBagUIPanel extends JPanel {

    /**
     * 
     */
    private static final long serialVersionUID = 8550646555771187030L;

    private HashMap<String,JComponent> mappedComponents;
    private GridBagConstraints cons;    
    private GridBagLayout layout;
    private ArrayList<JComponent> children;


    /**
     * Creates a new UIPanel with a double buffer and a flow layout.
     */
    public GridBagUIPanel() {
        super();
        init();
    }

    /**
     * Create a new buffered UIPanel with the specified layout manager
     * @param layout - the LayoutManager to use
     */
    public GridBagUIPanel(LayoutManager layout) {
        super(layout);
        init();
    }

    /**
     * Creates a new UIPanel with FlowLayout and the specified buffering strategy. If isDoubleBuffered is true, the UIPanel will use a double buffer.
     * @param isDoubleBuffered - a boolean, true for double-buffering, which uses additional memory space to achieve fast, flicker-free updates
     */
    public GridBagUIPanel(boolean isDoubleBuffered) {
        super(isDoubleBuffered);
        init();
    }

    /**
     * 
     * @param layout - the LayoutManager to use
     * @param isDoubleBuffered - a boolean, true for double-buffering, which uses additional memory space to achieve fast, flicker-free updates
     */
    public GridBagUIPanel(LayoutManager layout, boolean isDoubleBuffered) {
        super(layout, isDoubleBuffered);
        init();
    }

    /**
     * Groups common parts of object initialization away from the separate constructors
     */
    public void init()
    {
        this.mappedComponents = new HashMap<String,JComponent>();
        this.children = new ArrayList<JComponent>();
        this.cons = new GridBagConstraints();
        this.layout = new GridBagLayout();
        this.setComponentOrientation(ComponentOrientation.LEFT_TO_RIGHT);        
        this.setLayout(layout);

        cons.ipadx = 5;
        cons.ipady = 5;
        cons.anchor = GridBagConstraints.NORTHWEST;
        cons.fill = GridBagConstraints.NONE;
        cons.weighty = 1.0;
        cons.weightx = 1.0;
        cons.gridheight = 1;
        cons.gridwidth = 1;
    }

    /**
     * Inserts a spacer to signify the end of components for this row (effects layout so it doesn't center)
     * @param x nonzero int
     * @param y nonzero int
     */
    public void endRow(int x, int y)
    {
        cons.weightx = 100.0;
        addLabel(x,y,"");
        cons.weightx = 1.0;
    }

    /**
     * Inserts a spacer to signify the end of components vertically (effects layout so it doesn't center)
     * @param x nonzero int
     * @param y nonzero int
     */
    public void endVertical(int x, int y)
    { 
        cons.weighty = 100.0;
        addLabel(x,y,"");
        cons.weighty = 1.0;
    }

    /**
     * Adds a JLabel JTextField pair (each weight 1 ), and adds the textField to the mappedComponents list
     * using the labelText for a key
     * 
     * @param x non-negative integer
     * @param y non-negative integer
     * @param labelText Label to display
     * @param updateListen If not null, will add listener passed in to the text field
     * @param cols number of cols for the text field
     */
    public void addMappedTF(int x, int y, String labelText, int cols, DocumentListener updateListen)
    {
        addMappedTF(x,y,labelText,labelText,cols,updateListen);
    }

    /**
     * Adds a JLabel JTextField pair (each weight 1 ), and adds the textField to the mappedComponents list
     * using the labelText for a key. This version specifies a different String for the label and the map key
     * 
     * @param x non-negative integer
     * @param y non-negative integer
     * @param labelText Label to display
     * @param mapName String to use for the map key for the text field
     * @param updateListen If not null, will add listener passed in to the text field
     * @param cols number of cols for the text field
     */
    public void addMappedTF(int x, int y, String labelText, String mapName, int cols, DocumentListener updateListen)
    {
        addLabel(x,y,labelText);
        this.mappedComponents.put(mapName, addTextF(x+1,y,cols,updateListen));
    }

    /**
     * Adds a JLabel JTextField pair (each weight 1 ), and adds the textField to the mappedComponents list
     * using the labelText for a key. The text field is not edittable
     * 
     * @param x non-negative integer
     * @param y non-negative integer
     * @param labelText Label to display
     * @param value The text to display in the field
     * @param cols number of cols for the text field
     * @param horiztonal Whether the pair is arranged horizontally (vertically if false)
     */
    public void addMappedFixedTF(int x, int y, String labelText, String value, int cols, boolean horizontal)
    {
        int newX,newY;

        if (horizontal)
        {
            newX = x+1;
            newY = y;
        }
        else
        {
            newX = x;
            newY = y+1;
        }

        addLabel(x,y,labelText);
        JTextField temp = addTextF(newX,newY,value,cols,null);
        temp.setEditable(false);
        this.mappedComponents.put(labelText, temp);
    }

    /**
     * Shorthand command to add label at coordinates with text
     * @param x non-negative integer
     * @param y non-negative integer
     * @param text Display Text for label
     */
    public JLabel addLabel(int x, int y, String text)
    {
        cons.gridx = x;
        cons.gridy = y;
        JLabel temp = new JLabel(text);
        addC(temp);
        return temp;
    }

    /**
     * Shorthand command to add Button at coordinates with text
     * @param x non-negative integer
     * @param y non-negative integer
     * @param text Display Text for Button
     * @return The component created
     */
    public JButton addButton(int x, int y, String text)
    {
        cons.gridx = x;
        cons.gridy = y;
        JButton temp = new JButton(text);
        addC(temp);
        return temp;
    }

    /**
     * Shorthand command to add Button at coordinates with text add adds it to the mapped components list
     * with key = text
     * @param x non-negative integer
     * @param y non-negative integer
     * @param text Display Text for Button
     * @return The component created
     */
    public JButton addMappedButton(int x, int y, String text)
    {
        JButton temp = addButton(x,y,text);
        this.mappedComponents.put(text, temp);
        return temp;
    }

    /**
     * Shorthand command to add Text Field at coordinates with text
     * @param x non-negative integer
     * @param y non-negative integer
     * @param value the default value for the text field
     * @param cols Number of Columns
     * @param updateListen If not null, will add listener passed in to the text field
     * @return The component created
     */
    public JTextField addTextF(int x, int y, String value, int cols, DocumentListener updateListen)
    {
        cons.gridx = x;
        cons.gridy = y;
        JTextField temp = new JTextField(value, cols);

        // this prevents the common issue of the text fields turning into slits
        temp.setMinimumSize(temp.getPreferredSize());

        if (updateListen != null)
        {
            temp.getDocument().addDocumentListener(updateListen);
        }

        addC(temp);
        return temp;
    }

    /**
     * Shorthand command to add Text Field at coordinates with text. No value set in this version
     * @param x non-negative integer
     * @param y non-negative integer
     * @param cols Number of Columns
     * @param updateListen If not null, will add listener passed in to the text field
     * @return The component created
     */
    public JTextField addTextF(int x, int y, int cols, DocumentListener updateListen)
    {
        return this.addTextF(x, y, "", cols, updateListen);
    }

    /**
     * Shorthand command to add Text Area at coordinates with text
     * @param x non-negative integer
     * @param y non-negative integer
     * @param rows Number of Rows
     * @param cols Number of Columns
     * @return The component created
     */
    public JTextArea addTextArea(int x, int y, int rows, int cols)
    {
        cons.gridx = x;
        cons.gridy = y;
        JTextArea temp = new JTextArea(rows,cols);
        addC(temp);
        return temp;
    }

    /**
     * Shorthand to add new components to the UI tab
     * @param comp Component to add to UI
     */
    public void addC(JComponent comp) {
        this.add(comp,cons);
        this.children.add(comp);
    }

    /**
     * Shorthand to add new components to the UI tab at given coords
     * @param comp Component to add to UI
     * @param x non-negative integer
     * @param y non-negative integer
     * 
     */
    public void addC(JComponent comp, int x, int y) {
        cons.gridx = x;
        cons.gridy = y;
        this.addC(comp);
        this.children.add(comp);
    }

    /**
     * Shorthand to add new components to the UI tab at given coords with height and width
     * @param comp Component to add to UI
     * @param x non-negative integer
     * @param y non-negative integer
     * @param gridHeight valid number or GridBagConstraints constant
     * @param gridWidth valid number or GridBagConstraints constant
     */
    public void addC(JComponent comp, int x, int y, int gridHeight, int gridWidth) {
        int oldHeight = cons.gridheight;
        int oldWidth = cons.gridwidth;
        cons.gridheight = gridHeight;
        cons.gridwidth = gridWidth;
        this.addC(comp,x,y);
        cons.gridheight = oldHeight;
        cons.gridwidth = oldWidth;
    }

    /**
     * Shorthand to add new components to the UI tab at given coords with height, width, and fill value
     * @param comp Component to add to UI
     * @param x non-negative integer
     * @param y non-negative integer
     * @param gridHeight valid number or GridBagConstraints constant
     * @param gridWidth valid number or GridBagConstraints constant
     * @param fill valid number or GridBagConstraints constant
     */
    public void addC(JComponent comp, int x, int y, int gridHeight, int gridWidth, int fill) {
        int oldFill = cons.fill;
        cons.fill = fill;
        this.addC(comp,x,y,gridHeight,gridWidth);
        cons.fill = oldFill;
    }

    /**
     * Returns whether either this Panel or any of it's children have the component in question
     * @param name Name of the mapped component to look for
     * @return true or false as appropriate
     */
    public boolean hasComponent(String name)
    {
        if (this.mappedComponents.containsKey(name))
        {
            return true;
        }
        else
        {
            for (JComponent j : this.children)
            {
                if (j.getClass().getSimpleName().equalsIgnoreCase("GridBagUIPanel"))
                {
                    GridBagUIPanel temp = (GridBagUIPanel)j;
                    return temp.hasComponent(name);
                }
            }

            return false;
        }
    }

    /**
     * Returns eitehr the mapped component of the given name, or null
     * @param name Name of the mapped component to look for
     * @return the JComponent if it exists, null otherwise
     */
    public JComponent getComponent(String name)
    {
        if (this.mappedComponents.containsKey(name))
        {
            return this.mappedComponents.get(name);
        }
        else
        {
            for (JComponent j : this.children)
            {
                if (j.getClass().getSimpleName().equalsIgnoreCase("GridBagUIPanel"))
                {
                    GridBagUIPanel temp = (GridBagUIPanel)j;
                    if ( temp.hasComponent(name) )
                    {
                        return temp.getComponent(name);
                    }
                }
            }

            return null;
        }
    }

    /**
     * Helper method, searches mappedComponents for a JTextField with name, and returns it properly cast to JTextField
     * 
     * @param name Valid name 
     * @return The JTextField of matching name, or null if no matching JTextField exists
     */
    public JTextField getTextF(String name)
    {
        // does it exist?
        if (!this.hasComponent(name) )
        {
            return null;
        }
        else
        {

            JComponent temp = this.getComponent(name);

            // is it the right type?
            if ( !temp.getClass().getSimpleName().equalsIgnoreCase("jtextfield") )
            {
                return null;
            }
            else
            {
                return (JTextField)temp;
            }

        }

    }

    /**
     * Returns the integer value stored in a mapped textfield, if applicable
     * @param name Textfield to search for
     * @return Either the appropriate int value or 0 if no valid integer bearing field could be found
     */
    public int getTextFVal(String name)
    {
        JTextField temp = getTextF(name);       

        // does it exist and is it a number
        if (temp == null || !Utils.isInteger(temp.getText()))
        {
            return 0;
        }
        else
        {
            return Integer.parseInt(temp.getText());
        }
    }

    /**
     * Attempts to retrieve a mapped text field with the matching name and change its text value to val
     * 
     * This function will fail silently if the field doesn't exist
     * 
     * @param name Name of the text field to look for
     * @param val Value to setText to
     */
    public void setTextF(String name, String val)
    {
        JTextField temp = getTextF(name);       

        // does it exist and is it a number
        if (temp == null)
        {
            return;
        }
        else
        {
            temp.setText(val);
        }
    }

    /**
     * Attempts to retrieve a mapped text field with the matching name and change its text value to val
     * This version takes an int value for convenience
     * @param name Name of the text field to look for
     * @param val int Value to setText to, will be converted to string
     */
    public void setTextF(String name, int val)
    {
        this.setTextF(name, ""+val);
    }

    /**
     * Attempts to retrieve a mapped button with the matching name and change its display text to val
     * 
     * This function will fail silently if the button doesn't exist
     * 
     * @param name Name of the button to look for
     * @param val Value to setText to
     */
    public void setButtonText(String name, String val)
    {
        JComponent temp = mappedComponents.get(name);       

        // does it exist and is it a number
        if (temp == null || !temp.getClass().getSimpleName().equalsIgnoreCase("JButton"))
        {
            return;
        }
        else
        {
            ((JButton)temp).setText(val);
        }
    }

    /**
     * Adds a component to this Panel's mappedComponet list
     * @param name Name/key for the component
     * @param comp Component to add
     * @return the component added
     */
    public JComponent put(String name, JComponent comp)
    {
        return this.mappedComponents.put(name, comp);
    }
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to Code Review! That's a really good question, we're glad you found this site! I'm certain our Java pros can help you improve this \$\endgroup\$
    – Phrancis
    Aug 30, 2015 at 2:26

3 Answers 3

3
\$\begingroup\$

Quite nice helper. Also, thanks from RPG fan, I'll check out Eclipse Phase later as well. :-)

My remarks:

No packages!

If the project is to be a portfolio one, use package keyword and put your classes in packages. There is number of advantages:

  1. packages organize areas or your program
  2. they put related classes together
  3. lack of packages means, that your code is all in ONE package, so-called default package. That's considered a code smell.
  4. so-called Fully Qualified Name of a class includes package. Therefore, I can ran number of classes called MyClass within 1 JVM, as long as they have different packages. With default package, there's greater risk of confusing JVM: which MyClass is which?

Read on packages prior to choosing how to group your code.

Code quality, various remarks

I'm sorry for not being able to process and review code fully. Here are things that stood out for me.

  1. Interfaces methods are public by default. You need not specify that.
  2. Each time you have an if X then true else false simplify it to return X; (Skill, LifePathUI, CharacterSheetUI).
  3. If you import a.b.*, there's no need to import a.b.something additionally, as a.b.* already did so.
  4. In number of places you have methods, values, local variables and parameters that are never used. Consider cutting. IDE can show you these, as well as FindBugs or Checkstyle or SonarQube tools.
  5. I found three type casts that were redundant. All to int. All in Character class.

I'm mentioning these only briefly as when one usually checks code for clarity, one uses tools. Tools report these, so you should be aware, if you want to use project to show your skills. Some, you may want to fix, depending on whom you want to impress.

Javadocs

  1. The class Javadoc is often empty, save @author tag. Please explain the class in class Javadoc, number of IDEs show such description when you hover over object of X type. It's quite helpful for others. Examples: Aptitude, Character.

  2. Javadocs for getters and setters are sometimes poor (autogenerated, it seems). Please remove. They bring no value, functions are trivial one-liners with very common purpose and naming adhering to convention. That is enough, Javadocs for such things are only making the file bloated. Examples: Aptitude, Character.

  3. If you have file comments that are empty, remove them. If you have file comments, that hold value to class (example: Function, Sleight, ExistsValidator) not to file, integrate them as Javadocs. Good file comment might be about encoding, or file being tool-generated. I'm yet to see one that's about code and it makes sense for it to be a file comment. For your convenience: File comment is a comment at the top of the file (or just under package and imports, but prior to class javadoc and class itself).

Working with Strings

There are two aspects here. One, this application is very String-reliant. Are you sure you wouldn't want to use types more? The other thing is technical aspect of using Strings.

Technically

You have a Utils class, that works with Strings heavily. Please take a look at projects like StringUtils from Apache (or other, similar). You may find them to your liking, they do a lot of nice String operations: join, split, isEmptyOrBlank, etc. I believe there's also a sister library, NumberUtils, which has a method to check if String contains a digit.

Of course, feel free to reject pulling outside libraries into your project - there may be good reasons not to. However:

  1. NumberUtil.isNumber(String str) also factors in Unicode digits
  2. Knowing common libraries looks good in portfolio.
  3. You don't need to maintain code that doesn't add to your domain (Eclipse Phase RPG helper).

Up to you here. Both libraries can be downloaded together if you pull in Apache Commons. There's also Google alternative: Guava (even better IMO).

More info: https://stackoverflow.com/questions/1444437/apache-commons-vs-guava-formerly-google-collections

https://stackoverflow.com/questions/7015739/is-there-a-viable-generic-alternative-to-apache-commons-collections-collectionut

http://eclipsesource.com/blogs/2013/11/06/get-rid-of-your-stringutils/

Objects vs Strings

Consider Sleight class. In excerpt taken from it, everything is a String!

private String sleightType; 
private String isExsurgent;
private String name;        
private String psiType;
private String actionType;
private String range;
private String duration;
private String strainMod;
private String skillUsed;
private String description;

// stores all the below sleights
public static HashMap<String,Sleight> sleightList = new HashMap<String,Sleight>();

/**
 * @param sleightType Classification of Sleight : chi, gamma, etc
 * @param isExsurgent Whether the sleight is meant to be exsurgent only
 * @param name Name of the sleight
 * @param psiType Active or Passive
 * @param actionType What type of action the sleight uses
 * @param range Range of the sleight
 * @param duration Duration of the sleight
 * @param strainMod Strain mod of the sleight (if any)
 * @param skillUsed Skill the sleight uses (if active
 * @param description Human readable description of sleight

Now, from really great javadoc on Sleight class constructor, I can see that we're actually having an enum here, a boolean, and so on. Number of types. Why remain with only Strings? Booleans behave nicely in flow control, enums have compile-time checks for whether I chose correct value or not, as well as run-time checks, and so on.

Types give you great benefits over just Strings.

Domain classes

Please put them in package. Core, base, or ep (from Eclipse Phase, but lowercased).

Aptitude

  1. Aptitudes are presented as String array. For such a case, I'd use Enum type. It's purpose are enumerations.

  2. Aptitude maximum is set to 40. I'd add a comment citing source for this. If Eclipse Phase will get a second version (or any other), that may change. Also, if someone recalls this to be fifty, quoting source allows him/her to check it.

  3. Is subValue method used anywhere?

  4. Aptitude max and aptitudes array are both public static. Is that the way you want it?

Character

This class has 1357 lines. Consider splitting it to several classes. There's too much functionality in one class.

======= UPDATE =========================

It's for the most part just a giant container that organizes the various stats and container classes, the only thing I could think of doing is making some larger containers to group things in, like a Skills or Sleights class?

That's some start, but read on for more pointers.

Currently it's hard to judge what is it, when should I change it and when I absolutely should abstain from changing it.

Single Responsibility Principle. Character class breaks it, dealing with many things. This will make you add more to it (after all, it's a giant container) and after some time, everything will happen in Character class. Then, simple mistake will have numerous consequences in different places.

Pointers:

  • cut Javadoc
  • fix wrong Javadoc ref for trait (see where my cursor is in the image below, red t)
  • fight warnings, there's 88 warnings in my IDE just in that file
  • consider character builder: class that takes character and builds it, adds aptitudes, randomizes whatever character has by fate, etc. That way you'll have character value object, being a simple container for character AND - separately - logic for building it, or filling it out

sample warnings in Character class ======= UPDATE END======================

Indentation

You are indenting your files with Tabs. That's quite OK, but be aware, that your tab and mine may differ. I can set my OS to make Tab equal to 8 spaces. You may set it to 4. Somebody else, to two. That means, the indentation will not be consistent. IIRC, the standard is to set Tabs to 4 spaces and set your IDE to use SPACES instead. This way, when you hit Tab, IDE inserts 4 spaces and behaviour is consistent across everybody's machine.

Still - your flavour here. That's minor thing IMO (though some folks rage wars over it).

Your specific questions

Last but not least!

Is GridBagUIPanel a proper abstraction of functionality, or is it too little/too much?

At 557 lines of code, I'd say it needs a split. Number of methods should be extracted out of it. Why do you have 4 different constructors there? What about shortening 4 methods to add components into one, using int vararg? Is it really panel's job to have 4 methods to add components and 8 more methods to add other things? Perhaps you need PanelAdder or PanelDecorator where all those add methods would be? Such a class needs only one private field Panel and it can operate on this panel and add things to it as you please.

Is there anything I should be doing to clean up init()/update() more in LifePathUI?

YES.

  1. When constructing your application, read both internal files into memory and keep them there. No need to init data proc when creating UI. These files are crucial and you will need them sooner or later. Don't bundle them with UI, bundle them with domain. That clears dependency I've spotted in LifePathUI's constructor.

  2. Init method is rather long. Consider grouping relevant code together and putting it in functions. Consider creating object, which only has one purpose: to initialize LifePathUI. LifePathInitializer. Whenever you spot a piece of code with comment at the top, it's prime candidate for a method.

  3. Aim for fluent interface (see below).

Consider this code:

import javax.swing.*;

/**
 * Created by LAFK on 01.09.15.
 */
public class MyJFrame extends JFrame {

    private final MyJFrame frame;

    public MyJFrame(JFrame frame) {
        this.frame = (MyJFrame)frame;
    }

    public MyJFrame addStatPanel(JPanel panel) {
        frame.add(panel);
        return frame;
    }
}

Now, let's say it has more methods, including addBonuses, addTotal, addSomethingElse and I can create MyJFrame and just add things to it easily, within few lines. Let me know if this is unclear.

Am I using things right with the TextChangeListeners and Button Events in LifePathUI? Defining classes inside method calls still feels

very off to me.

You'll get used to defining classes on the fly.

Is there any glaring reinventing the wheel or other mistakes going on? I want to cut out any particularly embarrassing things since this

is one of my few projects that would be able to be used in portfolios and such.

Strings vs Objects. Lack of packages. Public in interfaces. Too many unused things. Fix these and you're much better looking IMO. Also - if you want to present it to someone who likes testing, do change the name of the class UnitTests, as you may get scolded. :-)

Finally

Do not be discouraged. Despite all my findings, this is not such a bad code (that's not sarcasm). You obviously put lot of work into it and this can be spotted. There's just number of things yet to do.

Kudos for:

  • coding this in the first place
  • putting it up here for code review
  • putting it on GitHub = sharing with everyone
  • writing UI for it, number of such tools uses only esoteric CLI switches
  • keeping files from Eclipse Phase in the repo
  • writing readme for others
  • writing number of comments in your code
\$\endgroup\$
3
  • \$\begingroup\$ Wow, this is great, thanks! Only thing I could use clarification on is, aside from removing some of the autogen javadoc, what can be done with Character class? It's for the most part just a giant container that organizes the various stats and container classes, the only thing I could think of doing is making some larger containers to group things in, like a Skills or Sleights class? \$\endgroup\$
    – Vigilant
    Sep 2, 2015 at 1:06
  • \$\begingroup\$ My mistake, missed two other things. Are you sure about the redundant ints in Character? Math.round returns long, which is why it's getting cast. Also for Aptitude, the only place aptitudes is used is somewhere where it's immediately iterated and used as a String. I agree thought that for Sleight, and possibly others like Morph, I should put some enums to work, true. \$\endgroup\$
    – Vigilant
    Sep 2, 2015 at 5:04
  • \$\begingroup\$ Np @Vigilant. Unused casts: yes, IDE said so and it does auto-verification. Of course, there's a chance they have bug in it, but I'd also say it's safe due to type promotion. Still, check it. Change, run the app, see for yourself. Or even better, write a test, change the code, see if test fails. Real unit tests will look good here. Character class... I'll extend my answer, as it fits there more than here in comment. \$\endgroup\$ Sep 5, 2015 at 9:27
2
\$\begingroup\$

I only have minor suggestions:

    if (resp == JOptionPane.YES_OPTION)
    {
        return true;
    }
    else            
    {
        return false;
    }       

Should become:

return resp == JOptionPane.YES_OPTION;

Because if bool_condition return true else return false should become return bool_condition in general.


About this behaviour:

This function will fail silently if the button doesn't exist

Are you sure that it is the best choice? I tend to follow the Fail Fast Fail Loudly style and would throw an exception. If I try to set the text of a non-existent button, it must be a bug.

\$\endgroup\$
1
\$\begingroup\$

Perhaps methods of GridBagUIPanel: setButtonText(), getTextF() and etc would be better if to check a class like:

!(temp instanceof JButton)
!(temp instanceof JTextField)
\$\endgroup\$

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.